Subject: Raising the SCRAM iteration count

From: Daniel Gustafsson <[email-masked]>
Date: Fri, 9 Dec 2022 11:55:07 +0100

In the thread about user space SCRAM functions [0] I mentioned that it might be
wise to consider raising our SCRAM iteration count.  The iteration count is an
important defence against brute-force attacks.

Our current hardcoded value for iteration count is 4096, which is based on a
recommendation from RFC 7677.  This is however the lower end of the scale, and
is related to computing power in 2015 generation handheld devices.  The
relevant paragraph in section 4 of RFC 7677 [1] reads:

   "As a rule of thumb, the hash iteration-count should be such that a modern
   machine will take 0.1 seconds to perform the complete algorithm; however,
   this is unlikely to be practical on mobile devices and other relatively low-
   performance systems.  At the time this was written, the rule of thumb gives
   around 15,000 iterations required; however, a hash iteration- count of 4096
   takes around 0.5 seconds on current mobile handsets."

It goes on to say:

   "..the recommendation of this specification is that the hash iteration- count
   SHOULD be at least 4096, but careful consideration ought to be given to
   using a significantly higher value, particularly where mobile use is less
   important."

Selecting 4096 was thus a conservative take already in 2015, and is now very
much so.  On my 2020-vintage Macbook I need ~200k iterations to consume 0.1
seconds (in a build with assertions).  Calculating tens of thousands of hashes
per second on a consumer laptop at a 4096 iteration count is no stretch.  A
brief look shows that MongoDB has a minimum of 5000 with a default of 15000
[2]; Kafka has a minimum of 4096 [3].

Making the iteration count a configurable setting would allow installations to
raise the iteration count to strengthen against brute force attacks, while
still supporting those with lower end clients who prefer the trade-off of
shorter authentication times.

The attached introduces a scram_iteration_count GUC with a default of 15000
(still conservative, from RFC7677) and a minimum of 4096.  Since the iterations
are stored per secret it can be altered with backwards compatibility.

Clientside the count is still at 4096 to limit the scope of this patch a bit.
For psql it would mean adding options to \password which should be a thread of
its own.  For libpq one can imagine specifying this in the algorithm parameter
passed to PQencryptPasswordConn like "scram-sha-256:100000" or something
similar.  It's premature to pursue those without agreement that we should make
the count configurable though.  If this patch is accepted, I'll work on that
next.

Thoughts?

--
Daniel Gustafsson		https://vmware.com/

[0] [email-masked]
[1] https://www.rfc-editor.org/rfc/rfc7677#section-4
[2] https://www.mongodb.com/docs/manual/reference/parameters/#mongodb-parameter-param.scramSHA256IterationCount
[3] https://docs.confluent.io/platform/current/kafka/authentication_sasl/authentication_sasl_scram.html#security-considerations-for-sasl-scram

---

From: Heikki Linnakangas <[email-masked]>
Date: Fri, 9 Dec 2022 17:50:00 +0200

> are stored per secret it can be altered with backwards compatibility.

We just had a discussion with a colleague about using a *smaller* 
iteration count. Why? To make the connection startup faster. We're 
experimenting with a client that runs in a Cloudflare worker, which is a 
wasm runtime with very small limits on how much CPU time you're allowed 
to use (without paying extra). And we know that the password is randomly 
generated and long enough. If I understand correctly, the point of 
iterations is to slow down brute-force or dictionary attacks, but if the 
password is strong enough to begin with, those attacks are not possible 
regardless of iteration count. So I would actually like to set the 
minimum iteration count all the way down to 1.

- Heikki

---

From: Michael Paquier <[email-masked]>
Date: Sat, 10 Dec 2022 08:21:51 +0900

On Fri, Dec 09, 2022 at 05:50:00PM +0200, Heikki Linnakangas wrote:
> We just had a discussion with a colleague about using a *smaller* iteration
> count. Why? To make the connection startup faster. We're experimenting with
> a client that runs in a Cloudflare worker, which is a wasm runtime with very
> small limits on how much CPU time you're allowed to use (without paying
> extra). And we know that the password is randomly generated and long enough.
> If I understand correctly, the point of iterations is to slow down
> brute-force or dictionary attacks, but if the password is strong enough to
> begin with, those attacks are not possible regardless of iteration count. So
> I would actually like to set the minimum iteration count all the way down to
> 1.

