Re: [HACKERS] pgcrypto: add s2k-count

[Alvaro Herrera]

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

[Jeff Janes]

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]

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

[Alvaro Herrera]

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(>s2k, ctx->s2k_mode, ctx->s2k_digest_algo);
+	res = pgp_s2k_fill(>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 && strcmp(key, 

Re: [HACKERS] pgcrypto: add s2k-count

[Michael Paquier]

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

[Robert Haas]

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

[Jeff Janes]

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(>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(>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)
***