Date: Sat, 21 Sep 2024 15:25:54 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: "Jonathan S. Katz" <jkatz@postgresql.org>,
	Michael Paquier <michael@paquier.xyz>,
	Alexander Lakhin <exclusion@gmail.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Should rolpassword be toastable?
Message-ID: <Zu8r0oO0Vi0FxdpB@nathan>
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>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="2R5fTxH63iv17WWy"
Content-Disposition: inline
In-Reply-To: <2445149.1726849661@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/Zu8r0oO0Vi0FxdpB%40nathan>
Precedence: bulk


--2R5fTxH63iv17WWy
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--2R5fTxH63iv17WWy
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename="v4-0001-place-limit-on-password-hash-length.patch"

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.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)


--2R5fTxH63iv17WWy--