From: Nathan Bossart <nathandbossart@gmail.com>
To: Jonathan S. Katz <jkatz@postgresql.org>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Alexander Lakhin <exclusion@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Should rolpassword be toastable?
Date: Thu, 19 Sep 2024 21:46:00 -0500
Message-ID: <Zuzh6Nq750L9BZn5@nathan> (raw)
In-Reply-To: <0a9b7f96-aa2f-41eb-8e69-62f7990ebf74@postgresql.org>
References: <89e8649c-eb74-db25-7945-6d6b23992394@gmail.com>
<1284094.1695479962@sss.pgh.pa.us>
<67793ae1-3adc-a6e0-0bb3-c23a28c0627a@gmail.com>
<a9524f94-316c-349a-6e4d-79b878706f82@gmail.com>
<Zuwzp09o9uiG0f0F@nathan>
<1986859.1726756275@sss.pgh.pa.us>
<ZuxjAIOJ4GSa34KC@nathan>
<ZuydAj4nrt4cpGU5@nathan>
<2047353.1726784074@sss.pgh.pa.us>
<0a9b7f96-aa2f-41eb-8e69-62f7990ebf74@postgresql.org>
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 <nathandbossart@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
Attachments:download all (2 files, .tar.gz)v2-0001-place-limit-on-password-hash-length.patch (text/plain, 6.6K) download
From 0493783d99195080f5ef48f8b5d749b2a3979be6 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@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.cindex 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.hindex 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.outindex 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.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)
v2-0002-remove-pg_authid-s-TOAST-table.patch (text/plain, 9.6K) download
From 535057307ac9ad47668bed4c48767e6d131391bf Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@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.cindex 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.hindex 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.outindex 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.outindex 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.outindex 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.sqlindex 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.sqlindex 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.sqlindex 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)
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, jkatz@postgresql.org, tgl@sss.pgh.pa.us, exclusion@gmail.com
Subject: Re: Should rolpassword be toastable?
In-Reply-To: <Zuzh6Nq750L9BZn5@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