Skip to content

Commit 53a936b

Browse files
tglsfdctglsfdc
committed
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
1 parent d69836b commit 53a936b

File tree

3 files changed

+53
-8
lines changed

3 files changed

+53
-8
lines changed

‎src/backend/utils/adt/acl.c

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,22 @@ static AclResult pg_role_aclcheck(Oid role_oid, Oid roleid, AclMode mode);
124124
static void RoleMembershipCacheCallback(Datum arg, int cacheid, uint32 hashvalue);
125125

126126

127+
/*
128+
* Test whether an identifier char can be left unquoted in ACLs.
129+
*
130+
* Formerly, we used isalnum() even on non-ASCII characters, resulting in
131+
* unportable behavior. To ensure dump compatibility with old versions,
132+
* we now treat high-bit-set characters as always requiring quoting during
133+
* putid(), but getid() will always accept them without quotes.
134+
*/
135+
static inline bool
136+
is_safe_acl_char(unsigned char c, bool is_getid)
137+
{
138+
if (IS_HIGHBIT_SET(c))
139+
return is_getid;
140+
return isalnum(c) || c == '_';
141+
}
142+
127143
/*
128144
* getid
129145
* Consumes the first alphanumeric string (identifier) found in string
@@ -149,21 +165,22 @@ getid(const char *s, char *n, Node *escontext)
149165

150166
while (isspace((unsigned char) *s))
151167
s++;
152-
/* This code had better match what putid() does, below */
153168
for (;
154169
*s != '\0' &&
155-
(isalnum((unsigned char) *s) ||
156-
*s == '_' ||
157-
*s == '"' ||
158-
in_quotes);
170+
(in_quotes || *s == '"' || is_safe_acl_char(*s, true));
159171
s++)
160172
{
161173
if (*s == '"')
162174
{
175+
if (!in_quotes)
176+
{
177+
in_quotes = true;
178+
continue;
179+
}
163180
/* safe to look at next char (could be '\0' though) */
164181
if (*(s + 1) != '"')
165182
{
166-
in_quotes = !in_quotes;
183+
in_quotes = false;
167184
continue;
168185
}
169186
/* it's an escaped double quote; skip the escaping char */
@@ -197,10 +214,10 @@ putid(char *p, const char *s)
197214
const char *src;
198215
bool safe = true;
199216

217+
/* Detect whether we need to use double quotes */
200218
for (src = s; *src; src++)
201219
{
202-
/* This test had better match what getid() does, above */
203-
if (!isalnum((unsigned char) *src) && *src != '_')
220+
if (!is_safe_acl_char(*src, false))
204221
{
205222
safe = false;
206223
break;

‎src/test/regress/expected/privileges.out

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2332,6 +2332,26 @@ SELECT makeaclitem('regress_priv_user1'::regrole, 'regress_priv_user2'::regrole,
23322332
SELECT makeaclitem('regress_priv_user1'::regrole, 'regress_priv_user2'::regrole,
23332333
'SELECT, fake_privilege', FALSE); -- error
23342334
ERROR: unrecognized privilege type: "fake_privilege"
2335+
-- Test quoting and dequoting of user names in ACLs
2336+
CREATE ROLE "regress_""quoted";
2337+
SELECT makeaclitem('regress_"quoted'::regrole, 'regress_"quoted'::regrole,
2338+
'SELECT', TRUE);
2339+
makeaclitem
2340+
------------------------------------------
2341+
"regress_""quoted"=r*/"regress_""quoted"
2342+
(1 row)
2343+
2344+
SELECT '"regress_""quoted"=r*/"regress_""quoted"'::aclitem;
2345+
aclitem
2346+
------------------------------------------
2347+
"regress_""quoted"=r*/"regress_""quoted"
2348+
(1 row)
2349+
2350+
SELECT '""=r*/""'::aclitem; -- used to be misparsed as """"
2351+
ERROR: a name must follow the "/" sign
2352+
LINE 1: SELECT '""=r*/""'::aclitem;
2353+
^
2354+
DROP ROLE "regress_""quoted";
23352355
-- Test non-throwing aclitem I/O
23362356
SELECT pg_input_is_valid('regress_priv_user1=r/regress_priv_user2', 'aclitem');
23372357
pg_input_is_valid

‎src/test/regress/sql/privileges.sql

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1481,6 +1481,14 @@ SELECT makeaclitem('regress_priv_user1'::regrole, 'regress_priv_user2'::regrole,
14811481
SELECT makeaclitem('regress_priv_user1'::regrole, 'regress_priv_user2'::regrole,
14821482
'SELECT, fake_privilege', FALSE); -- error
14831483

1484+
-- Test quoting and dequoting of user names in ACLs
1485+
CREATE ROLE "regress_""quoted";
1486+
SELECT makeaclitem('regress_"quoted'::regrole, 'regress_"quoted'::regrole,
1487+
'SELECT', TRUE);
1488+
SELECT '"regress_""quoted"=r*/"regress_""quoted"'::aclitem;
1489+
SELECT '""=r*/""'::aclitem; -- used to be misparsed as """"
1490+
DROP ROLE "regress_""quoted";
1491+
14841492
-- Test non-throwing aclitem I/O
14851493
SELECT pg_input_is_valid('regress_priv_user1=r/regress_priv_user2', 'aclitem');
14861494
SELECT pg_input_is_valid('regress_priv_user1=r/', 'aclitem');

0 commit comments

Comments
 (0)