This is the kind of thing that should be easily measurable with
pgbench -C and an empty script.  How much difference are you seeing
with 1, 4096 and more than that?

All that comes down to provide more capability for the existing
routines in my opinion.  So what if we finally extended with a new
flavor PQencryptPasswordConn() able to get a list of options, say
PQencryptPasswordConn() extended that has a string with all the
options?  psql could use for \password a grammar consistent with \g,
as of: \password (iteration=4096, salt_length=123) PASS_STR

Note that scram_build_secret() is already able to handle any iteration
count, even at 1, so IMO it is not a good idea to lower the default to
be so.  I'd agree with Daniel to make it higher by default and follow
the RFCs, though like you I have wanted also in core much more control
over that.
--
Michael

---

From: Andres Freund <[email-masked]>
Date: Fri, 9 Dec 2022 16:15:38 -0800

Hi,

> are stored per secret it can be altered with backwards compatibility.

I am extremely doubtful it's a good idea to increase the default (if anything
the opposite). 0.1 seconds is many times the connection establishment
overhead, even over network.  I've seen users complain about postgres
connection establishment overhead being high, and it just turned out to be due
to scram - yes, they ended up switching to md5, because that was the only
viable alternative.

PGPASSWORD=passme pgbench -n -C -f ~/tmp/select.sql -h 127.0.0.1 -T10 -U passme

md5: tps = 158.577609
scram: tps = 38.196362

Greetings,

Andres Freund

---

From: Daniel Gustafsson <[email-masked]>
Date: Sun, 11 Dec 2022 00:46:23 +0100

> On 10 Dec 2022, at 01:15, Andres Freund <[email-masked]> wrote:

> to scram - yes, they ended up switching to md5, because that was the only

That's a fair point.  For the record I don't think we should raise the default
to match 0.1 seconds, but we should make the option available to those who want
it.  If we provide a GUC for the iteration count which has a lower limit than
todays hardcoded value, then maybe we can help workloads with long-lived
connections who want increased on-disk safety as well as workloads where low
connection establishment is critical (or where the env is constrained like in
Heikki's example).

> PGPASSWORD=passme pgbench -n -C -f ~/tmp/select.sql -h 127.0.0.1 -T10 -U passme

Lowering the minimum for scram_iteration_count I tried out the patch on a set
of iteration counts of interest.  Values are averaged over three runs, using
the same pgbench setup you had above with basically a noop select.sql.  The
relative difference between the values are way off from your results, but I
haven't done much digging to figure that out yet (different OpenSSL versions
might be one factor).

md5: tps = 154.052690
scram 1: tps = 150.060285
scram 1024: tps = 138.191224
scram 4096: tps = 115.197533
scram 15000: tps = 75.156399

For the fun of it, 100000 iterations yields tps = 20.822393.

SCRAM with an iteration count of 1 still provides a lot of benefits over md5,
so if we can make those comparable in performance then that could be a way
forward (with the tradeoffs properly documented).

---

From: Michael Paquier <[email-masked]>
Date: Sun, 11 Dec 2022 12:32:38 +0900

> forward (with the tradeoffs properly documented).

Okay, it looks like there is a wish to make that configurable anyway,
and I have a few comments about that.

       {"scram_iteration_count", PGC_SUSET, CONN_AUTH_AUTH,
+           gettext_noop("Sets the iteration count for SCRAM secret generation."),
+           NULL,
+           GUC_NOT_IN_SAMPLE | GUC_SUPERUSER_ONLY
+       },

Shouldn't this be user-settable as a PGC_USERSET rather than
PGC_SUSET which would limit its updates to superusers?

As shaped, the GUC would not benefit to \password, and we should not
encourage users to give a raw password over the wire if possible if
they wish to compute a verifier with a given interation number.
Hence, wouldn't it be better to mark it as GUC_REPORT, and store its 
status in [email-masked] in the same fashion as
default_transaction_read_only and hot_standby?  This way,
PQencryptPasswordConn() would be able to feed on it automatically
rather than always assume the default implied by
pg_fe_scram_build_secret().
--
Michael

