From b0b1effeaaab81db65857d792a343c0079f87c79 Mon Sep 17 00:00:00 2001 From: Joshua Brindle Date: Tue, 26 Oct 2021 14:19:44 -0700 Subject: [PATCH] use has_privs_for_roles for predefined role checks Generally if a role is granted membership to another role with NOINHERIT they must use SET ROLE to access the privileges of that role, however with predefined roles the membership and privilege is conflated, as demonstrated by: CREATE ROLE readrole; CREATE ROLE role2 NOINHERIT; CREATE ROLE brindle LOGIN; GRANT role2 TO brindle; CREATE TABLE foo(i INT); GRANT readrole TO role2; GRANT ALL ON TABLE foo TO readrole; GRANT pg_read_all_stats,pg_read_all_settings,pg_read_server_files,pg_write_server_files,pg_execute_server_program TO role2; Log in as brindle: postgres=> select current_user; current_user -------------- brindle (1 row) postgres=> SELECT * FROM foo; ERROR: permission denied for table foo postgres=> SELECT DISTINCT query FROM pg_stat_activity; query ---------------------------------------------- SELECT DISTINCT query FROM pg_stat_activity; (2 rows) postgres=> SET ROLE readrole; SET postgres=> SELECT * FROM foo; i Signed-off-by: Joshua Brindle --- contrib/adminpack/adminpack.c | 2 +- contrib/file_fdw/file_fdw.c | 8 ++++---- contrib/file_fdw/output/file_fdw.source | 2 +- .../pg_stat_statements/pg_stat_statements.c | 4 ++-- contrib/pgrowlocks/pgrowlocks.c | 2 +- doc/src/sgml/catalogs.sgml | 12 +++++------ src/backend/commands/copy.c | 12 +++++------ src/backend/replication/walreceiver.c | 8 ++++---- src/backend/replication/walsender.c | 8 ++++---- src/backend/utils/adt/acl.c | 4 ++++ src/backend/utils/adt/dbsize.c | 8 ++++---- src/backend/utils/adt/genfile.c | 6 +++--- src/backend/utils/adt/pgstatfuncs.c | 2 +- src/backend/utils/misc/guc.c | 20 +++++++++---------- .../unsafe_tests/expected/rolenames.out | 2 +- 15 files changed, 52 insertions(+), 48 deletions(-) diff --git a/contrib/adminpack/adminpack.c b/contrib/adminpack/adminpack.c index 48c17469104..0457d7b591d 100644 --- a/contrib/adminpack/adminpack.c +++ b/contrib/adminpack/adminpack.c @@ -79,7 +79,7 @@ convert_and_check_filename(text *arg) * files on the server as the PG user, so no need to do any further checks * here. */ - if (is_member_of_role(GetUserId(), ROLE_PG_WRITE_SERVER_FILES)) + if (has_privs_of_role(GetUserId(), ROLE_PG_WRITE_SERVER_FILES)) return filename; /* diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c index 2c2f149fb01..a769b2ef7b9 100644 --- a/contrib/file_fdw/file_fdw.c +++ b/contrib/file_fdw/file_fdw.c @@ -269,16 +269,16 @@ file_fdw_validator(PG_FUNCTION_ARGS) * otherwise there'd still be a security hole. */ if (strcmp(def->defname, "filename") == 0 && - !is_member_of_role(GetUserId(), ROLE_PG_READ_SERVER_FILES)) + !has_privs_of_role(GetUserId(), ROLE_PG_READ_SERVER_FILES)) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("only superuser or a member of the pg_read_server_files role may specify the filename option of a file_fdw foreign table"))); + errmsg("only superuser or a role with privileges of the pg_read_server_files role may specify the filename option of a file_fdw foreign table"))); if (strcmp(def->defname, "program") == 0 && - !is_member_of_role(GetUserId(), ROLE_PG_EXECUTE_SERVER_PROGRAM)) + !has_privs_of_role(GetUserId(), ROLE_PG_EXECUTE_SERVER_PROGRAM)) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("only superuser or a member of the pg_execute_server_program role may specify the program option of a file_fdw foreign table"))); + errmsg("only superuser or a role with privileges of the pg_execute_server_program role may specify the program option of a file_fdw foreign table"))); filename = defGetString(def); } diff --git a/contrib/file_fdw/output/file_fdw.source b/contrib/file_fdw/output/file_fdw.source index 52b4d5f1df7..a1d44ed665a 100644 --- a/contrib/file_fdw/output/file_fdw.source +++ b/contrib/file_fdw/output/file_fdw.source @@ -436,7 +436,7 @@ ALTER FOREIGN TABLE agg_text OWNER TO regress_file_fdw_user; ALTER FOREIGN TABLE agg_text OPTIONS (SET format 'text'); SET ROLE regress_file_fdw_user; ALTER FOREIGN TABLE agg_text OPTIONS (SET format 'text'); -ERROR: only superuser or a member of the pg_read_server_files role may specify the filename option of a file_fdw foreign table +ERROR: only superuser or a role with privileges of the pg_read_server_files role may specify the filename option of a file_fdw foreign table SET ROLE regress_file_fdw_superuser; -- cleanup RESET ROLE; diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index 726ba59e2bf..f6120f5c48b 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -1511,8 +1511,8 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo, HASH_SEQ_STATUS hash_seq; pgssEntry *entry; - /* Superusers or members of pg_read_all_stats members are allowed */ - is_allowed_role = is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS); + /* Superusers or roles with the privileges of pg_read_all_stats members are allowed */ + is_allowed_role = has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS); /* hash table must exist already */ if (!pgss || !pgss_hash) diff --git a/contrib/pgrowlocks/pgrowlocks.c b/contrib/pgrowlocks/pgrowlocks.c index 669a7d7730b..b9b724a554d 100644 --- a/contrib/pgrowlocks/pgrowlocks.c +++ b/contrib/pgrowlocks/pgrowlocks.c @@ -130,7 +130,7 @@ pgrowlocks(PG_FUNCTION_ARGS) aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(), ACL_SELECT); if (aclresult != ACLCHECK_OK) - aclresult = is_member_of_role(GetUserId(), ROLE_PG_STAT_SCAN_TABLES) ? ACLCHECK_OK : ACLCHECK_NO_PRIV; + aclresult = has_privs_of_role(GetUserId(), ROLE_PG_STAT_SCAN_TABLES) ? ACLCHECK_OK : ACLCHECK_NO_PRIV; if (aclresult != ACLCHECK_OK) aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind), diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index c1d11be73f7..87254c4b8bd 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -9916,8 +9916,8 @@ SCRAM-SHA-256$<iteration count>:&l By default, the pg_backend_memory_contexts view can be - read only by superusers or members of the pg_read_all_stats - role. + read only by superusers or roles with the privileges of the + pg_read_all_stats role. @@ -12358,7 +12358,7 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx Configuration file the current value was set in (null for values set from sources other than configuration files, or when - examined by a user who is neither a superuser or a member of + examined by a user who neither is a superuser nor has privileges of pg_read_all_settings); helpful when using include directives in configuration files @@ -12371,7 +12371,7 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx Line number within the configuration file the current value was set at (null for values set from sources other than configuration files, - or when examined by a user who is neither a superuser or a member of + or when examined by a user who neither is a superuser nor has privileges of pg_read_all_settings). @@ -12747,8 +12747,8 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx By default, the pg_shmem_allocations view can be - read only by superusers or members of the pg_read_all_stats - role. + read only by superusers or roles with privilges of the + pg_read_all_stats role. diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index 53f48531419..e26ff42fd82 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -80,26 +80,26 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt, { if (stmt->is_program) { - if (!is_member_of_role(GetUserId(), ROLE_PG_EXECUTE_SERVER_PROGRAM)) + if (!has_privs_of_role(GetUserId(), ROLE_PG_EXECUTE_SERVER_PROGRAM)) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("must be superuser or a member of the pg_execute_server_program role to COPY to or from an external program"), + errmsg("must be superuser or have privileges of the pg_execute_server_program role to COPY to or from an external program"), errhint("Anyone can COPY to stdout or from stdin. " "psql's \\copy command also works for anyone."))); } else { - if (is_from && !is_member_of_role(GetUserId(), ROLE_PG_READ_SERVER_FILES)) + if (is_from && !has_privs_of_role(GetUserId(), ROLE_PG_READ_SERVER_FILES)) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("must be superuser or a member of the pg_read_server_files role to COPY from a file"), + errmsg("must be superuser or have privileges of the pg_read_server_files role to COPY from a file"), errhint("Anyone can COPY to stdout or from stdin. " "psql's \\copy command also works for anyone."))); - if (!is_from && !is_member_of_role(GetUserId(), ROLE_PG_WRITE_SERVER_FILES)) + if (!is_from && !has_privs_of_role(GetUserId(), ROLE_PG_WRITE_SERVER_FILES)) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("must be superuser or a member of the pg_write_server_files role to COPY to a file"), + errmsg("must be superuser or have privileges of the pg_write_server_files role to COPY to a file"), errhint("Anyone can COPY to stdout or from stdin. " "psql's \\copy command also works for anyone."))); } diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c index 7a7eb3784e7..9de72962322 100644 --- a/src/backend/replication/walreceiver.c +++ b/src/backend/replication/walreceiver.c @@ -1402,12 +1402,12 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS) /* Fetch values */ values[0] = Int32GetDatum(pid); - if (!is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS)) + if (!has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS)) { /* - * Only superusers and members of pg_read_all_stats can see details. - * Other users only get the pid value to know whether it is a WAL - * receiver, but no details. + * Only superusers and roles with privileges of pg_read_all_stats + * can see details. Other users only get the pid value to know whether + * it is a WAL receiver, but no details. */ MemSet(&nulls[1], true, sizeof(bool) * (tupdesc->natts - 1)); } diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c index fff7dfc6409..c9b57f2ff36 100644 --- a/src/backend/replication/walsender.c +++ b/src/backend/replication/walsender.c @@ -3487,12 +3487,12 @@ pg_stat_get_wal_senders(PG_FUNCTION_ARGS) memset(nulls, 0, sizeof(nulls)); values[0] = Int32GetDatum(pid); - if (!is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS)) + if (!has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS)) { /* - * Only superusers and members of pg_read_all_stats can see - * details. Other users only get the pid value to know it's a - * walsender, but no details. + * Only superusers and roles with privileges of pg_read_all_stats + * can see details. Other users only get the pid value to know + * it's a walsender, but no details. */ MemSet(&nulls[1], true, PG_STAT_GET_WAL_SENDERS_COLS - 1); } diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c index 67f8b29434a..e2ce4582f19 100644 --- a/src/backend/utils/adt/acl.c +++ b/src/backend/utils/adt/acl.c @@ -4864,6 +4864,8 @@ has_privs_of_role(Oid member, Oid role) * Is member a member of role (directly or indirectly)? * * This is defined to recurse through roles regardless of rolinherit. + * + * Do not use this for privilege checking, instead use has_privs_of_role() */ bool is_member_of_role(Oid member, Oid role) @@ -4904,6 +4906,8 @@ check_is_member_of_role(Oid member, Oid role) * * This is identical to is_member_of_role except we ignore superuser * status. + * + * Do not use this for privilege checking, instead use has_privs_of_role() */ bool is_member_of_role_nosuper(Oid member, Oid role) diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c index d5a7fb13f3c..91ec2615cd5 100644 --- a/src/backend/utils/adt/dbsize.c +++ b/src/backend/utils/adt/dbsize.c @@ -112,12 +112,12 @@ calculate_database_size(Oid dbOid) AclResult aclresult; /* - * User must have connect privilege for target database or be a member of + * User must have connect privilege for target database or have privileges of * pg_read_all_stats */ aclresult = pg_database_aclcheck(dbOid, GetUserId(), ACL_CONNECT); if (aclresult != ACLCHECK_OK && - !is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS)) + !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS)) { aclcheck_error(aclresult, OBJECT_DATABASE, get_database_name(dbOid)); @@ -196,12 +196,12 @@ calculate_tablespace_size(Oid tblspcOid) AclResult aclresult; /* - * User must be a member of pg_read_all_stats or have CREATE privilege for + * User must have privileges of pg_read_all_stats or have CREATE privilege for * target tablespace, either explicitly granted or implicitly because it * is default for current database. */ if (tblspcOid != MyDatabaseTableSpace && - !is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS)) + !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS)) { aclresult = pg_tablespace_aclcheck(tblspcOid, GetUserId(), ACL_CREATE); if (aclresult != ACLCHECK_OK) diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c index c436d9318b6..f87f77093a6 100644 --- a/src/backend/utils/adt/genfile.c +++ b/src/backend/utils/adt/genfile.c @@ -58,11 +58,11 @@ convert_and_check_filename(text *arg) canonicalize_path(filename); /* filename can change length here */ /* - * Members of the 'pg_read_server_files' role are allowed to access any - * files on the server as the PG user, so no need to do any further checks + * Roles with privleges of the 'pg_read_server_files' role are allowed to access + * any files on the server as the PG user, so no need to do any further checks * here. */ - if (is_member_of_role(GetUserId(), ROLE_PG_READ_SERVER_FILES)) + if (has_privs_of_role(GetUserId(), ROLE_PG_READ_SERVER_FILES)) return filename; /* diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c index ff5aedc99cb..56762a7d98d 100644 --- a/src/backend/utils/adt/pgstatfuncs.c +++ b/src/backend/utils/adt/pgstatfuncs.c @@ -34,7 +34,7 @@ #define UINT32_ACCESS_ONCE(var) ((uint32)(*((volatile uint32 *)&(var)))) -#define HAS_PGSTAT_PERMISSIONS(role) (is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS) || has_privs_of_role(GetUserId(), role)) +#define HAS_PGSTAT_PERMISSIONS(role) (has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS) || has_privs_of_role(GetUserId(), role)) Datum pg_stat_get_numscans(PG_FUNCTION_ARGS) diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index e91d5a3cfda..e400bfdea13 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -8154,10 +8154,10 @@ GetConfigOption(const char *name, bool missing_ok, bool restrict_privileged) return NULL; if (restrict_privileged && (record->flags & GUC_SUPERUSER_ONLY) && - !is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS)) + !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS)) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("must be superuser or a member of pg_read_all_settings to examine \"%s\"", + errmsg("must be superuser have privileges of pg_read_all_settings to examine \"%s\"", name))); switch (record->vartype) @@ -8201,10 +8201,10 @@ GetConfigOptionResetString(const char *name) record = find_option(name, false, false, ERROR); Assert(record != NULL); if ((record->flags & GUC_SUPERUSER_ONLY) && - !is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS)) + !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS)) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("must be superuser or a member of pg_read_all_settings to examine \"%s\"", + errmsg("must be superuser or have privileges of pg_read_all_settings to examine \"%s\"", name))); switch (record->vartype) @@ -9448,7 +9448,7 @@ ShowAllGUCConfig(DestReceiver *dest) if ((conf->flags & GUC_NO_SHOW_ALL) || ((conf->flags & GUC_SUPERUSER_ONLY) && - !is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS))) + !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS))) continue; /* assign to the values array */ @@ -9515,7 +9515,7 @@ get_explain_guc_options(int *num) /* return only options visible to the current user */ if ((conf->flags & GUC_NO_SHOW_ALL) || ((conf->flags & GUC_SUPERUSER_ONLY) && - !is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS))) + !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS))) continue; /* return only options that are different from their boot values */ @@ -9597,10 +9597,10 @@ GetConfigOptionByName(const char *name, const char **varname, bool missing_ok) } if ((record->flags & GUC_SUPERUSER_ONLY) && - !is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS)) + !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS)) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("must be superuser or a member of pg_read_all_settings to examine \"%s\"", + errmsg("must be superuser or have privileges of pg_read_all_settings to examine \"%s\"", name))); if (varname) @@ -9628,7 +9628,7 @@ GetConfigOptionByNum(int varnum, const char **values, bool *noshow) { if ((conf->flags & GUC_NO_SHOW_ALL) || ((conf->flags & GUC_SUPERUSER_ONLY) && - !is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS))) + !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS))) *noshow = true; else *noshow = false; @@ -9823,7 +9823,7 @@ GetConfigOptionByNum(int varnum, const char **values, bool *noshow) * insufficiently-privileged users. */ if (conf->source == PGC_S_FILE && - is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS)) + has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS)) { values[14] = conf->sourcefile; snprintf(buffer, sizeof(buffer), "%d", conf->sourceline); diff --git a/src/test/modules/unsafe_tests/expected/rolenames.out b/src/test/modules/unsafe_tests/expected/rolenames.out index eb608fdc2ea..88b1ff843be 100644 --- a/src/test/modules/unsafe_tests/expected/rolenames.out +++ b/src/test/modules/unsafe_tests/expected/rolenames.out @@ -1077,7 +1077,7 @@ SHOW session_preload_libraries; SET SESSION AUTHORIZATION regress_role_nopriv; -- fails with role not member of pg_read_all_settings SHOW session_preload_libraries; -ERROR: must be superuser or a member of pg_read_all_settings to examine "session_preload_libraries" +ERROR: must be superuser or have privileges of pg_read_all_settings to examine "session_preload_libraries" RESET SESSION AUTHORIZATION; ERROR: current transaction is aborted, commands ignored until end of transaction block ROLLBACK; -- 2.31.1