Greetings
Consider a table set up as follows:
CREATE TABLE foo (id INT, val TEXT);
ALTER TABLE foo DROP COLUMN val;
When specifying the name of a dropped or non-existent column, the function
"has_column_privilege()" works as expected:
postgres=# SELECT has_column_privilege('foo', 'val', 'SELECT');
ERROR: column "val" of relation "foo" does not exist
postgres=# SELECT has_column_privilege('foo', 'bar', 'SELECT');
ERROR: column "bar" of relation "foo" does not exist
However when specifying a dropped or non-existent attnum, it returns
"TRUE", which is unexpected:
postgres=# SELECT has_column_privilege('foo', 2::int2, 'SELECT');
has_column_privilege
----------------------
t
(1 row)
postgres=# SELECT has_column_privilege('foo', 999::int2, 'SELECT');
has_column_privilege
----------------------
t
(1 row)
The comment on the relevant code section in "src/backend/utils/adt/acl.c"
(related to "column_privilege_check()") indicate that NULL is the intended
return value in these cases:
Likewise, the variants that take an integer attnum
return NULL (rather than throwing an error) if there is no such
pg_attribute entry. All variants return NULL if an attisdropped
column is selected.
The unexpected "TRUE" value is a result of "column_privilege_check()" returning
TRUE if the user has table-level privileges. This returns a valid result with
the function variants where the column name is specified, as the calling
function will have already performed a check of the column through its call to
"convert_column_name()". However when the attnum is specified, the status of
the column never gets checked.
Attached patch resolves this by having "column_privilege_check()" callers
indicate whether they have checked the column. If this is the case, and if the
user as table-level privileges, "column_privilege_check()" can return as before
with no further lookups needed.
If the user has table-level privileges but the column was not checked (i.e
attnum provided), the column lookup is performed.
The regression tests did contain checks for dropped/non-existent attnums,
however one group was acting on a table where the user had no table-level
privilege, giving a fall-through to the column-level check; the other group
contained a check which was just plain wrong.
This issue appears to have been present since "has_column_privilege()" was
introduced in PostreSQL 8.4 (commit 7449427a1e6a099bc7e76164cb99a01d5e87237b),
so falls into the "obscure, extreme corner-case" category, OTOH seems worth
backpatching to supported versions.
The second patch adds a bunch of missing static prototypes to "acl.c",
on general
principles.
I'll add this to the next commitfest.
Regards
Ian Barwick
--
EnterpriseDB: https://www.enterprisedb.com
commit 7e22f2d245888c41b66ae214c226b4a101f27a89
Author: Ian Barwick <[email protected]>
Date: Tue Jan 12 13:40:31 2021 +0900
Fix has_column_privilege() with attnums and non-existent columns
The existence of a column should be confirmed even if the user has
the table-level privilege, otherwise the function will happily report
the user has privilege on a dropped or non-existent column if an
invalid attnum is provided.
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index c7f029e218..be5649b7ac 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -2447,7 +2447,7 @@ has_any_column_privilege_id_id(PG_FUNCTION_ARGS)
*/
static int
column_privilege_check(Oid tableoid, AttrNumber attnum,
- Oid roleid, AclMode mode)
+ Oid roleid, AclMode mode, bool column_checked)
{
AclResult aclresult;
HeapTuple attTuple;
@@ -2472,7 +2472,11 @@ column_privilege_check(Oid tableoid, AttrNumber attnum,
aclresult = pg_class_aclcheck(tableoid, roleid, mode);
- if (aclresult == ACLCHECK_OK)
+ /*
+ * Table-level privilege is present and the column has been checked by the
+ * caller.
+ */
+ if (aclresult == ACLCHECK_OK && column_checked == true)
return true;
/*
@@ -2493,6 +2497,16 @@ column_privilege_check(Oid tableoid, AttrNumber attnum,
}
ReleaseSysCache(attTuple);
+ /*
+ * If the table level privilege exists, and the existence of the column has
+ * been confirmed, we can skip the per-column check.
+ */
+ if (aclresult == ACLCHECK_OK)
+ return true;
+
+ /*
+ * No table privilege, so try per-column privileges.
+ */
aclresult = pg_attribute_aclcheck(tableoid, attnum, roleid, mode);
return (aclresult == ACLCHECK_OK);
@@ -2521,7 +2535,7 @@ has_column_privilege_name_name_name(PG_FUNCTION_ARGS)
colattnum = convert_column_name(tableoid, column);
mode = convert_column_priv_string(priv_type_text);
- privresult = column_privilege_check(tableoid, colattnum, roleid, mode);
+ privresult = column_privilege_check(tableoid, colattnum, roleid, mode, true);
if (privresult < 0)
PG_RETURN_NULL();
PG_RETURN_BOOL(privresult);
@@ -2548,7 +2562,8 @@ has_column_privilege_name_name_attnum(PG_FUNCTION_ARGS)
tableoid = convert_table_name(tablename);
mode = convert_column_priv_string(priv_type_text);
- privresult = column_privilege_check(tableoid, colattnum, roleid, mode);
+ privresult = column_privilege_check(tableoid, colattnum, roleid, mode, false);
+
if (privresult < 0)
PG_RETURN_NULL();
PG_RETURN_BOOL(privresult);
@@ -2575,7 +2590,7 @@ has_column_privilege_name_id_name(PG_FUNCTION_ARGS)
colattnum = convert_column_name(tableoid, column);
mode = convert_column_priv_string(priv_type_text);
- privresult = column_privilege_check(tableoid, colattnum, roleid, mode);
+ privresult = column_privilege_check(tableoid, colattnum, roleid, mode, true);
if (privresult < 0)
PG_RETURN_NULL();
PG_RETURN_BOOL(privresult);
@@ -2600,7 +2615,7 @@ has_column_privilege_name_id_attnum(PG_FUNCTION_ARGS)
roleid = get_role_oid_or_public(NameStr(*username));
mode = convert_column_priv_string(priv_type_text);
- privresult = column_privilege_check(tableoid, colattnum, roleid, mode);
+ privresult = column_privilege_check(tableoid, colattnum, roleid, mode, false);
if (privresult < 0)
PG_RETURN_NULL();
PG_RETURN_BOOL(privresult);
@@ -2627,7 +2642,7 @@ has_column_privilege_id_name_name(PG_FUNCTION_ARGS)
colattnum = convert_column_name(tableoid, column);
mode = convert_column_priv_string(priv_type_text);
- privresult = column_privilege_check(tableoid, colattnum, roleid, mode);
+ privresult = column_privilege_check(tableoid, colattnum, roleid, mode, true);
if (privresult < 0)
PG_RETURN_NULL();
PG_RETURN_BOOL(privresult);
@@ -2652,7 +2667,7 @@ has_column_privilege_id_name_attnum(PG_FUNCTION_ARGS)
tableoid = convert_table_name(tablename);
mode = convert_column_priv_string(priv_type_text);
- privresult = column_privilege_check(tableoid, colattnum, roleid, mode);
+ privresult = column_privilege_check(tableoid, colattnum, roleid, mode, false);
if (privresult < 0)
PG_RETURN_NULL();
PG_RETURN_BOOL(privresult);
@@ -2677,7 +2692,7 @@ has_column_privilege_id_id_name(PG_FUNCTION_ARGS)
colattnum = convert_column_name(tableoid, column);
mode = convert_column_priv_string(priv_type_text);
- privresult = column_privilege_check(tableoid, colattnum, roleid, mode);
+ privresult = column_privilege_check(tableoid, colattnum, roleid, mode, true);
if (privresult < 0)
PG_RETURN_NULL();
PG_RETURN_BOOL(privresult);
@@ -2700,7 +2715,7 @@ has_column_privilege_id_id_attnum(PG_FUNCTION_ARGS)
mode = convert_column_priv_string(priv_type_text);
- privresult = column_privilege_check(tableoid, colattnum, roleid, mode);
+ privresult = column_privilege_check(tableoid, colattnum, roleid, mode, false);
if (privresult < 0)
PG_RETURN_NULL();
PG_RETURN_BOOL(privresult);
@@ -2729,7 +2744,7 @@ has_column_privilege_name_name(PG_FUNCTION_ARGS)
colattnum = convert_column_name(tableoid, column);
mode = convert_column_priv_string(priv_type_text);
- privresult = column_privilege_check(tableoid, colattnum, roleid, mode);
+ privresult = column_privilege_check(tableoid, colattnum, roleid, mode, true);
if (privresult < 0)
PG_RETURN_NULL();
PG_RETURN_BOOL(privresult);
@@ -2756,7 +2771,8 @@ has_column_privilege_name_attnum(PG_FUNCTION_ARGS)
tableoid = convert_table_name(tablename);
mode = convert_column_priv_string(priv_type_text);
- privresult = column_privilege_check(tableoid, colattnum, roleid, mode);
+ privresult = column_privilege_check(tableoid, colattnum, roleid, mode, false);
+
if (privresult < 0)
PG_RETURN_NULL();
PG_RETURN_BOOL(privresult);
@@ -2783,7 +2799,7 @@ has_column_privilege_id_name(PG_FUNCTION_ARGS)
colattnum = convert_column_name(tableoid, column);
mode = convert_column_priv_string(priv_type_text);
- privresult = column_privilege_check(tableoid, colattnum, roleid, mode);
+ privresult = column_privilege_check(tableoid, colattnum, roleid, mode, true);
if (privresult < 0)
PG_RETURN_NULL();
PG_RETURN_BOOL(privresult);
@@ -2808,7 +2824,7 @@ has_column_privilege_id_attnum(PG_FUNCTION_ARGS)
roleid = GetUserId();
mode = convert_column_priv_string(priv_type_text);
- privresult = column_privilege_check(tableoid, colattnum, roleid, mode);
+ privresult = column_privilege_check(tableoid, colattnum, roleid, mode, false);
if (privresult < 0)
PG_RETURN_NULL();
PG_RETURN_BOOL(privresult);
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 7754c20db4..284fc9f081 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -1275,7 +1275,13 @@ select has_column_privilege('mytable','........pg.dropped.2........','select');
select has_column_privilege('mytable',2::int2,'select');
has_column_privilege
----------------------
- t
+
+(1 row)
+
+select has_column_privilege('mytable',99::int2,'select');
+ has_column_privilege
+----------------------
+
(1 row)
revoke select on table mytable from regress_priv_user3;
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 4911ad4add..7b231d78f0 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -777,6 +777,7 @@ alter table mytable drop column f2;
select has_column_privilege('mytable','f2','select');
select has_column_privilege('mytable','........pg.dropped.2........','select');
select has_column_privilege('mytable',2::int2,'select');
+select has_column_privilege('mytable',99::int2,'select');
revoke select on table mytable from regress_priv_user3;
select has_column_privilege('mytable',2::int2,'select');
drop table mytable;
commit e0da3c8aebdcc9b3af27c1ae10df97ee73abf3a2
Author: Ian Barwick <[email protected]>
Date: Tue Jan 12 14:23:47 2021 +0900
Add missing function declarations to acl.c
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index be5649b7ac..0a7b8fc46b 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -89,10 +89,15 @@ static void check_circularity(const Acl *old_acl, const AclItem *mod_aip,
Oid ownerId);
static Acl *recursive_revoke(Acl *acl, Oid grantee, AclMode revoke_privs,
Oid ownerId, DropBehavior behavior);
+static AclMode aclmask_direct(const Acl *acl, Oid roleid, Oid ownerId,
+ AclMode mask, AclMaskHow how);
static AclMode convert_priv_string(text *priv_type_text);
static AclMode convert_any_priv_string(text *priv_type_text,
const priv_map *privileges);
+static const char *convert_aclright_to_string(int aclright);
+static int column_privilege_check(Oid tableoid, AttrNumber attnum,
+ Oid roleid, AclMode mode, bool column_checked);
static Oid convert_table_name(text *tablename);
static AclMode convert_table_priv_string(text *priv_type_text);
@@ -119,6 +124,10 @@ static AclMode convert_role_priv_string(text *priv_type_text);
static AclResult pg_role_aclcheck(Oid role_oid, Oid roleid, AclMode mode);
static void RoleMembershipCacheCallback(Datum arg, int cacheid, uint32 hashvalue);
+static bool has_rolinherit(Oid roleid);
+static List *roles_has_privs_of(Oid roleid);
+static List *roles_is_member_of(Oid roleid);
+static int count_one_bits(AclMode mask);
/*
