From: Nathan Bossart <nathandbossart@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Jonathan S. Katz <jkatz@postgresql.org>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Alexander Lakhin <exclusion@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Should rolpassword be toastable?
Date: Sat, 21 Sep 2024 15:25:54 -0500
Message-ID: <Zu8r0oO0Vi0FxdpB@nathan> (raw)
In-Reply-To: <2445149.1726849661@sss.pgh.pa.us>
References: <1986859.1726756275@sss.pgh.pa.us>
<ZuxjAIOJ4GSa34KC@nathan>
<ZuydAj4nrt4cpGU5@nathan>
<2047353.1726784074@sss.pgh.pa.us>
<0a9b7f96-aa2f-41eb-8e69-62f7990ebf74@postgresql.org>
<Zuzh6Nq750L9BZn5@nathan>
<Zu0G7s-k8iECVG7i@paquier.xyz>
<0d8f3541-13f4-4194-8dca-bae881cf1a9a@postgresql.org>
<Zu2eT2H8OT3OXauc@nathan>
<2445149.1726849661@sss.pgh.pa.us>
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
Attachments:v4-0001-place-limit-on-password-hash-length.patch (text/plain, 7.0K) download
From f039f02cc35d57862af64648d4152693e52fbee2 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@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.cindex 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.hindex 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.outindex 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.sqlindex 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)
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: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, tgl@sss.pgh.pa.us, jkatz@postgresql.org, michael@paquier.xyz, exclusion@gmail.com
Subject: Re: Should rolpassword be toastable?
In-Reply-To: <Zu8r0oO0Vi0FxdpB@nathan>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox