On Sun, Jul 09, 2023 at 08:54:30PM -0400, Joseph Koshakow wrote: > I just realized that you moved this comment from > SetSessionAuthorization. I think we should leave the part about setting > the GUC variable is_superuser on top of SetSessionAuthorization since > that's where we actually set the GUC.
Okay. Here's a new patch set in which I believe I've addressed all feedback. I didn't keep the GetAuthenticatedUserIsSuperuser() helper function around, as I didn't see a strong need for it. And I haven't touched the "is_superuser" GUC, either. I figured we can take up any changes for it in the other thread. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
>From 691c7a30d86385cca33a7ac43a5d63c4bc39dae1 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <nat...@postgresql.org> Date: Sun, 9 Jul 2023 11:28:56 -0700 Subject: [PATCH v6 1/3] Rename session_auth_is_superuser to current_role_is_superuser. Suggested-by: Joseph Koshakow Discussion: https://postgr.es/m/CAAvxfHc-HHzONQ2oXdvhFF9ayRnidPwK%2BfVBhRzaBWYYLVQL-g%40mail.gmail.com --- src/backend/access/transam/parallel.c | 2 +- src/backend/utils/misc/guc_tables.c | 9 ++++++--- src/include/utils/guc.h | 2 +- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c index 2b8bc2f58d..12d97998cc 100644 --- a/src/backend/access/transam/parallel.c +++ b/src/backend/access/transam/parallel.c @@ -326,7 +326,7 @@ InitializeParallelDSM(ParallelContext *pcxt) fps->database_id = MyDatabaseId; fps->authenticated_user_id = GetAuthenticatedUserId(); fps->outer_user_id = GetCurrentRoleId(); - fps->is_superuser = session_auth_is_superuser; + fps->is_superuser = current_role_is_superuser; GetUserIdAndSecContext(&fps->current_user_id, &fps->sec_context); GetTempNamespaceState(&fps->temp_namespace_id, &fps->temp_toast_namespace_id); diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index c14456060c..93dc2e7680 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -511,7 +511,7 @@ bool check_function_bodies = true; * details. */ bool default_with_oids = false; -bool session_auth_is_superuser; +bool current_role_is_superuser; int log_min_error_statement = ERROR; int log_min_messages = WARNING; @@ -1037,13 +1037,16 @@ struct config_bool ConfigureNamesBool[] = NULL, NULL, NULL }, { - /* Not for general use --- used by SET SESSION AUTHORIZATION */ + /* + * Not for general use --- used by SET SESSION AUTHORIZATION and SET + * ROLE + */ {"is_superuser", PGC_INTERNAL, UNGROUPED, gettext_noop("Shows whether the current user is a superuser."), NULL, GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE }, - &session_auth_is_superuser, + ¤t_role_is_superuser, false, NULL, NULL, NULL }, diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h index d5253c7ed2..223a19f80d 100644 --- a/src/include/utils/guc.h +++ b/src/include/utils/guc.h @@ -250,7 +250,7 @@ extern PGDLLIMPORT bool log_statement_stats; extern PGDLLIMPORT bool log_btree_build_stats; extern PGDLLIMPORT bool check_function_bodies; -extern PGDLLIMPORT bool session_auth_is_superuser; +extern PGDLLIMPORT bool current_role_is_superuser; extern PGDLLIMPORT bool log_duration; extern PGDLLIMPORT int log_parameter_max_length; -- 2.25.1
>From a08490dd85f0d41830aa06982a5c5ea588ba79d1 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <nat...@postgresql.org> Date: Sat, 8 Jul 2023 21:35:03 -0700 Subject: [PATCH v6 2/3] Move session auth privilege check to check_session_authorization(). Author: Joseph Koshakow Discussion: https://postgr.es/m/CAAvxfHc-HHzONQ2oXdvhFF9ayRnidPwK%2BfVBhRzaBWYYLVQL-g%40mail.gmail.com --- src/backend/commands/variable.c | 21 +++++++++++++++++++++ src/backend/utils/init/miscinit.c | 30 ++++++++++++------------------ src/include/miscadmin.h | 1 + 3 files changed, 34 insertions(+), 18 deletions(-) diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c index f0f2e07655..b8a75012a5 100644 --- a/src/backend/commands/variable.c +++ b/src/backend/commands/variable.c @@ -846,6 +846,27 @@ check_session_authorization(char **newval, void **extra, GucSource source) ReleaseSysCache(roleTup); + /* + * Only superusers may SET SESSION AUTHORIZATION to roles other than + * itself. Note that in case of multiple SETs in a single session, the + * original authenticated user's superuserness is what matters. + */ + if (roleid != GetAuthenticatedUserId() && + !GetAuthenticatedUserIsSuperuser()) + { + if (source == PGC_S_TEST) + { + ereport(NOTICE, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission will be denied to set session authorization \"%s\"", + *newval))); + return true; + } + GUC_check_errcode(ERRCODE_INSUFFICIENT_PRIVILEGE); + GUC_check_errmsg("permission denied to set session authorization"); + return false; + } + /* Set up "extra" struct for assign_session_authorization to use */ myextra = (role_auth_extra *) guc_malloc(LOG, sizeof(role_auth_extra)); if (!myextra) diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c index a604432126..64545bc373 100644 --- a/src/backend/utils/init/miscinit.c +++ b/src/backend/utils/init/miscinit.c @@ -582,6 +582,16 @@ GetAuthenticatedUserId(void) return AuthenticatedUserId; } +/* + * Return whether the authenticated user was superuser at connection start. + */ +bool +GetAuthenticatedUserIsSuperuser(void) +{ + Assert(OidIsValid(AuthenticatedUserId)); + return AuthenticatedUserIsSuperuser; +} + /* * GetUserIdAndSecContext/SetUserIdAndSecContext - get/set the current user ID @@ -889,28 +899,12 @@ system_user(PG_FUNCTION_ARGS) /* * Change session auth ID while running * - * Only a superuser may set auth ID to something other than himself. Note - * that in case of multiple SETs in a single session, the original userid's - * superuserness is what matters. But we set the GUC variable is_superuser - * to indicate whether the *current* session userid is a superuser. - * - * Note: this is not an especially clean place to do the permission check. - * It's OK because the check does not require catalog access and can't - * fail during an end-of-transaction GUC reversion, but we may someday - * have to push it up into assign_session_authorization. + * Note that we set the GUC variable is_superuser to indicate whether the + * current role is a superuser. */ void SetSessionAuthorization(Oid userid, bool is_superuser) { - /* Must have authenticated already, else can't make permission check */ - Assert(OidIsValid(AuthenticatedUserId)); - - if (userid != AuthenticatedUserId && - !AuthenticatedUserIsSuperuser) - ereport(ERROR, - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("permission denied to set session authorization"))); - SetSessionUserId(userid, is_superuser); SetConfigOption("is_superuser", diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index 14bd574fc2..11d6e6869d 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -357,6 +357,7 @@ extern Oid GetUserId(void); extern Oid GetOuterUserId(void); extern Oid GetSessionUserId(void); extern Oid GetAuthenticatedUserId(void); +extern bool GetAuthenticatedUserIsSuperuser(void); extern void GetUserIdAndSecContext(Oid *userid, int *sec_context); extern void SetUserIdAndSecContext(Oid userid, int sec_context); extern bool InLocalUserIdChange(void); -- 2.25.1
>From 7d54a90606a35571151db433df94976e2024c205 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <nat...@postgresql.org> Date: Sun, 9 Jul 2023 15:01:53 -0700 Subject: [PATCH v6 3/3] Prevent non-superusers from altering session auth Previously, if a user connected with as a role that had the superuser attribute, then they could always execute a SET SESSION AUTHORIZATION statement for the duration of their session. Even if the role was altered to set superuser to false, the user was still allowed to execute SET SESSION AUTHORIZATION. This allowed them to set their session role to some other superuser and effectively regain the superuser privileges. They could even reset their own superuser attribute to true. This commit alters the privilege checks for SET SESSION AUTHORIZATION such that a user can only execute it if the role they connected with is currently a superuser. This prevents users from regaining superuser privileges after it has been revoked. Author: Joseph Koshakow Discussion: https://postgr.es/m/CAAvxfHc-HHzONQ2oXdvhFF9ayRnidPwK%2BfVBhRzaBWYYLVQL-g%40mail.gmail.com --- doc/src/sgml/ref/set_session_auth.sgml | 2 +- src/backend/commands/variable.c | 2 +- src/backend/utils/init/miscinit.c | 28 ++++++++------------------ src/include/miscadmin.h | 1 - 4 files changed, 10 insertions(+), 23 deletions(-) diff --git a/doc/src/sgml/ref/set_session_auth.sgml b/doc/src/sgml/ref/set_session_auth.sgml index f8fcafc194..94adab2468 100644 --- a/doc/src/sgml/ref/set_session_auth.sgml +++ b/doc/src/sgml/ref/set_session_auth.sgml @@ -51,7 +51,7 @@ RESET SESSION AUTHORIZATION <para> The session user identifier can be changed only if the initial session - user (the <firstterm>authenticated user</firstterm>) had the + user (the <firstterm>authenticated user</firstterm>) has the superuser privilege. Otherwise, the command is accepted only if it specifies the authenticated user name. </para> diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c index b8a75012a5..8404818d09 100644 --- a/src/backend/commands/variable.c +++ b/src/backend/commands/variable.c @@ -852,7 +852,7 @@ check_session_authorization(char **newval, void **extra, GucSource source) * original authenticated user's superuserness is what matters. */ if (roleid != GetAuthenticatedUserId() && - !GetAuthenticatedUserIsSuperuser()) + !superuser_arg(GetAuthenticatedUserId())) { if (source == PGC_S_TEST) { diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c index 64545bc373..1e671c560c 100644 --- a/src/backend/utils/init/miscinit.c +++ b/src/backend/utils/init/miscinit.c @@ -467,8 +467,8 @@ ChangeToDataDir(void) * AuthenticatedUserId is determined at connection start and never changes. * * SessionUserId is initially the same as AuthenticatedUserId, but can be - * changed by SET SESSION AUTHORIZATION (if AuthenticatedUserIsSuperuser). - * This is the ID reported by the SESSION_USER SQL function. + * changed by SET SESSION AUTHORIZATION (if AuthenticatedUserId is a + * superuser). This is the ID reported by the SESSION_USER SQL function. * * OuterUserId is the current user ID in effect at the "outer level" (outside * any transaction or function). This is initially the same as SessionUserId, @@ -492,8 +492,7 @@ static Oid OuterUserId = InvalidOid; static Oid CurrentUserId = InvalidOid; static const char *SystemUser = NULL; -/* We also have to remember the superuser state of some of these levels */ -static bool AuthenticatedUserIsSuperuser = false; +/* We also have to remember the superuser state of the session user */ static bool SessionUserIsSuperuser = false; static int SecurityRestrictionContext = 0; @@ -582,16 +581,6 @@ GetAuthenticatedUserId(void) return AuthenticatedUserId; } -/* - * Return whether the authenticated user was superuser at connection start. - */ -bool -GetAuthenticatedUserIsSuperuser(void) -{ - Assert(OidIsValid(AuthenticatedUserId)); - return AuthenticatedUserIsSuperuser; -} - /* * GetUserIdAndSecContext/SetUserIdAndSecContext - get/set the current user ID @@ -741,6 +730,7 @@ InitializeSessionUserId(const char *rolename, Oid roleid) HeapTuple roleTup; Form_pg_authid rform; char *rname; + bool is_superuser; /* * Don't do scans if we're bootstrapping, none of the system catalogs @@ -780,10 +770,10 @@ InitializeSessionUserId(const char *rolename, Oid roleid) rname = NameStr(rform->rolname); AuthenticatedUserId = roleid; - AuthenticatedUserIsSuperuser = rform->rolsuper; + is_superuser = rform->rolsuper; /* This sets OuterUserId/CurrentUserId too */ - SetSessionUserId(roleid, AuthenticatedUserIsSuperuser); + SetSessionUserId(roleid, is_superuser); /* Also mark our PGPROC entry with the authenticated user id */ /* (We assume this is an atomic store so no lock is needed) */ @@ -816,7 +806,7 @@ InitializeSessionUserId(const char *rolename, Oid roleid) * just document that the connection limit is approximate. */ if (rform->rolconnlimit >= 0 && - !AuthenticatedUserIsSuperuser && + !is_superuser && CountUserBackends(roleid) > rform->rolconnlimit) ereport(FATAL, (errcode(ERRCODE_TOO_MANY_CONNECTIONS), @@ -828,7 +818,7 @@ InitializeSessionUserId(const char *rolename, Oid roleid) SetConfigOption("session_authorization", rname, PGC_BACKEND, PGC_S_OVERRIDE); SetConfigOption("is_superuser", - AuthenticatedUserIsSuperuser ? "on" : "off", + is_superuser ? "on" : "off", PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT); ReleaseSysCache(roleTup); @@ -851,8 +841,6 @@ InitializeSessionUserIdStandalone(void) Assert(!OidIsValid(AuthenticatedUserId)); AuthenticatedUserId = BOOTSTRAP_SUPERUSERID; - AuthenticatedUserIsSuperuser = true; - SetSessionUserId(BOOTSTRAP_SUPERUSERID, true); } diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index 11d6e6869d..14bd574fc2 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -357,7 +357,6 @@ extern Oid GetUserId(void); extern Oid GetOuterUserId(void); extern Oid GetSessionUserId(void); extern Oid GetAuthenticatedUserId(void); -extern bool GetAuthenticatedUserIsSuperuser(void); extern void GetUserIdAndSecContext(Oid *userid, int *sec_context); extern void SetUserIdAndSecContext(Oid userid, int sec_context); extern bool InLocalUserIdChange(void); -- 2.25.1
