Обсуждение: 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=65000000'),
   '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

On Wed, Feb 10, 2016 at 12:44 AM, Jeff Janes <jeff.janes@gmail.com> 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=65000000'),
>    '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

On Fri, Feb 12, 2016 at 2:46 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Feb 10, 2016 at 12:44 AM, Jeff Janes <jeff.janes@gmail.com> 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

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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Mar 8, 2016 at 4:09 PM, Alvaro Herrera <alvherre@2ndquadrant.com> 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

Jeff Janes wrote:
> On Tue, Mar 8, 2016 at 4:09 PM, Alvaro Herrera <alvherre@2ndquadrant.com> 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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services