public inbox for [email protected]
help / color / mirror / Atom feedFrom: Daniel Gustafsson <[email protected]>
To: Michael Paquier <[email protected]>
Cc: Jonathan S. Katz <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Raising the SCRAM iteration count
Date: Wed, 22 Feb 2023 14:39:23 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
> On 17 Dec 2022, at 04:27, Michael Paquier <[email protected]> wrote:
>
> On Thu, Dec 15, 2022 at 12:09:15PM +0100, Daniel Gustafsson wrote:
>>> On 15 Dec 2022, at 00:52, Michael Paquier <[email protected]> wrote:
>>> 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?
>>
>> 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?
>
> 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.
ok, renamed.
>>> @@ -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.
>>
>> 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.
>
> + "SET password_encryption='scram-sha-256';
> + SET scram_sha_256_iterations=100000;
> Maybe use a lower value to keep the test cheap?
Fixed.
> + time of encryption. In order to make use of a changed value, new
> + password must be set.
> "A new password must be set".
Fixed.
> Superuser-only GUCs should be documented as such, or do you intend to
> make it user-settable like I suggested upthread :) ?
I don't really have strong feelings, so I reverted to being user-settable since
I can't really present a strong argument for superuser-only.
The attached is a rebase on top of master with no other additional hacking done
on top of the above review comments.
--
Daniel Gustafsson
Attachments:
[application/octet-stream] v4-0001-Make-SCRAM-iteration-count-configurable.patch (14.8K, ../[email protected]/2-v4-0001-Make-SCRAM-iteration-count-configurable.patch)
download | inline diff:
From d6afaca4da3cd60f837e27c2f371aceeb196c6e2 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <[email protected]>
Date: Wed, 22 Feb 2023 14:36:36 +0100
Subject: [PATCH v4] Make SCRAM iteration count configurable
Replace the hardcoded value with a GUC such that the iteration count
can be raised in order to increase protection against brute-force
attacks. The hardcoded value for SCRAM iteration count was defined
to be 4096, which is taken from RFC 7677, so set the default for the
GUC to 4096 to match. Raising the iteration count will make stored
passwords more resilient to brute-force attacks at the cost of more
computation performed during connection establishment. Lowering the
count will reduce computational overhead during connections at the
tradeoff of reducing strength against brute-force attacks. When the
alternative is to fall back to md5 instead, SCRAM with a very low
iteration count is still preferrable so allow iteration counts all
the way down to one.
Reviewed-by: Michael Paquier <[email protected]>
Reviewed-by: Jonathan S. Katz <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
doc/src/sgml/config.sgml | 20 +++++++++++++++++++
src/backend/libpq/auth-scram.c | 9 +++++++--
src/backend/utils/misc/guc_tables.c | 13 ++++++++++++
src/backend/utils/misc/postgresql.conf.sample | 1 +
src/common/scram-common.c | 3 +--
src/include/common/scram-common.h | 2 +-
src/include/libpq/scram.h | 3 +++
src/interfaces/libpq/fe-auth-scram.c | 4 ++--
src/interfaces/libpq/fe-auth.c | 4 +++-
src/interfaces/libpq/fe-auth.h | 1 +
src/interfaces/libpq/fe-connect.c | 2 ++
src/interfaces/libpq/fe-exec.c | 4 ++++
src/interfaces/libpq/libpq-int.h | 1 +
src/test/authentication/t/001_password.pl | 16 +++++++++++++++
src/test/regress/expected/password.out | 7 ++++++-
src/test/regress/sql/password.sql | 5 +++++
16 files changed, 86 insertions(+), 9 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index e5c41cc6c6..a0eecaaaf0 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1132,6 +1132,26 @@ include_dir 'conf.d'
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><varname>scram_sha_256_iterations</varname> (<type>integer</type>)
+ <indexterm>
+ <primary><varname>scram_sha_256_iterations</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ The number of computational iterations to be performed when encrypting
+ a password using SCRAM-SHA-256. 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 existing passwords
+ encrypted with SCRAM-SHA-256 as the iteration count is fixed at the
+ time of encryption. In order to make use of a changed value, a new
+ password must be set.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-krb-server-keyfile" xreflabel="krb_server_keyfile">
<term><varname>krb_server_keyfile</varname> (<type>string</type>)
<indexterm>
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 4441e0d774..75583dcbf0 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -191,6 +191,11 @@ static char *scram_mock_salt(const char *username,
pg_cryptohash_type hash_type,
int key_length);
+/*
+ * The number of iterations to use when generating new secrets.
+ */
+int scram_sha_256_iterations;
+
/*
* Get a list of SASL mechanisms that this module supports.
*
@@ -496,7 +501,7 @@ pg_be_scram_build_secret(const char *password)
result = scram_build_secret(PG_SHA256, SCRAM_SHA_256_KEY_LEN,
saltbuf, SCRAM_DEFAULT_SALT_LEN,
- SCRAM_DEFAULT_ITERATIONS, password,
+ scram_sha_256_iterations, password,
&errstr);
if (prep_password)
@@ -717,7 +722,7 @@ mock_scram_secret(const char *username, pg_cryptohash_type *hash_type,
encoded_salt[encoded_len] = '\0';
*salt = encoded_salt;
- *iterations = SCRAM_DEFAULT_ITERATIONS;
+ *iterations = SCRAM_SHA_256_DEFAULT_ITERATIONS;
/* StoredKey and ServerKey are not used in a doomed authentication */
memset(stored_key, 0, SCRAM_MAX_KEY_LEN);
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 1c0583fe26..554f706499 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -41,9 +41,11 @@
#include "commands/trigger.h"
#include "commands/user.h"
#include "commands/vacuum.h"
+#include "common/scram-common.h"
#include "jit/jit.h"
#include "libpq/auth.h"
#include "libpq/libpq.h"
+#include "libpq/scram.h"
#include "nodes/queryjumble.h"
#include "optimizer/cost.h"
#include "optimizer/geqo.h"
@@ -3468,6 +3470,17 @@ struct config_int ConfigureNamesInt[] =
NULL, NULL, NULL
},
+ {
+ {"scram_sha_256_iterations", PGC_USERSET, CONN_AUTH_AUTH,
+ gettext_noop("Sets the iteration count for SCRAM secret generation."),
+ NULL,
+ GUC_REPORT
+ },
+ &scram_sha_256_iterations,
+ SCRAM_SHA_256_DEFAULT_ITERATIONS, 1, INT_MAX,
+ NULL, NULL, NULL
+ },
+
/* End-of-list marker */
{
{NULL, 0, 0, NULL, NULL}, NULL, 0, 0, 0, NULL, NULL, NULL
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index d06074b86f..9e901fc112 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -95,6 +95,7 @@
#authentication_timeout = 1min # 1s-600s
#password_encryption = scram-sha-256 # scram-sha-256 or md5
+#scram_sha_256_iterations = 4096
#db_user_namespace = off
# GSSAPI using Kerberos
diff --git a/src/common/scram-common.c b/src/common/scram-common.c
index bd40d497a9..ef997ef684 100644
--- a/src/common/scram-common.c
+++ b/src/common/scram-common.c
@@ -214,8 +214,7 @@ scram_build_secret(pg_cryptohash_type hash_type, int key_length,
/* Only this hash method is supported currently */
Assert(hash_type == PG_SHA256);
- if (iterations <= 0)
- iterations = SCRAM_DEFAULT_ITERATIONS;
+ Assert(iterations > 0);
/* Calculate StoredKey and ServerKey */
if (scram_SaltedPassword(password, hash_type, key_length,
diff --git a/src/include/common/scram-common.h b/src/include/common/scram-common.h
index 0c75df5559..5ccff96ece 100644
--- a/src/include/common/scram-common.h
+++ b/src/include/common/scram-common.h
@@ -47,7 +47,7 @@
* Default number of iterations when generating secret. Should be at least
* 4096 per RFC 7677.
*/
-#define SCRAM_DEFAULT_ITERATIONS 4096
+#define SCRAM_SHA_256_DEFAULT_ITERATIONS 4096
extern int scram_SaltedPassword(const char *password,
pg_cryptohash_type hash_type, int key_length,
diff --git a/src/include/libpq/scram.h b/src/include/libpq/scram.h
index b275e1e87e..310bc36517 100644
--- a/src/include/libpq/scram.h
+++ b/src/include/libpq/scram.h
@@ -18,6 +18,9 @@
#include "libpq/libpq-be.h"
#include "libpq/sasl.h"
+/* Number of iterations when generating new secrets */
+extern PGDLLIMPORT int scram_sha_256_iterations;
+
/* SASL implementation callbacks */
extern PGDLLIMPORT const pg_be_sasl_mech pg_be_scram_mech;
diff --git a/src/interfaces/libpq/fe-auth-scram.c b/src/interfaces/libpq/fe-auth-scram.c
index 9c42ea4f81..7162fdbb85 100644
--- a/src/interfaces/libpq/fe-auth-scram.c
+++ b/src/interfaces/libpq/fe-auth-scram.c
@@ -894,7 +894,7 @@ verify_server_signature(fe_scram_state *state, bool *match,
* error details.
*/
char *
-pg_fe_scram_build_secret(const char *password, const char **errstr)
+pg_fe_scram_build_secret(const char *password, int iterations, const char **errstr)
{
char *prep_password;
pg_saslprep_rc rc;
@@ -926,7 +926,7 @@ pg_fe_scram_build_secret(const char *password, const char **errstr)
result = scram_build_secret(PG_SHA256, SCRAM_SHA_256_KEY_LEN, saltbuf,
SCRAM_DEFAULT_SALT_LEN,
- SCRAM_DEFAULT_ITERATIONS, password,
+ iterations, password,
errstr);
free(prep_password);
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index 9afc6f19b9..0935356075 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -1253,7 +1253,9 @@ PQencryptPasswordConn(PGconn *conn, const char *passwd, const char *user,
{
const char *errstr = NULL;
- crypt_pwd = pg_fe_scram_build_secret(passwd, &errstr);
+ crypt_pwd = pg_fe_scram_build_secret(passwd,
+ conn->scram_sha_256_iterations,
+ &errstr);
if (!crypt_pwd)
libpq_append_conn_error(conn, "could not encrypt password: %s", errstr);
}
diff --git a/src/interfaces/libpq/fe-auth.h b/src/interfaces/libpq/fe-auth.h
index 1aa556ea2f..124dd5d031 100644
--- a/src/interfaces/libpq/fe-auth.h
+++ b/src/interfaces/libpq/fe-auth.h
@@ -26,6 +26,7 @@ extern char *pg_fe_getauthname(PQExpBuffer errorMessage);
/* Mechanisms in fe-auth-scram.c */
extern const pg_fe_sasl_mech pg_scram_mech;
extern char *pg_fe_scram_build_secret(const char *password,
+ int iterations,
const char **errstr);
#endif /* FE_AUTH_H */
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 50b5df3490..3bf9b08e36 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -586,6 +586,7 @@ pqDropServerData(PGconn *conn)
conn->std_strings = false;
conn->default_transaction_read_only = PG_BOOL_UNKNOWN;
conn->in_hot_standby = PG_BOOL_UNKNOWN;
+ conn->scram_sha_256_iterations = SCRAM_SHA_256_DEFAULT_ITERATIONS;
conn->sversion = 0;
/* Drop large-object lookup data */
@@ -3909,6 +3910,7 @@ makeEmptyPGconn(void)
conn->std_strings = false; /* unless server says differently */
conn->default_transaction_read_only = PG_BOOL_UNKNOWN;
conn->in_hot_standby = PG_BOOL_UNKNOWN;
+ conn->scram_sha_256_iterations = SCRAM_SHA_256_DEFAULT_ITERATIONS;
conn->verbosity = PQERRORS_DEFAULT;
conn->show_context = PQSHOW_CONTEXT_ERRORS;
conn->sock = PGINVALID_SOCKET;
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index ec62550e38..8607bc062e 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -1181,6 +1181,10 @@ pqSaveParameterStatus(PGconn *conn, const char *name, const char *value)
conn->in_hot_standby =
(strcmp(value, "on") == 0) ? PG_BOOL_YES : PG_BOOL_NO;
}
+ else if (strcmp(name, "scram_sha_256_iterations") == 0)
+ {
+ conn->scram_sha_256_iterations = atoi(value);
+ }
}
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index d7ec5ed429..940d1a35a1 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -515,6 +515,7 @@ struct pg_conn
/* Assorted state for SASL, SSL, GSS, etc */
const pg_fe_sasl_mech *sasl;
void *sasl_state;
+ int scram_sha_256_iterations;
/* SSL structures */
bool ssl_in_use;
diff --git a/src/test/authentication/t/001_password.pl b/src/test/authentication/t/001_password.pl
index a2fde1408b..e0ab39ef52 100644
--- a/src/test/authentication/t/001_password.pl
+++ b/src/test/authentication/t/001_password.pl
@@ -86,6 +86,14 @@ $node->safe_psql('postgres',
q{SET password_encryption='md5'; CREATE ROLE "md5,role" LOGIN PASSWORD 'pass';}
);
+# Create a role with a non-default iteration count
+$node->safe_psql(
+ 'postgres',
+ "SET password_encryption='scram-sha-256';
+ SET scram_sha_256_iterations=10000;
+ CREATE ROLE scram_role_iter LOGIN PASSWORD 'pass';"
+);
+
# Create a database to test regular expression.
$node->safe_psql('postgres', "CREATE database regex_testdb;");
@@ -134,6 +142,14 @@ test_conn(
log_like => [
qr/connection authenticated: identity="scram_role" method=scram-sha-256/
]);
+test_conn(
+ $node,
+ 'user=scram_role_iter',
+ 'scram-sha-256',
+ 0,
+ log_like => [
+ qr/connection authenticated: identity="scram_role_iter" method=scram-sha-256/
+ ]);
test_conn($node, 'user=md5_role', 'scram-sha-256', 2,
log_unlike => [qr/connection authenticated:/]);
diff --git a/src/test/regress/expected/password.out b/src/test/regress/expected/password.out
index 7c84c9da33..53db07d1d5 100644
--- a/src/test/regress/expected/password.out
+++ b/src/test/regress/expected/password.out
@@ -72,6 +72,9 @@ CREATE ROLE regress_passwd6 PASSWORD 'SCRAM-SHA-256$1234';
CREATE ROLE regress_passwd7 PASSWORD 'md5012345678901234567890123456789zz';
-- invalid length
CREATE ROLE regress_passwd8 PASSWORD 'md501234567890123456789012345678901zz';
+-- Increasing the SCRAM iteration count
+SET scram_sha_256_iterations = 9999;
+CREATE ROLE regress_passwd9 PASSWORD 'raisediterationcount';
SELECT rolname, regexp_replace(rolpassword, '(SCRAM-SHA-256)\$(\d+):([a-zA-Z0-9+/=]+)\$([a-zA-Z0-9+=/]+):([a-zA-Z0-9+/=]+)', '\1$\2:<salt>$<storedkey>:<serverkey>') as rolpassword_masked
FROM pg_authid
WHERE rolname LIKE 'regress_passwd%'
@@ -86,7 +89,8 @@ SELECT rolname, regexp_replace(rolpassword, '(SCRAM-SHA-256)\$(\d+):([a-zA-Z0-9+
regress_passwd6 | SCRAM-SHA-256$4096:<salt>$<storedkey>:<serverkey>
regress_passwd7 | SCRAM-SHA-256$4096:<salt>$<storedkey>:<serverkey>
regress_passwd8 | SCRAM-SHA-256$4096:<salt>$<storedkey>:<serverkey>
-(8 rows)
+ regress_passwd9 | SCRAM-SHA-256$9999:<salt>$<storedkey>:<serverkey>
+(9 rows)
-- An empty password is not allowed, in any form
CREATE ROLE regress_passwd_empty PASSWORD '';
@@ -129,6 +133,7 @@ DROP ROLE regress_passwd5;
DROP ROLE regress_passwd6;
DROP ROLE regress_passwd7;
DROP ROLE regress_passwd8;
+DROP ROLE regress_passwd9;
DROP ROLE regress_passwd_empty;
DROP ROLE regress_passwd_sha_len0;
DROP ROLE regress_passwd_sha_len1;
diff --git a/src/test/regress/sql/password.sql b/src/test/regress/sql/password.sql
index 98f49916e5..59ef7de21e 100644
--- a/src/test/regress/sql/password.sql
+++ b/src/test/regress/sql/password.sql
@@ -63,6 +63,10 @@ CREATE ROLE regress_passwd7 PASSWORD 'md5012345678901234567890123456789zz';
-- invalid length
CREATE ROLE regress_passwd8 PASSWORD 'md501234567890123456789012345678901zz';
+-- Increasing the SCRAM iteration count
+SET scram_sha_256_iterations = 9999;
+CREATE ROLE regress_passwd9 PASSWORD 'raisediterationcount';
+
SELECT rolname, regexp_replace(rolpassword, '(SCRAM-SHA-256)\$(\d+):([a-zA-Z0-9+/=]+)\$([a-zA-Z0-9+=/]+):([a-zA-Z0-9+/=]+)', '\1$\2:<salt>$<storedkey>:<serverkey>') as rolpassword_masked
FROM pg_authid
WHERE rolname LIKE 'regress_passwd%'
@@ -97,6 +101,7 @@ DROP ROLE regress_passwd5;
DROP ROLE regress_passwd6;
DROP ROLE regress_passwd7;
DROP ROLE regress_passwd8;
+DROP ROLE regress_passwd9;
DROP ROLE regress_passwd_empty;
DROP ROLE regress_passwd_sha_len0;
DROP ROLE regress_passwd_sha_len1;
--
2.32.1 (Apple Git-133)
view thread (2+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Raising the SCRAM iteration count
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox