On Thu, Sep 19, 2024 at 07:37:55PM -0400, Jonathan S. Katz wrote:
> On 9/19/24 6:14 PM, Tom Lane wrote:
>> Nathan Bossart <nathandboss...@gmail.com> writes:
>> > Oh, actually, I see that we are already validating the hash, but you can
>> > create valid SCRAM-SHA-256 hashes that are really long.
>
> You _can_, but it's up to a driver or a very determined user to do this, as
> it involves creating a very long salt.
I can't think of any reason to support this, unless we want Alexander to
find more bugs.
>> So putting an
>> > arbitrary limit (patch attached) is probably the correct path forward. I'd
>> > also remove pg_authid's TOAST table while at it.
>>
>> Shouldn't we enforce the limit in every case in encrypt_password,
>> not just this one? (I do agree that encrypt_password is an okay
>> place to enforce it.)
Yeah, that seems like a good idea. I've attached a more fleshed-out patch
set that applies the limit in all cases.
> +1; if there's any breakage, my guess is it would be on very long plaintext
> passwords, but that would be from a very old upgrade?
IIUC there's zero support for plain-text passwords in newer versions, and
any that remain in older clusters will be silently converted to a hash by
pg_upgrade.
>> I think you will get pushback from a limit of 256 bytes --- I seem
>> to recall discussion of actual use-cases where people were using
>> strings of a couple of kB. Whatever the limit is, the error message
>> had better cite it explicitly.
>
> I think it's OK to be a bit generous with the limit. Also, currently oru
> hashes are 256-bit (I know the above says byte), but this could increase
> should we support larger hashes.
Hm. Are you thinking of commit 67a472d? That one removed the password
length restrictions in client-side code and password message packets, which
I think is entirely separate from the lengths of the hashes stored in
rolpassword.
>> Also, the ereport call needs an errcode.
>> ERRCODE_PROGRAM_LIMIT_EXCEEDED is probably suitable.
This is added in v2.
--
nathan
>From 0493783d99195080f5ef48f8b5d749b2a3979be6 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Thu, 19 Sep 2024 20:59:10 -0500
Subject: [PATCH v2 1/2] place limit on password hash length
---
src/backend/libpq/crypt.c | 56 ++++++++++++++++++--------
src/include/libpq/crypt.h | 9 +++++
src/test/regress/expected/password.out | 7 ++++
src/test/regress/sql/password.sql | 4 ++
4 files changed, 59 insertions(+), 17 deletions(-)
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index 629e51e00b..2cdc977bea 100644
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -125,32 +125,54 @@ 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'");
+ encrypted_password = palloc(0); /* keep
compiler quiet */
+ break;
+ }
}
/*
- * 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 get TOASTed, 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 (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 characters.",
+
MAX_ENCRYPTED_PASSWORD_LEN)));
+ }
+
+ return encrypted_password;
}
/*
diff --git a/src/include/libpq/crypt.h b/src/include/libpq/crypt.h
index f744de4d20..7a1099b917 100644
--- a/src/include/libpq/crypt.h
+++ b/src/include/libpq/crypt.h
@@ -15,6 +15,15 @@
#include "datatype/timestamp.h"
+/*
+ * Valid password hashes may be very long, but we don't want to store anything
+ * that might get TOASTed, 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..8ab995a162 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 characters.
+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 characters.
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)
>From 535057307ac9ad47668bed4c48767e6d131391bf Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Thu, 19 Sep 2024 21:09:30 -0500
Subject: [PATCH v2 2/2] remove pg_authid's TOAST table
XXX: NEEDS CATVERSION BUMP
---
src/backend/catalog/catalog.c | 4 +---
src/include/catalog/pg_authid.h | 2 --
src/test/regress/expected/create_index.out | 12 ++++++------
src/test/regress/expected/misc_sanity.out | 5 ++++-
src/test/regress/expected/tablespace.out | 12 ++++++------
src/test/regress/sql/create_index.sql | 8 ++++----
src/test/regress/sql/misc_sanity.sql | 2 ++
src/test/regress/sql/tablespace.sql | 8 ++++----
8 files changed, 27 insertions(+), 26 deletions(-)
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 6c39434a30..d6b07a7865 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -300,9 +300,7 @@ IsSharedRelation(Oid relationId)
relationId == TablespaceOidIndexId)
return true;
/* These are their toast tables and toast indexes */
- if (relationId == PgAuthidToastTable ||
- relationId == PgAuthidToastIndex ||
- relationId == PgDatabaseToastTable ||
+ if (relationId == PgDatabaseToastTable ||
relationId == PgDatabaseToastIndex ||
relationId == PgDbRoleSettingToastTable ||
relationId == PgDbRoleSettingToastIndex ||
diff --git a/src/include/catalog/pg_authid.h b/src/include/catalog/pg_authid.h
index e08863f78a..e846d75731 100644
--- a/src/include/catalog/pg_authid.h
+++ b/src/include/catalog/pg_authid.h
@@ -55,8 +55,6 @@ CATALOG(pg_authid,1260,AuthIdRelationId) BKI_SHARED_RELATION
BKI_ROWTYPE_OID(284
*/
typedef FormData_pg_authid *Form_pg_authid;
-DECLARE_TOAST_WITH_MACRO(pg_authid, 4175, 4176, PgAuthidToastTable,
PgAuthidToastIndex);
-
DECLARE_UNIQUE_INDEX(pg_authid_rolname_index, 2676, AuthIdRolnameIndexId,
pg_authid, btree(rolname name_ops));
DECLARE_UNIQUE_INDEX_PKEY(pg_authid_oid_index, 2677, AuthIdOidIndexId,
pg_authid, btree(oid oid_ops));
diff --git a/src/test/regress/expected/create_index.out
b/src/test/regress/expected/create_index.out
index cf6eac5734..22c8787e80 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2665,9 +2665,9 @@ ERROR: cannot reindex system catalogs concurrently
REINDEX INDEX CONCURRENTLY pg_class_oid_index; -- no catalog index
ERROR: cannot reindex system catalogs concurrently
-- These are the toast table and index of pg_authid.
-REINDEX TABLE CONCURRENTLY pg_toast.pg_toast_1260; -- no catalog toast table
+REINDEX TABLE CONCURRENTLY pg_toast.pg_toast_1262; -- no catalog toast table
ERROR: cannot reindex system catalogs concurrently
-REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_1260_index; -- no catalog toast
index
+REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_1262_index; -- no catalog toast
index
ERROR: cannot reindex system catalogs concurrently
REINDEX SYSTEM CONCURRENTLY postgres; -- not allowed for SYSTEM
ERROR: cannot reindex system catalogs concurrently
@@ -2974,10 +2974,10 @@ ERROR: must be owner of schema schema_to_reindex
RESET ROLE;
GRANT USAGE ON SCHEMA pg_toast TO regress_reindexuser;
SET SESSION ROLE regress_reindexuser;
-REINDEX TABLE pg_toast.pg_toast_1260;
-ERROR: permission denied for table pg_toast_1260
-REINDEX INDEX pg_toast.pg_toast_1260_index;
-ERROR: permission denied for index pg_toast_1260_index
+REINDEX TABLE pg_toast.pg_toast_1262;
+ERROR: permission denied for table pg_toast_1262
+REINDEX INDEX pg_toast.pg_toast_1262_index;
+ERROR: permission denied for index pg_toast_1262_index
-- Clean up
RESET ROLE;
REVOKE USAGE ON SCHEMA pg_toast FROM regress_reindexuser;
diff --git a/src/test/regress/expected/misc_sanity.out
b/src/test/regress/expected/misc_sanity.out
index 45e7492877..b032a3f476 100644
--- a/src/test/regress/expected/misc_sanity.out
+++ b/src/test/regress/expected/misc_sanity.out
@@ -40,6 +40,8 @@ WHERE refclassid = 0 OR refobjid = 0 OR
-- 2. pg_largeobject and pg_largeobject_metadata. Large object catalogs
-- and toast tables are mutually exclusive and large object data is handled
-- as user data by pg_upgrade, which would cause failures.
+-- 3. pg_authid, since its toast table cannot be accessed when it would be
+-- needed, i.e., during authentication before we've selected a database.
SELECT relname, attname, atttypid::regtype
FROM pg_class c JOIN pg_attribute a ON c.oid = attrelid
WHERE c.oid < 16384 AND
@@ -53,12 +55,13 @@ ORDER BY 1, 2;
pg_attribute | attfdwoptions | text[]
pg_attribute | attmissingval | anyarray
pg_attribute | attoptions | text[]
+ pg_authid | rolpassword | text
pg_class | relacl | aclitem[]
pg_class | reloptions | text[]
pg_class | relpartbound | pg_node_tree
pg_largeobject | data | bytea
pg_largeobject_metadata | lomacl | aclitem[]
-(9 rows)
+(10 rows)
-- system catalogs without primary keys
--
diff --git a/src/test/regress/expected/tablespace.out
b/src/test/regress/expected/tablespace.out
index 9aabb85349..dd535d41a3 100644
--- a/src/test/regress/expected/tablespace.out
+++ b/src/test/regress/expected/tablespace.out
@@ -51,13 +51,13 @@ ERROR: cannot move system relation
"pg_authid_rolname_index"
REINDEX (TABLESPACE regress_tblspace) TABLE CONCURRENTLY pg_authid;
ERROR: cannot reindex system catalogs concurrently
-- toast relations, fail
-REINDEX (TABLESPACE regress_tblspace) INDEX pg_toast.pg_toast_1260_index;
-ERROR: cannot move system relation "pg_toast_1260_index"
-REINDEX (TABLESPACE regress_tblspace) INDEX CONCURRENTLY
pg_toast.pg_toast_1260_index;
+REINDEX (TABLESPACE regress_tblspace) INDEX pg_toast.pg_toast_1262_index;
+ERROR: cannot move system relation "pg_toast_1262_index"
+REINDEX (TABLESPACE regress_tblspace) INDEX CONCURRENTLY
pg_toast.pg_toast_1262_index;
ERROR: cannot reindex system catalogs concurrently
-REINDEX (TABLESPACE regress_tblspace) TABLE pg_toast.pg_toast_1260;
-ERROR: cannot move system relation "pg_toast_1260_index"
-REINDEX (TABLESPACE regress_tblspace) TABLE CONCURRENTLY
pg_toast.pg_toast_1260;
+REINDEX (TABLESPACE regress_tblspace) TABLE pg_toast.pg_toast_1262;
+ERROR: cannot move system relation "pg_toast_1262_index"
+REINDEX (TABLESPACE regress_tblspace) TABLE CONCURRENTLY
pg_toast.pg_toast_1262;
ERROR: cannot reindex system catalogs concurrently
-- system catalog, fail
REINDEX (TABLESPACE pg_global) TABLE pg_authid;
diff --git a/src/test/regress/sql/create_index.sql
b/src/test/regress/sql/create_index.sql
index e296891cab..145860b6ec 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -1127,8 +1127,8 @@ COMMIT;
REINDEX TABLE CONCURRENTLY pg_class; -- no catalog relation
REINDEX INDEX CONCURRENTLY pg_class_oid_index; -- no catalog index
-- These are the toast table and index of pg_authid.
-REINDEX TABLE CONCURRENTLY pg_toast.pg_toast_1260; -- no catalog toast table
-REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_1260_index; -- no catalog toast
index
+REINDEX TABLE CONCURRENTLY pg_toast.pg_toast_1262; -- no catalog toast table
+REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_1262_index; -- no catalog toast
index
REINDEX SYSTEM CONCURRENTLY postgres; -- not allowed for SYSTEM
REINDEX (CONCURRENTLY) SYSTEM postgres; -- ditto
REINDEX (CONCURRENTLY) SYSTEM; -- ditto
@@ -1305,8 +1305,8 @@ REINDEX SCHEMA schema_to_reindex;
RESET ROLE;
GRANT USAGE ON SCHEMA pg_toast TO regress_reindexuser;
SET SESSION ROLE regress_reindexuser;
-REINDEX TABLE pg_toast.pg_toast_1260;
-REINDEX INDEX pg_toast.pg_toast_1260_index;
+REINDEX TABLE pg_toast.pg_toast_1262;
+REINDEX INDEX pg_toast.pg_toast_1262_index;
-- Clean up
RESET ROLE;
diff --git a/src/test/regress/sql/misc_sanity.sql
b/src/test/regress/sql/misc_sanity.sql
index 16f3a7c0c1..135793871b 100644
--- a/src/test/regress/sql/misc_sanity.sql
+++ b/src/test/regress/sql/misc_sanity.sql
@@ -43,6 +43,8 @@ WHERE refclassid = 0 OR refobjid = 0 OR
-- 2. pg_largeobject and pg_largeobject_metadata. Large object catalogs
-- and toast tables are mutually exclusive and large object data is handled
-- as user data by pg_upgrade, which would cause failures.
+-- 3. pg_authid, since its toast table cannot be accessed when it would be
+-- needed, i.e., during authentication before we've selected a database.
SELECT relname, attname, atttypid::regtype
FROM pg_class c JOIN pg_attribute a ON c.oid = attrelid
diff --git a/src/test/regress/sql/tablespace.sql
b/src/test/regress/sql/tablespace.sql
index d274d9615e..c8b83788f0 100644
--- a/src/test/regress/sql/tablespace.sql
+++ b/src/test/regress/sql/tablespace.sql
@@ -40,10 +40,10 @@ REINDEX (TABLESPACE regress_tblspace) TABLE CONCURRENTLY
pg_am;
REINDEX (TABLESPACE regress_tblspace) TABLE pg_authid;
REINDEX (TABLESPACE regress_tblspace) TABLE CONCURRENTLY pg_authid;
-- toast relations, fail
-REINDEX (TABLESPACE regress_tblspace) INDEX pg_toast.pg_toast_1260_index;
-REINDEX (TABLESPACE regress_tblspace) INDEX CONCURRENTLY
pg_toast.pg_toast_1260_index;
-REINDEX (TABLESPACE regress_tblspace) TABLE pg_toast.pg_toast_1260;
-REINDEX (TABLESPACE regress_tblspace) TABLE CONCURRENTLY
pg_toast.pg_toast_1260;
+REINDEX (TABLESPACE regress_tblspace) INDEX pg_toast.pg_toast_1262_index;
+REINDEX (TABLESPACE regress_tblspace) INDEX CONCURRENTLY
pg_toast.pg_toast_1262_index;
+REINDEX (TABLESPACE regress_tblspace) TABLE pg_toast.pg_toast_1262;
+REINDEX (TABLESPACE regress_tblspace) TABLE CONCURRENTLY
pg_toast.pg_toast_1262;
-- system catalog, fail
REINDEX (TABLESPACE pg_global) TABLE pg_authid;
REINDEX (TABLESPACE pg_global) TABLE CONCURRENTLY pg_authid;
--
2.39.5 (Apple Git-154)
