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 <peter@eisentraut.org>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/3792884.1751492172@sss.pgh.pa.us
Backpatch-through: 13

diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index 5b7236abc98a31d5a3a926bbd646807d90dd3db8..84d9bd5c2a62aefc775f036cb65fc2b8a756a764 100644 (file)
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -118,6 +118,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
@@ -140,21 +156,22 @@ getid(const char *s, char *n)
 
        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 */
@@ -188,10 +205,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 6d58833af23ea810300b7147d638c1ca56991494..d076b5d1f8bdc9528f80221a5a33abf06648cc61 100644 (file)
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -2184,6 +2184,26 @@ SELECT has_table_privilege('regress_priv_user1', 'testns.acltest1', 'INSERT'); -
 ALTER DEFAULT PRIVILEGES FOR ROLE regress_priv_user1 REVOKE EXECUTE ON FUNCTIONS FROM public;
 ALTER DEFAULT PRIVILEGES IN SCHEMA testns GRANT USAGE ON SCHEMAS TO regress_priv_user2; -- error
 ERROR:  cannot use IN SCHEMA clause when using GRANT/REVOKE ON SCHEMAS
+-- 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";
 --
 -- Testing blanket default grants is very hazardous since it might change
 -- the privileges attached to objects created by concurrent regression tests.

diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index fe657be533e80c8d45d6e2f9baff4894adab42f4..1d302253f8d5eafe3e5bc6ad7f8266e49da6f63b 100644 (file)
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -1387,6 +1387,14 @@ ALTER DEFAULT PRIVILEGES FOR ROLE regress_priv_user1 REVOKE EXECUTE ON FUNCTIONS
 
 ALTER DEFAULT PRIVILEGES IN SCHEMA testns GRANT USAGE ON SCHEMAS TO regress_priv_user2; -- 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";
+
 --
 -- Testing blanket default grants is very hazardous since it might change
 -- the privileges attached to objects created by concurrent regression tests.