On Fri, Sep 20, 2024 at 12:27:41PM -0400, Tom Lane wrote:
> Nitpick: the message should say "%d bytes" not "%d characters",
> because we're counting bytes. Passes an eyeball check otherwise.
Thanks for reviewing. I went ahead and committed 0002 since it seems like
there's consensus on that one. I've attached a rebased version of 0001
with s/characters/bytes.
--
nathan
>From f039f02cc35d57862af64648d4152693e52fbee2 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Thu, 19 Sep 2024 20:59:10 -0500
Subject: [PATCH v4 1/1] place limit on password hash length
---
src/backend/libpq/crypt.c | 60 ++++++++++++++++++--------
src/include/libpq/crypt.h | 10 +++++
src/test/regress/expected/password.out | 7 +++
src/test/regress/sql/password.sql | 4 ++
4 files changed, 63 insertions(+), 18 deletions(-)
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index 629e51e00b..05d289977f 100644
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -116,7 +116,7 @@ encrypt_password(PasswordType target_type, const char *role,
const char *password)
{
PasswordType guessed_type = get_password_type(password);
- char *encrypted_password;
+ char *encrypted_password = NULL;
const char *errstr = NULL;
if (guessed_type != PASSWORD_TYPE_PLAINTEXT)
@@ -125,32 +125,56 @@ encrypt_password(PasswordType target_type, const char
*role,
* Cannot convert an already-encrypted password from one format
to
* another, so return it as it is.
*/
- return pstrdup(password);
+ encrypted_password = pstrdup(password);
}
-
- switch (target_type)
+ else
{
- case PASSWORD_TYPE_MD5:
- encrypted_password = palloc(MD5_PASSWD_LEN + 1);
+ switch (target_type)
+ {
+ case PASSWORD_TYPE_MD5:
+ encrypted_password = palloc(MD5_PASSWD_LEN + 1);
- if (!pg_md5_encrypt(password, role, strlen(role),
-
encrypted_password, &errstr))
- elog(ERROR, "password encryption failed: %s",
errstr);
- return encrypted_password;
+ if (!pg_md5_encrypt(password, role,
strlen(role),
+
encrypted_password, &errstr))
+ elog(ERROR, "password encryption
failed: %s", errstr);
+ break;
- case PASSWORD_TYPE_SCRAM_SHA_256:
- return pg_be_scram_build_secret(password);
+ case PASSWORD_TYPE_SCRAM_SHA_256:
+ encrypted_password =
pg_be_scram_build_secret(password);
+ break;
- case PASSWORD_TYPE_PLAINTEXT:
- elog(ERROR, "cannot encrypt password with 'plaintext'");
+ case PASSWORD_TYPE_PLAINTEXT:
+ elog(ERROR, "cannot encrypt password with
'plaintext'");
+ break;
+ }
}
+ Assert(encrypted_password);
+
/*
- * This shouldn't happen, because the above switch statements should
- * handle every combination of source and target password types.
+ * Valid password hashes may be very long, but we don't want to store
+ * anything that might need out-of-line storage, since de-TOASTing won't
+ * work during authentication because we haven't selected a database yet
+ * and cannot read pg_class. 256 bytes should be more than enough for
all
+ * practical use, so fail for anything longer.
*/
- elog(ERROR, "cannot encrypt password to requested type");
- return NULL; /* keep compiler quiet */
+ if (encrypted_password && /* keep compiler quiet */
+ strlen(encrypted_password) > MAX_ENCRYPTED_PASSWORD_LEN)
+ {
+ /*
+ * We don't expect any of our own hashing routines to produce
hashes
+ * that are too long.
+ */
+ Assert(guessed_type != PASSWORD_TYPE_PLAINTEXT);
+
+ ereport(ERROR,
+ (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("encrypted password is too long"),
+ errdetail("Encrypted passwords must be no
longer than %d bytes.",
+
MAX_ENCRYPTED_PASSWORD_LEN)));
+ }
+
+ return encrypted_password;
}
/*
diff --git a/src/include/libpq/crypt.h b/src/include/libpq/crypt.h
index f744de4d20..a2c99cd16a 100644
--- a/src/include/libpq/crypt.h
+++ b/src/include/libpq/crypt.h
@@ -15,6 +15,16 @@
#include "datatype/timestamp.h"
+/*
+ * Valid password hashes may be very long, but we don't want to store anything
+ * that might need out-of-line storage, since de-TOASTing won't work during
+ * authentication because we haven't selected a database yet and cannot read
+ * pg_class. 256 bytes should be more than enough for all practical use, and
+ * our own password encryption routines should never produce hashes longer than
+ * this.
+ */
+#define MAX_ENCRYPTED_PASSWORD_LEN (256)
+
/*
* Types of password hashes or secrets.
*
diff --git a/src/test/regress/expected/password.out
b/src/test/regress/expected/password.out
index 924d6e001d..dbfe92a27d 100644
--- a/src/test/regress/expected/password.out
+++ b/src/test/regress/expected/password.out
@@ -127,6 +127,13 @@ SELECT rolname, rolpassword not like
'%A6xHKoH/494E941doaPOYg==%' as is_rolpassw
regress_passwd_sha_len2 | t
(3 rows)
+-- Test that valid hashes that are too long are rejected
+CREATE ROLE regress_passwd10 PASSWORD
'SCRAM-SHA-256$00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000004096:NuDacwYSUxeOeFUEf3ivTQ==$Wgvq3OCYrJI6eUfvKlAzn4p/j3mzgWzXbVnWeFK1qhY=:r1qSP0j2QojCjLpFUjI0i6ckInvxJDKoyWnN3zF8WCM=';
+ERROR: encrypted password is too long
+DETAIL: Encrypted passwords must be no longer than 256 bytes.
+ALTER ROLE regress_passwd9 PASSWORD
'SCRAM-SHA-256$00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000004096:NuDacwYSUxeOeFUEf3ivTQ==$Wgvq3OCYrJI6eUfvKlAzn4p/j3mzgWzXbVnWeFK1qhY=:r1qSP0j2QojCjLpFUjI0i6ckInvxJDKoyWnN3zF8WCM=';
+ERROR: encrypted password is too long
+DETAIL: Encrypted passwords must be no longer than 256 bytes.
DROP ROLE regress_passwd1;
DROP ROLE regress_passwd2;
DROP ROLE regress_passwd3;
diff --git a/src/test/regress/sql/password.sql
b/src/test/regress/sql/password.sql
index bb82aa4aa2..442c903c00 100644
--- a/src/test/regress/sql/password.sql
+++ b/src/test/regress/sql/password.sql
@@ -95,6 +95,10 @@ SELECT rolname, rolpassword not like
'%A6xHKoH/494E941doaPOYg==%' as is_rolpassw
WHERE rolname LIKE 'regress_passwd_sha_len%'
ORDER BY rolname;
+-- Test that valid hashes that are too long are rejected
+CREATE ROLE regress_passwd10 PASSWORD
'SCRAM-SHA-256$00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000004096:NuDacwYSUxeOeFUEf3ivTQ==$Wgvq3OCYrJI6eUfvKlAzn4p/j3mzgWzXbVnWeFK1qhY=:r1qSP0j2QojCjLpFUjI0i6ckInvxJDKoyWnN3zF8WCM=';
+ALTER ROLE regress_passwd9 PASSWORD
'SCRAM-SHA-256$00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000004096:NuDacwYSUxeOeFUEf3ivTQ==$Wgvq3OCYrJI6eUfvKlAzn4p/j3mzgWzXbVnWeFK1qhY=:r1qSP0j2QojCjLpFUjI0i6ckInvxJDKoyWnN3zF8WCM=';
+
DROP ROLE regress_passwd1;
DROP ROLE regress_passwd2;
DROP ROLE regress_passwd3;
--
2.39.5 (Apple Git-154)
