Hi all,
I briefly mentioned this issue in another mailing thread [0].
Currently, a user is allowed to execute SET SESSION AUTHORIZATION [1]
if the role they connected to PostgreSQL with was a superuser at the
time of connection. Even if the role is later altered to no longer be a
superuser, the session can still execute SET SESSION AUTHORIZATION, as
long as the session isn't disconnected. As a consequence, if that role
is altered to no longer be a superuser, then the user can use SET
SESSION AUTHORIZATION to switch to another role that is a superuser and
regain superuser privileges. They can even re-grant themselves the
superuser attribute.
It is possible that the user had already run SET SESSION AUTHORIZATION
to set their session to a superuser before their connecting role lost
the superuser attribute. In this case there's not much we can do.
Also, from looking at the code and documentation, it looks like SET
SESSION AUTHORIZATION works this way intentionally. However, I'm unable
to figure out why we'd want it to work this way.
I've attached a patch that would fix this issue by checking the catalog
to see if the connecting role is currently a superuser every time SET
SESSION AUTHORIZATION is run. However, according to the comment I
deleted there's something invalid about reading the catalog from that
function, though I wasn't able to understand it fully.
One downside is that if a user switches their session authorization to
some role, then loses the superuser attribute on their connecting role,
they may be stuck in a that role with no way to reset their session
authorization without disconnecting and reconnecting.
Thanks,
Joe Koshakow
[0]
https://www.postgresql.org/message-id/CAAvxfHco7iGw4NarymhfLWN6PjzYRrbYFt2BnSFeSD5sFzqEJQ%40mail.gmail.com
[1] https://www.postgresql.org/docs/15/sql-set-session-authorization.html
From b5f7d42ea325b2be46b7c93e5404792046f1befc Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <kosh...@gmail.com>
Date: Thu, 15 Jun 2023 14:53:11 -0400
Subject: [PATCH] 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.
---
doc/src/sgml/ref/set_session_auth.sgml | 2 +-
src/backend/utils/init/miscinit.c | 33 +++++++++++++++-----------
2 files changed, 20 insertions(+), 15 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/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index a604432126..459af11691 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -467,7 +467,7 @@ 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).
+ * changed by SET SESSION AUTHORIZATION.
* This is the ID reported by the SESSION_USER SQL function.
*
* OuterUserId is the current user ID in effect at the "outer level" (outside
@@ -492,8 +492,6 @@ 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;
static bool SessionUserIsSuperuser = false;
static int SecurityRestrictionContext = 0;
@@ -731,6 +729,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
@@ -770,10 +769,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) */
@@ -806,7 +805,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),
@@ -818,7 +817,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);
@@ -841,7 +840,6 @@ InitializeSessionUserIdStandalone(void)
Assert(!OidIsValid(AuthenticatedUserId));
AuthenticatedUserId = BOOTSTRAP_SUPERUSERID;
- AuthenticatedUserIsSuperuser = true;
SetSessionUserId(BOOTSTRAP_SUPERUSERID, true);
}
@@ -893,20 +891,25 @@ system_user(PG_FUNCTION_ARGS)
* 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.
*/
void
SetSessionAuthorization(Oid userid, bool is_superuser)
{
+ HeapTuple roleTup;
+ Form_pg_authid rform;
+
/* Must have authenticated already, else can't make permission check */
Assert(OidIsValid(AuthenticatedUserId));
+ roleTup = SearchSysCache1(AUTHOID, ObjectIdGetDatum(AuthenticatedUserId));
+ if (!HeapTupleIsValid(roleTup))
+ ereport(FATAL,
+ (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
+ errmsg("role with OID %u does not exist", AuthenticatedUserId)));
+ rform = (Form_pg_authid) GETSTRUCT(roleTup);
+
if (userid != AuthenticatedUserId &&
- !AuthenticatedUserIsSuperuser)
+ !rform->rolsuper)
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied to set session authorization")));
@@ -916,6 +919,8 @@ SetSessionAuthorization(Oid userid, bool is_superuser)
SetConfigOption("is_superuser",
is_superuser ? "on" : "off",
PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
+
+ ReleaseSysCache(roleTup);
}
/*
--
2.34.1
