From b0918c1286d316f6ffa93995452270afd4fc4335 Mon Sep 17 00:00:00 2001 From: Noah Misch Date: Fri, 15 Nov 2024 20:39:56 -0800 Subject: [PATCH] Fix per-session activation of ALTER {ROLE|DATABASE} SET role. After commit 5a2fed911a85ed6d8a015a6bafe3a0d9a69334ae, the catalog state resulting from these commands ceased to affect sessions. Restore the longstanding behavior, which is like beginning the session with a SET ROLE command. If cherry-picking the CVE-2024-10978 fixes, default to including this, too. (This fixes an unintended side effect of fixing CVE-2024-10978.) Back-patch to v12, like that commit. The release team decided to include v12, despite the original intent to halt v12 commits earlier this week. Tom Lane and Noah Misch. Reported by Etienne LAFARGE. Discussion: https://postgr.es/m/CADOZwSb0UsEr4_UTFXC5k7=fyyK8uKXekucd+-uuGjJsGBfxgw@mail.gmail.com --- src/backend/utils/init/miscinit.c | 20 +++++++++++- src/backend/utils/misc/guc.c | 10 +++++- src/test/modules/unsafe_tests/Makefile | 5 ++- .../unsafe_tests/expected/setconfig.out | 31 +++++++++++++++++++ src/test/modules/unsafe_tests/meson.build | 3 ++ .../modules/unsafe_tests/sql/setconfig.sql | 24 ++++++++++++++ 6 files changed, 90 insertions(+), 3 deletions(-) create mode 100644 src/test/modules/unsafe_tests/expected/setconfig.out create mode 100644 src/test/modules/unsafe_tests/sql/setconfig.sql diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c index e7bffcd11b8..fac79f7871a 100644 --- a/src/backend/utils/init/miscinit.c +++ b/src/backend/utils/init/miscinit.c @@ -821,7 +821,25 @@ InitializeSessionUserId(const char *rolename, Oid roleid) { SetAuthenticatedUserId(roleid, is_superuser); - /* Set SessionUserId and related variables via the GUC mechanisms */ + /* + * Set SessionUserId and related variables, including "role", via the + * GUC mechanisms. + * + * Note: ideally we would use PGC_S_DYNAMIC_DEFAULT here, so that + * session_authorization could subsequently be changed from + * pg_db_role_setting entries. Instead, session_authorization in + * pg_db_role_setting has no effect. Changing that would require + * solving two problems: + * + * 1. If pg_db_role_setting has values for both session_authorization + * and role, we could not be sure which order those would be applied + * in, and it would matter. + * + * 2. Sites may have years-old session_authorization entries. There's + * not been any particular reason to remove them. Ending the dormancy + * of those entries could seriously change application behavior, so + * only a major release should do that. + */ SetConfigOption("session_authorization", rname, PGC_BACKEND, PGC_S_OVERRIDE); } diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 6083c095c2a..96c8783fb32 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -4015,6 +4015,12 @@ set_config_option_ext(const char *name, const char *value, * expect that if "role" isn't supposed to be default, it * has been or will be set by a separate reload action. * + * Also, for the call from InitializeSessionUserId with + * source == PGC_S_OVERRIDE, use PGC_S_DYNAMIC_DEFAULT for + * "role"'s source, so that it's still possible to set + * "role" from pg_db_role_setting entries. (See notes in + * InitializeSessionUserId before changing this.) + * * A fine point: for RESET session_authorization, we do * "RESET role" not "SET ROLE NONE" (by passing down NULL * rather than "none" for the value). This would have the @@ -4027,7 +4033,9 @@ set_config_option_ext(const char *name, const char *value, (void) set_config_option_ext("role", value ? "none" : NULL, orig_context, - orig_source, + (orig_source == PGC_S_OVERRIDE) + ? PGC_S_DYNAMIC_DEFAULT + : orig_source, orig_srole, action, true, diff --git a/src/test/modules/unsafe_tests/Makefile b/src/test/modules/unsafe_tests/Makefile index 90d19791871..d4ff227ef07 100644 --- a/src/test/modules/unsafe_tests/Makefile +++ b/src/test/modules/unsafe_tests/Makefile @@ -1,6 +1,9 @@ # src/test/modules/unsafe_tests/Makefile -REGRESS = rolenames alter_system_table guc_privs +REGRESS = rolenames setconfig alter_system_table guc_privs +REGRESS_OPTS = \ + --create-role=regress_authenticated_user_sr \ + --create-role=regress_authenticated_user_ssa # the whole point of these tests is to not run installcheck NO_INSTALLCHECK = 1 diff --git a/src/test/modules/unsafe_tests/expected/setconfig.out b/src/test/modules/unsafe_tests/expected/setconfig.out new file mode 100644 index 00000000000..6a021d9ad03 --- /dev/null +++ b/src/test/modules/unsafe_tests/expected/setconfig.out @@ -0,0 +1,31 @@ +-- This is borderline unsafe in that an additional login-capable user exists +-- during the test run. Under installcheck, a too-permissive pg_hba.conf +-- might allow unwanted logins as regress_authenticated_user_ssa. +ALTER USER regress_authenticated_user_ssa superuser; +CREATE ROLE regress_session_user; +CREATE ROLE regress_current_user; +GRANT regress_current_user TO regress_authenticated_user_sr; +GRANT regress_session_user TO regress_authenticated_user_ssa; +ALTER ROLE regress_authenticated_user_ssa + SET session_authorization = regress_session_user; +ALTER ROLE regress_authenticated_user_sr SET ROLE = regress_current_user; +\c - regress_authenticated_user_sr +SELECT current_user, session_user; + current_user | session_user +----------------------+------------------------------- + regress_current_user | regress_authenticated_user_sr +(1 row) + +-- The longstanding historical behavior is that session_authorization in +-- setconfig has no effect. Hence, session_user remains +-- regress_authenticated_user_ssa. See comment in InitializeSessionUserId(). +\c - regress_authenticated_user_ssa +SELECT current_user, session_user; + current_user | session_user +--------------------------------+-------------------------------- + regress_authenticated_user_ssa | regress_authenticated_user_ssa +(1 row) + +RESET SESSION AUTHORIZATION; +DROP USER regress_session_user; +DROP USER regress_current_user; diff --git a/src/test/modules/unsafe_tests/meson.build b/src/test/modules/unsafe_tests/meson.build index b6806e4a017..1a39763463c 100644 --- a/src/test/modules/unsafe_tests/meson.build +++ b/src/test/modules/unsafe_tests/meson.build @@ -7,9 +7,12 @@ tests += { 'regress': { 'sql': [ 'rolenames', + 'setconfig', 'alter_system_table', 'guc_privs', ], + 'regress_args': ['--create-role=regress_authenticated_user_sr', + '--create-role=regress_authenticated_user_ssa'], 'runningcheck': false, }, } diff --git a/src/test/modules/unsafe_tests/sql/setconfig.sql b/src/test/modules/unsafe_tests/sql/setconfig.sql new file mode 100644 index 00000000000..8817a7c7636 --- /dev/null +++ b/src/test/modules/unsafe_tests/sql/setconfig.sql @@ -0,0 +1,24 @@ +-- This is borderline unsafe in that an additional login-capable user exists +-- during the test run. Under installcheck, a too-permissive pg_hba.conf +-- might allow unwanted logins as regress_authenticated_user_ssa. + +ALTER USER regress_authenticated_user_ssa superuser; +CREATE ROLE regress_session_user; +CREATE ROLE regress_current_user; +GRANT regress_current_user TO regress_authenticated_user_sr; +GRANT regress_session_user TO regress_authenticated_user_ssa; +ALTER ROLE regress_authenticated_user_ssa + SET session_authorization = regress_session_user; +ALTER ROLE regress_authenticated_user_sr SET ROLE = regress_current_user; + +\c - regress_authenticated_user_sr +SELECT current_user, session_user; + +-- The longstanding historical behavior is that session_authorization in +-- setconfig has no effect. Hence, session_user remains +-- regress_authenticated_user_ssa. See comment in InitializeSessionUserId(). +\c - regress_authenticated_user_ssa +SELECT current_user, session_user; +RESET SESSION AUTHORIZATION; +DROP USER regress_session_user; +DROP USER regress_current_user;