---

From: "Jonathan S. Katz" <[email-masked]>
Date: Mon, 12 Dec 2022 09:47:56 -0500

>> are stored per secret it can be altered with backwards compatibility.

To throw on a bit of paint, if we do change it, we should likely follow 
what would come out in a RFC.

While the SCRAM-SHA-512 RFC is still in draft[1], the latest draft it 
contains a "SHOULD" recommendation of 10000, which was bumped up from 
4096 in an earlier version of the draft:

==snip==
Therefore, the recommendation of this specification is that the hash 
iteration- count SHOULD be at least 10000, but careful consideration 
ought to be given to using a significantly higher value, particularly 
where mobile use is less important.¶
==snip==

I'm currently ambivalent (+0) on changing the default. I think giving 
the user more control over iterations ([2], and follow up work to make 
it easier to set iteration account via client) can help with this.

However, I do like the idea of a GUC.

> to scram - yes, they ended up switching to md5, because that was the only

Ugh, I'd be curious to know how often that is the case. That said, I 
think some of the above work could help with that.

Thanks,

Jonathan

[1] https://datatracker.ietf.org/doc/html/draft-melnikov-scram-sha-512
[2] https://postgr.es/m/[email-masked]/

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 13 Dec 2022 12:17:58 +0100

> On 12 Dec 2022, at 15:47, Jonathan S. Katz <[email-masked]> wrote:

> To throw on a bit of paint, if we do change it, we should likely follow what would come out in a RFC.
> 
> While the SCRAM-SHA-512 RFC is still in draft[1], the latest draft it contains a "SHOULD" recommendation of 10000, which was bumped up from 4096 in an earlier version of the draft:

This is however the draft for a different algorithm: SCRAM-SHA-512.  We are
supporting SCRAM-SHA-256 which is defined in RFC7677.  The slightly lower
recommendation there makes sense as SHA-512 is more computationally expensive
than SHA-256.

It does raise an interesting point though, if we in the future add suppprt for
SCRAM-SHA-512 (which seems reasonable to do) it's not good enough to have a
single GUC for SCRAM iterations; we'd need to be able to set the iteration
count per algorithm. I'll account for that when updating the patch downthread.

---

From: Michael Paquier <[email-masked]>
Date: Wed, 14 Dec 2022 10:00:13 +0900

> count per algorithm. I'll account for that when updating the patch downthread.

So, you mean that the GUC should be named like password_iterations,
taking a grammar with a list like 'scram-sha-256=4096,algo2=5000'?
--
Michael

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 14 Dec 2022 12:25:19 +0100

> On 14 Dec 2022, at 02:00, Michael Paquier <[email-masked]> wrote:
> 

I was thinking about it but opted for the simpler approach of a GUC name with
the algorithm baked into it: scram_sha256_iterations.  It doesn't seem all that
likely that we'll have more than two versions of SCRAM (sha256/sha512) so
the additional complexity doesn't seem worth it.

The attached v2 has the GUC rename and a change to GUC_REPORT such that the
frontend can use the real value rather than the default.  I kept it for super
users so far, do you think it should be a user setting being somewhat sensitive? 

The default in this version is rolled back to 4096 as there was pushback
against raising it, and the lower limit is one in order to potentially assist
situations like the one Andres mentioned where md5 is used.

---

From: "Jonathan S. Katz" <[email-masked]>
Date: Wed, 14 Dec 2022 13:59:04 -0500

> the additional complexity doesn't seem worth it.

I would not rule this out. There is a RFC draft for SCRAM-SHA3-512[1].

I do have mixed feelings on the 'x1=y1,x2=y2' style GUC, but we do have 
machinery to handle it and it gives a bit more flexibility over how many 
SCRAM hash methods get added. I'd like to hear more feedback.

(I don't know if there will be a world if we ever let users BYO-hash, 
but that case may force separate GUCs anyway).

[1] https://datatracker.ietf.org/doc/draft-melnikov-scram-sha3-512/

> users so far, do you think it should be a user setting being somewhat sensitive?

