Re: [HACKERS] pgcrypto: add s2k-count
Jeff Janes wrote: > On Tue, Mar 8, 2016 at 4:09 PM, Alvaro Herrera > wrote: > Yeah, I find that pretty impenetrable too. I just treated it as a > black box, I changed how the number passed into it gets set, but not > the meaning of that number. Initially I had the user set the one-byte > format directly because that was much simpler, but before submitting > the patch I changed it to take the human-readable value and do the > conversion to the one byte format, because the gpg command-line tool > takes the number of iterations, not the one byte format, as the input > for its own s2k-count setting. Funny -- I partially edited the patch to use the one-byte number instead too, because that seemed more reasonable, but eventually (looking at gnupg) decided not to. And deleted the email on which I explained that, without sending. > > I would love to be able to read gnupg's code to figure out what it is > > that they do, but the structure of their code is even more impenetrable > > than pgcrypto's. Perhaps it would be easier to measure the time it > > takes to run some s2k operations ... > > The timings are about the same between the patched pgcrypto and gpg > when using the same settings for s2k-count. Also, if I encrypt with > gpg with a certain setting, pgcrypto properly detects that iteration > count and uses it (if it didn't get it correct, it would be unable to > decrypt). And vice versa. OK, it seems we're good then. > Do we know why the default for pgcrypto is to use a stochastic number > of iterations? I don't see how that can enhance security, as the > number of iterations actually done is not a secret. Nope, unless Marko has some input there. I find it baffling too. > And I see that you committed it now, so thanks for that too. You're welcome, thanks for the patch. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: add s2k-count
On Tue, Mar 8, 2016 at 4:09 PM, Alvaro Herrera wrote: > Jeff Janes wrote: >> pgcrypto supports s2k-mode for key-stretching during symmetric >> encryption, and even defaults to s2k-mode=3, which means configurable >> iterations. But it doesn't support s2k-count to actually set those >> iterations to be anything other than the default. If you are >> interested in key-stretching, the default is not going to cut it. > > Talking about deep rabbit holes ... > > I gave this a look. I adjusted it here and there for project style and > general cleanliness (something that could be applied to pgcrypto much > more generally) and eventually arrived at trying to figure out how is > the s2k count actually used by the underlying algorithm. I arrived at > the function calc_s2k_iter_salted which is where it is actually used to > encrypt things. But that function is completely devoid of comments and > not completely trivial. At this point I cannot for the life of me > determine whether that function should use the one-byte format specified > by the relevant RFC (4880) or the decoded, human-understandable number > of iterations. Thanks for taking this up. Yeah, I find that pretty impenetrable too. I just treated it as a black box, I changed how the number passed into it gets set, but not the meaning of that number. Initially I had the user set the one-byte format directly because that was much simpler, but before submitting the patch I changed it to take the human-readable value and do the conversion to the one byte format, because the gpg command-line tool takes the number of iterations, not the one byte format, as the input for its own s2k-count setting. > I would love to be able to read gnupg's code to figure out what it is > that they do, but the structure of their code is even more impenetrable > than pgcrypto's. Perhaps it would be easier to measure the time it > takes to run some s2k operations ... The timings are about the same between the patched pgcrypto and gpg when using the same settings for s2k-count. Also, if I encrypt with gpg with a certain setting, pgcrypto properly detects that iteration count and uses it (if it didn't get it correct, it would be unable to decrypt). And vice versa. Do we know why the default for pgcrypto is to use a stochastic number of iterations? I don't see how that can enhance security, as the number of iterations actually done is not a secret. And I see that you committed it now, so thanks for that too. Cheers, Jeff -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: add s2k-count
Alvaro Herrera wrote: > Anyway, assuming that the iteration count was already being used > correctly, then as far as I'm concerned we're ready to go. The attached > patch is what I would commit. I read some more (gnupg code as well as our own) and applied some more tweaks, and pushed. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: add s2k-count
Jeff Janes wrote: > pgcrypto supports s2k-mode for key-stretching during symmetric > encryption, and even defaults to s2k-mode=3, which means configurable > iterations. But it doesn't support s2k-count to actually set those > iterations to be anything other than the default. If you are > interested in key-stretching, the default is not going to cut it. Talking about deep rabbit holes ... I gave this a look. I adjusted it here and there for project style and general cleanliness (something that could be applied to pgcrypto much more generally) and eventually arrived at trying to figure out how is the s2k count actually used by the underlying algorithm. I arrived at the function calc_s2k_iter_salted which is where it is actually used to encrypt things. But that function is completely devoid of comments and not completely trivial. At this point I cannot for the life of me determine whether that function should use the one-byte format specified by the relevant RFC (4880) or the decoded, human-understandable number of iterations. I would love to be able to read gnupg's code to figure out what it is that they do, but the structure of their code is even more impenetrable than pgcrypto's. Perhaps it would be easier to measure the time it takes to run some s2k operations ... I CCed Marko here. Hopefully he can chime in on whether this patch is correct. Anyway, assuming that the iteration count was already being used correctly, then as far as I'm concerned we're ready to go. The attached patch is what I would commit. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services diff --git a/contrib/pgcrypto/expected/pgp-encrypt.out b/contrib/pgcrypto/expected/pgp-encrypt.out index b35de79..8fc558c 100644 --- a/contrib/pgcrypto/expected/pgp-encrypt.out +++ b/contrib/pgcrypto/expected/pgp-encrypt.out @@ -103,6 +103,25 @@ select pgp_sym_decrypt( Secret. (1 row) +-- s2k count change +select pgp_sym_decrypt( + pgp_sym_encrypt('Secret.', 'key', 's2k-count=1024'), + 'key', 'expect-s2k-count=1024'); + pgp_sym_decrypt +- + Secret. +(1 row) + +-- s2k_count rounds up +select pgp_sym_decrypt( + pgp_sym_encrypt('Secret.', 'key', 's2k-count=6500'), + 'key', 'expect-s2k-count=6500'); +NOTICE: pgp_decrypt: unexpected s2k_count: expected 6500 got 65011712 + pgp_sym_decrypt +- + Secret. +(1 row) + -- s2k digest change select pgp_sym_decrypt( pgp_sym_encrypt('Secret.', 'key', 's2k-digest-algo=md5'), diff --git a/contrib/pgcrypto/pgp-decrypt.c b/contrib/pgcrypto/pgp-decrypt.c index 5c69745..37f374a 100644 --- a/contrib/pgcrypto/pgp-decrypt.c +++ b/contrib/pgcrypto/pgp-decrypt.c @@ -643,6 +643,7 @@ parse_symenc_sesskey(PGP_Context *ctx, PullFilter *src) if (res < 0) return res; ctx->s2k_mode = ctx->s2k.mode; + ctx->s2k_count = s2k_iter_to_count(ctx->s2k.iter); ctx->s2k_digest_algo = ctx->s2k.digest_algo; /* diff --git a/contrib/pgcrypto/pgp-encrypt.c b/contrib/pgcrypto/pgp-encrypt.c index 2320c75..c9148fd 100644 --- a/contrib/pgcrypto/pgp-encrypt.c +++ b/contrib/pgcrypto/pgp-encrypt.c @@ -567,7 +567,7 @@ init_s2k_key(PGP_Context *ctx) if (ctx->s2k_cipher_algo < 0) ctx->s2k_cipher_algo = ctx->cipher_algo; - res = pgp_s2k_fill(&ctx->s2k, ctx->s2k_mode, ctx->s2k_digest_algo); + res = pgp_s2k_fill(&ctx->s2k, ctx->s2k_mode, ctx->s2k_digest_algo, ctx->s2k_count); if (res < 0) return res; diff --git a/contrib/pgcrypto/pgp-pgsql.c b/contrib/pgcrypto/pgp-pgsql.c index 1842985..1f65b66 100644 --- a/contrib/pgcrypto/pgp-pgsql.c +++ b/contrib/pgcrypto/pgp-pgsql.c @@ -181,6 +181,7 @@ struct debug_expect int expect; int cipher_algo; int s2k_mode; + int s2k_count; int s2k_cipher_algo; int s2k_digest_algo; int compress_algo; @@ -196,6 +197,7 @@ fill_expect(struct debug_expect * ex, int text_mode) ex->expect = 0; ex->cipher_algo = -1; ex->s2k_mode = -1; + ex->s2k_count = -1; ex->s2k_cipher_algo = -1; ex->s2k_digest_algo = -1; ex->compress_algo = -1; @@ -218,6 +220,7 @@ check_expect(PGP_Context *ctx, struct debug_expect * ex) { EX_CHECK(cipher_algo); EX_CHECK(s2k_mode); + EX_CHECK(s2k_count); EX_CHECK(s2k_digest_algo); EX_CHECK(use_sess_key); if (ctx->use_sess_key) @@ -247,6 +250,8 @@ set_arg(PGP_Context *ctx, char *key, char *val, res = pgp_set_sess_key(ctx, atoi(val)); else if (strcmp(key, "s2k-mode") == 0) res = pgp_set_s2k_mode(ctx, atoi(val)); + else if (strcmp(key, "s2k-count") == 0) + res = pgp_set_s2k_count(ctx, atoi(val)); else if (strcmp(key, "s2k-digest-algo") == 0) res = pgp_set_s2k_digest_algo(ctx, val); else if (strcmp(key, "s2k-cipher-algo") == 0) @@ -286,6 +291,11 @@ set_arg(PGP_Context *ctx, char *key, char *val, ex->expect = 1; ex->s2k_mode = atoi(val); } + else if (ex != NULL && strcmp(key, "expect-s2k-count") == 0) + { + ex->expect = 1; + ex->s2k_count = atoi(val); + } else if (ex != NULL && str
Re: [HACKERS] pgcrypto: add s2k-count
On Fri, Feb 12, 2016 at 2:46 AM, Robert Haas wrote: > On Wed, Feb 10, 2016 at 12:44 AM, Jeff Janes wrote: >> I did not bump the extension version. I realized the migration file >> would be empty, as there no change to SQL-level functionality (the new >> s2k-count is parsed out of a string down in the C code). Since only >> one version of contrib extensions binary object files are installed in >> any given postgres installation, people using the newer binary gets >> the feature even if they have not updated the extension version. So I >> don't know if it makes sense to bump the version if people inherently >> get the feature anyway. > > There's zero reason to bump the extension version if the SQL interface > hasn't changed. > > (I have no opinion on the underlying patch.) +1. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgcrypto: add s2k-count
On Wed, Feb 10, 2016 at 12:44 AM, Jeff Janes wrote: > pgcrypto supports s2k-mode for key-stretching during symmetric > encryption, and even defaults to s2k-mode=3, which means configurable > iterations. But it doesn't support s2k-count to actually set those > iterations to be anything other than the default. If you are > interested in key-stretching, the default is not going to cut it. > (You could argue that pgp's s2k doesn't cut it either even at the max, > but at least we should offer the maximum that the pgp spec makes > available.) > > This patch implements s2k-count as an option to pgp_sym_encrypt. > > Demo (note the password is intentionally wrong in the last character): > > select pgp_sym_decrypt( > pgp_sym_encrypt('foobar','acf86729b6b0289f4d1909db8c1aaf0c','s2k-mode=3'), > 'acf86729b6b0289f4d1909db8c1aaf0d'); > ERROR: Wrong key or corrupt data > Time: 1.606 ms > > select pgp_sym_decrypt( > > pgp_sym_encrypt('foobar','acf86729b6b0289f4d1909db8c1aaf0c','s2k-mode=3,s2k-count=6500'), >'acf86729b6b0289f4d1909db8c1aaf0d'); > ERROR: Wrong key or corrupt data > Time: 615.720 ms > > I did not bump the extension version. I realized the migration file > would be empty, as there no change to SQL-level functionality (the new > s2k-count is parsed out of a string down in the C code). Since only > one version of contrib extensions binary object files are installed in > any given postgres installation, people using the newer binary gets > the feature even if they have not updated the extension version. So I > don't know if it makes sense to bump the version if people inherently > get the feature anyway. There's zero reason to bump the extension version if the SQL interface hasn't changed. (I have no opinion on the underlying patch.) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] pgcrypto: add s2k-count
pgcrypto supports s2k-mode for key-stretching during symmetric encryption, and even defaults to s2k-mode=3, which means configurable iterations. But it doesn't support s2k-count to actually set those iterations to be anything other than the default. If you are interested in key-stretching, the default is not going to cut it. (You could argue that pgp's s2k doesn't cut it either even at the max, but at least we should offer the maximum that the pgp spec makes available.) This patch implements s2k-count as an option to pgp_sym_encrypt. Demo (note the password is intentionally wrong in the last character): select pgp_sym_decrypt( pgp_sym_encrypt('foobar','acf86729b6b0289f4d1909db8c1aaf0c','s2k-mode=3'), 'acf86729b6b0289f4d1909db8c1aaf0d'); ERROR: Wrong key or corrupt data Time: 1.606 ms select pgp_sym_decrypt( pgp_sym_encrypt('foobar','acf86729b6b0289f4d1909db8c1aaf0c','s2k-mode=3,s2k-count=6500'), 'acf86729b6b0289f4d1909db8c1aaf0d'); ERROR: Wrong key or corrupt data Time: 615.720 ms I did not bump the extension version. I realized the migration file would be empty, as there no change to SQL-level functionality (the new s2k-count is parsed out of a string down in the C code). Since only one version of contrib extensions binary object files are installed in any given postgres installation, people using the newer binary gets the feature even if they have not updated the extension version. So I don't know if it makes sense to bump the version if people inherently get the feature anyway. Cheers, Jeff diff --git a/contrib/pgcrypto/expected/pgp-encrypt.out b/contrib/pgcrypto/expected/pgp-encrypt.out new file mode 100644 index b35de79..2bf999f *** a/contrib/pgcrypto/expected/pgp-encrypt.out --- b/contrib/pgcrypto/expected/pgp-encrypt.out *** select pgp_sym_decrypt( *** 103,108 --- 103,126 Secret. (1 row) + -- s2k count change + select pgp_sym_decrypt( + pgp_sym_encrypt('Secret.', 'key', 's2k-count=1024'), + 'key', 'expect-s2k-count=1024'); + pgp_sym_decrypt + - + Secret. + (1 row) + + select pgp_sym_decrypt( + pgp_sym_encrypt('Secret.', 'key', 's2k-count=6500'), + 'key', 'expect-s2k-count=6500'); -- rounded up + NOTICE: pgp_decrypt: unexpected s2k_count: expected 6500 got 65011712 + pgp_sym_decrypt + - + Secret. + (1 row) + -- s2k digest change select pgp_sym_decrypt( pgp_sym_encrypt('Secret.', 'key', 's2k-digest-algo=md5'), diff --git a/contrib/pgcrypto/pgp-decrypt.c b/contrib/pgcrypto/pgp-decrypt.c new file mode 100644 index 5c69745..3d16033 *** a/contrib/pgcrypto/pgp-decrypt.c --- b/contrib/pgcrypto/pgp-decrypt.c *** parse_symenc_sesskey(PGP_Context *ctx, P *** 643,648 --- 643,649 if (res < 0) return res; ctx->s2k_mode = ctx->s2k.mode; + ctx->s2k_count = iter_to_count(ctx->s2k.iter); ctx->s2k_digest_algo = ctx->s2k.digest_algo; /* diff --git a/contrib/pgcrypto/pgp-encrypt.c b/contrib/pgcrypto/pgp-encrypt.c new file mode 100644 index 2320c75..c9148fd *** a/contrib/pgcrypto/pgp-encrypt.c --- b/contrib/pgcrypto/pgp-encrypt.c *** init_s2k_key(PGP_Context *ctx) *** 567,573 if (ctx->s2k_cipher_algo < 0) ctx->s2k_cipher_algo = ctx->cipher_algo; ! res = pgp_s2k_fill(&ctx->s2k, ctx->s2k_mode, ctx->s2k_digest_algo); if (res < 0) return res; --- 567,573 if (ctx->s2k_cipher_algo < 0) ctx->s2k_cipher_algo = ctx->cipher_algo; ! res = pgp_s2k_fill(&ctx->s2k, ctx->s2k_mode, ctx->s2k_digest_algo, ctx->s2k_count); if (res < 0) return res; diff --git a/contrib/pgcrypto/pgp-pgsql.c b/contrib/pgcrypto/pgp-pgsql.c new file mode 100644 index 1842985..1b55bc9 *** a/contrib/pgcrypto/pgp-pgsql.c --- b/contrib/pgcrypto/pgp-pgsql.c *** struct debug_expect *** 181,186 --- 181,187 int expect; int cipher_algo; int s2k_mode; + int s2k_count; int s2k_cipher_algo; int s2k_digest_algo; int compress_algo; *** fill_expect(struct debug_expect * ex, in *** 196,201 --- 197,203 ex->expect = 0; ex->cipher_algo = -1; ex->s2k_mode = -1; + ex->s2k_count= -1; ex->s2k_cipher_algo = -1; ex->s2k_digest_algo = -1; ex->compress_algo = -1; *** check_expect(PGP_Context *ctx, struct de *** 218,223 --- 220,226 { EX_CHECK(cipher_algo); EX_CHECK(s2k_mode); + EX_CHECK(s2k_count); EX_CHECK(s2k_digest_algo); EX_CHECK(use_sess_key); if (ctx->use_sess_key) *** set_arg(PGP_Context *ctx, char *key, cha *** 247,252 --- 250,257 res = pgp_set_sess_key(ctx, atoi(val)); else if (strcmp(key, "s2k-mode") == 0) res = pgp_set_s2k_mode(ctx, atoi(val)); + else if (strcmp(key, "s2k-count") == 0) + res = pgp_set_s2k_count(ctx, atoi(val)); else if (strcmp(key, "s2k-digest-algo") == 0) res = pgp_set_s2k_digest_algo(ctx, val); else if (strcmp(key, "s2k-cipher-algo") == 0)