From 53a936b61f12cd9220340abc1a7843eb003d1abf Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 11 Jul 2025 18:50:13 -0400 Subject: [PATCH] Fix inconsistent quoting of role names in ACLs. getid() and putid(), which parse and deparse role names within ACL input/output, applied isalnum() to see if a character within a role name requires quoting. They did this even for non-ASCII characters, which is problematic because the results would depend on encoding, locale, and perhaps even platform. So it's possible that putid() could elect not to quote some string that, later in some other environment, getid() will decide is not a valid identifier, causing dump/reload or similar failures. To fix this in a way that won't risk interoperability problems with unpatched versions, make getid() treat any non-ASCII as a legitimate identifier character (hence not requiring quotes), while making putid() treat any non-ASCII as requiring quoting. We could remove the resulting excess quoting once we feel that no unpatched servers remain in the wild, but that'll be years. A lesser problem is that getid() did the wrong thing with an input consisting of just two double quotes (""). That has to represent an empty string, but getid() read it as a single double quote instead. The case cannot arise in the normal course of events, since we don't allow empty-string role names. But let's fix it while we're here. Although we've not heard field reports of problems with non-ASCII role names, there's clearly a hazard there, so back-patch to all supported versions. Reported-by: Peter Eisentraut Author: Tom Lane Discussion: https://postgr.es/m/3792884.1751492172@sss.pgh.pa.us Backpatch-through: 13 --- src/backend/utils/adt/acl.c | 33 ++++++++++++++++++------ src/test/regress/expected/privileges.out | 20 ++++++++++++++ src/test/regress/sql/privileges.sql | 8 ++++++ 3 files changed, 53 insertions(+), 8 deletions(-) diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c index 883e09393a4..d2278374d97 100644 --- a/src/backend/utils/adt/acl.c +++ b/src/backend/utils/adt/acl.c @@ -124,6 +124,22 @@ static AclResult pg_role_aclcheck(Oid role_oid, Oid roleid, AclMode mode); static void RoleMembershipCacheCallback(Datum arg, int cacheid, uint32 hashvalue); +/* + * Test whether an identifier char can be left unquoted in ACLs. + * + * Formerly, we used isalnum() even on non-ASCII characters, resulting in + * unportable behavior. To ensure dump compatibility with old versions, + * we now treat high-bit-set characters as always requiring quoting during + * putid(), but getid() will always accept them without quotes. + */ +static inline bool +is_safe_acl_char(unsigned char c, bool is_getid) +{ + if (IS_HIGHBIT_SET(c)) + return is_getid; + return isalnum(c) || c == '_'; +} + /* * getid * Consumes the first alphanumeric string (identifier) found in string @@ -149,21 +165,22 @@ getid(const char *s, char *n, Node *escontext) while (isspace((unsigned char) *s)) s++; - /* This code had better match what putid() does, below */ for (; *s != '\0' && - (isalnum((unsigned char) *s) || - *s == '_' || - *s == '"' || - in_quotes); + (in_quotes || *s == '"' || is_safe_acl_char(*s, true)); s++) { if (*s == '"') { + if (!in_quotes) + { + in_quotes = true; + continue; + } /* safe to look at next char (could be '\0' though) */ if (*(s + 1) != '"') { - in_quotes = !in_quotes; + in_quotes = false; continue; } /* it's an escaped double quote; skip the escaping char */ @@ -197,10 +214,10 @@ putid(char *p, const char *s) const char *src; bool safe = true; + /* Detect whether we need to use double quotes */ for (src = s; *src; src++) { - /* This test had better match what getid() does, above */ - if (!isalnum((unsigned char) *src) && *src != '_') + if (!is_safe_acl_char(*src, false)) { safe = false; break; diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out index 620fbe8c522..99daf089e30 100644 --- a/src/test/regress/expected/privileges.out +++ b/src/test/regress/expected/privileges.out @@ -2332,6 +2332,26 @@ SELECT makeaclitem('regress_priv_user1'::regrole, 'regress_priv_user2'::regrole, SELECT makeaclitem('regress_priv_user1'::regrole, 'regress_priv_user2'::regrole, 'SELECT, fake_privilege', FALSE); -- error ERROR: unrecognized privilege type: "fake_privilege" +-- Test quoting and dequoting of user names in ACLs +CREATE ROLE "regress_""quoted"; +SELECT makeaclitem('regress_"quoted'::regrole, 'regress_"quoted'::regrole, + 'SELECT', TRUE); + makeaclitem +------------------------------------------ + "regress_""quoted"=r*/"regress_""quoted" +(1 row) + +SELECT '"regress_""quoted"=r*/"regress_""quoted"'::aclitem; + aclitem +------------------------------------------ + "regress_""quoted"=r*/"regress_""quoted" +(1 row) + +SELECT '""=r*/""'::aclitem; -- used to be misparsed as """" +ERROR: a name must follow the "/" sign +LINE 1: SELECT '""=r*/""'::aclitem; + ^ +DROP ROLE "regress_""quoted"; -- Test non-throwing aclitem I/O SELECT pg_input_is_valid('regress_priv_user1=r/regress_priv_user2', 'aclitem'); pg_input_is_valid diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql index 259f1aedd1b..d84b4041e8b 100644 --- a/src/test/regress/sql/privileges.sql +++ b/src/test/regress/sql/privileges.sql @@ -1481,6 +1481,14 @@ SELECT makeaclitem('regress_priv_user1'::regrole, 'regress_priv_user2'::regrole, SELECT makeaclitem('regress_priv_user1'::regrole, 'regress_priv_user2'::regrole, 'SELECT, fake_privilege', FALSE); -- error +-- Test quoting and dequoting of user names in ACLs +CREATE ROLE "regress_""quoted"; +SELECT makeaclitem('regress_"quoted'::regrole, 'regress_"quoted'::regrole, + 'SELECT', TRUE); +SELECT '"regress_""quoted"=r*/"regress_""quoted"'::aclitem; +SELECT '""=r*/""'::aclitem; -- used to be misparsed as """" +DROP ROLE "regress_""quoted"; + -- Test non-throwing aclitem I/O SELECT pg_input_is_valid('regress_priv_user1=r/regress_priv_user2', 'aclitem'); SELECT pg_input_is_valid('regress_priv_user1=r/', 'aclitem'); -- 2.39.5