No, because a user can set the number of iterations today if they build 
their own SCRAM secret. I think it's OK if they change it in a session.

If a superuser wants to enforce a minimum iteration count, they can 
write a password_check_hook. (Or we could add another GUC to enforce that).

> situations like the one Andres mentioned where md5 is used.

Reviewing patch as is.

Suggestion on text:

==snip==
The number of computational iterations to perform when generating
a SCRAM-SHA-256 secret. The default is <literal>4096</literal>. A
higher number of iterations provides additional protection against
brute-force attacks on stored passwords, but makes authentication
slower. Changing the value has no effect on previously created
SCRAM-SHA-256 secrets as the iteration count at the time of creation
is fixed. A password must be re-hashed to use an updated iteration
value.
==snip==

  /*
- * Default number of iterations when generating secret.  Should be at least
- * 4096 per RFC 7677.
+ * Default number of iterations when generating secret.
   */

I don't think we should remove the RFC 7677 reference entirely. Perhaps:

/*
  * Default number of iterations when generating secret. RFC 7677
  * recommend 4096 for SCRAM-SHA-256, which we set as the default,
  * but we allow users to select their own values.
  */

-pg_fe_scram_build_secret(const char *password, const char **errstr)
+pg_fe_scram_build_secret(const char *password, int iterations, const 
char **errstr)

I have mild worry about changing this function definition for downstream 
usage, esp. for drivers. Perhaps it's not that big of a deal, and 
perhaps this will end up being needed for the work we've discussed 
around "\password" but I do want to note that this could be a breaking 
change.

+	else if (strcmp(name, "scram_sha256_iterations") == 0)
+	{
+		conn->scram_iterations = atoi(value);
+	}

Maybe out of scope for this patch based on what else is in the patch, 
but I was wondering why we don't use a "strncmp" here?

Thanks,

Jonathan

---

From: Michael Paquier <[email-masked]>
Date: Thu, 15 Dec 2022 08:52:32 +0900

> SCRAM hash methods get added. I'd like to hear more feedback.

Technically, I would put the logic to parse the GUC to scram-common.c
and let libpq and the backend use it.  Saying that, we are just
talking about what looks like one new hashing method, so a separate
GUC is fine by me.

> (I don't know if there will be a world if we ever let users BYO-hash, but
> that case may force separate GUCs anyway).
> 

Still, the odds is that we are going to see one update to
SCRAM-SHA-256 that we will just need to pick up?

> If a superuser wants to enforce a minimum iteration count, they can write a
> password_check_hook. (Or we could add another GUC to enforce that).

Hm?  check_password_hook does not allow one to recompile the password
given by the user, except if I am missing your point?

> +pg_fe_scram_build_secret(const char *password, int iterations, const char
> **errstr)
> 
> usage, esp. for drivers. Perhaps it's not that big of a deal, and perhaps
> this will end up being needed for the work we've discussed around
> "\password" but I do want to note that this could be a breaking change.

FWIW, an extension would be required to enforce the type of hash
used, which is an extra parameter on top of the iteration number when
building the SCRAM verifier.

> Maybe out of scope for this patch based on what else is in the patch, but I
> was wondering why we don't use a "strncmp" here?

What would that change?  This needs an equal match.

    conn->in_hot_standby = PG_BOOL_UNKNOWN;
+   conn->scram_iterations = SCRAM_DEFAULT_ITERATIONS;

s/SCRAM_DEFAULT_ITERATIONS/SCRAM_SHA_256_DEFAULT_ITERATIONS/ and
s/scram_iterations/scram_sha_256_interations/ perhaps?  It does not
look like we'd have the same default across the various SHA variations
if we stick with the RFC definitions..

+#ifndef FRONTEND
+/*
+ * Number of iterations when generating new secrets.
+ */
+extern PGDLLIMPORT int scram_sha256_iterations;
+#endif

It looks like libpq/scram.h, which is backend-only, would be a better
location.

@@ -692,7 +697,7 @@ mock_scram_secret(const char *username, int *iterations, char **salt,
    encoded_salt[encoded_len] = '\0';
 
    *salt = encoded_salt;
-   *iterations = SCRAM_DEFAULT_ITERATIONS;
+   *iterations = scram_sha256_iterations;

This looks incorrect to me?  The mock authentication is here to
produce a realistic verifier, still it will fail.  It seems to me that
we'd better stick to the default in all the cases.

(FWIW, extending \password with custom options would have the
advantage to allow older server versions to use a custom iteration
number.  Perhaps that's not worth bothering about, just saying as a
separate thing to consider.)
--
Michael

---

From: "Jonathan S. Katz" <[email-masked]>
Date: Wed, 14 Dec 2022 19:00:54 -0500

> On Wed, Dec 14, 2022 at 01:59:04PM -0500, Jonathan S. Katz wrote:
HA-256 that we will just need to pick up?
> given by the user, except if I am missing your point?
My point is you can write a hook to reject the password if the iteration 
count is "too low". Not to re-hash the password.

Thanks,

Jonathan

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 15 Dec 2022 12:09:01 +0100

> On 14 Dec 2022, at 19:59, Jonathan S. Katz <[email-masked]> wrote:

> I would not rule this out. There is a RFC draft for SCRAM-SHA3-512[1].

Note that this draft is very far from RFC status, it has alredy expired twice
and hasn't been updated for a year.  The SCRAM-SHA-512 draft has an almost
identical history and neither are assigned a work group.  The author is also
drafting scram-bis which is setting up more context around these proposals,
this has yet to expire but is also very early.  The work on SCRAM-2FA seems the
most promising right now.

There might be additional versions of SCRAM published but it's looking pretty
distant now.

> Reviewing patch as is.

Thanks for review! Fixes coming downthread in an updated version.

> is fixed. A password must be re-hashed to use an updated iteration

I've rewritten to a version of this.  We don't use the terminology "SCRAM
secret" anywhere else so I used password instead.

> I don't think we should remove the RFC 7677 reference entirely.

Fixed.

> +pg_fe_scram_build_secret(const char *password, int iterations, const char **errstr)
> 
> I have mild worry about changing this function definition for downstream usage, esp. for drivers. Perhaps it's not that big of a deal, and perhaps this will end up being needed for the work we've discussed around "\password" but I do want to note that this could be a breaking change.

Not sure driver authors should be relying on this function..  Code scans
doesn't turn up any public consumers of it right now at least.  If we want to
support multiple SCRAM versions we'd still need to change it though as noted
downthread.

> Maybe out of scope for this patch based on what else is in the patch, but I was wondering why we don't use a "strncmp" here?

strncmp() would allow scram_sha256_iterations_foo to match, which we don't
want, we want an exact match.

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 15 Dec 2022 12:09:15 +0100

> On 15 Dec 2022, at 00:52, Michael Paquier <[email-masked]> wrote:

> s/scram_iterations/scram_sha_256_interations/ perhaps?  

Distinct members in the conn object is only of interest if there is a way for
the user to select a different password method in \password right?  I can
rename it now but I think doing too much here is premature, awaiting work on
\password (should that materialize) seems reasonable no?

Fixed.

> we'd better stick to the default in all the cases.

For avoiding revealing anything, I think a case can be argued for both.  I've
reverted back to the default though.

I also renamed the GUC sha_256 to match terminology we use.

---

From: Michael Paquier <[email-masked]>
Date: Sat, 17 Dec 2022 12:27:30 +0900

> \password (should that materialize) seems reasonable no?

You could do that already, somewhat indirectly, with
password_encryption, assuming that it supports more than one mode
whose password build is influenced by it.  If you wish to keep it
named this way, this is no big deal for me either way, so feel free to
use what you think is best based on the state of HEAD.  I think that
I'd value more the consistency with the backend in terms of naming,
though.

> I also renamed the GUC sha_256 to match terminology we use.

+   "SET password_encryption='scram-sha-256';
+    SET scram_sha_256_iterations=100000;
Maybe use a lower value to keep the test cheap?

+        time of encryption. In order to make use of a changed value, new
+        password must be set.
"A new password must be set".

Superuser-only GUCs should be documented as such, or do you intend to
make it user-settable like I suggested upthread :) ?
--
Michael