On Sun, Jul 9, 2023 at 12:47 AM Nathan Bossart <nathandboss...@gmail.com>
wrote:
> I think we should split this into two patches: one to move the permission
> check to check_session_authorization() and another for the behavior
change.
> I've attached an attempt at the first one (that borrows heavily from your
> latest patch). AFAICT the only reason that the permission check lives in
> SetSessionAuthorization() is because AuthenticatedUserIsSuperuser is
static
> to miscinit.c and doesn't have an accessor function. I added one, but it
> would probably just be removed by the following patch. WDYT?
I think that's a good idea. We could even keep around the accessor
function as a good place to bundle the calls to
Assert(OidIsValid(AuthenticatedUserId))
and
superuser_arg(AuthenticatedUserId)
> * Only a superuser may set auth ID to something other than himself
Is "auth ID" the right term here? Maybe something like "Only a
superuser may set their session authorization/ID to something other
than their authenticated ID."
> But we set the GUC variable
> * is_superuser to indicate whether the *current* session userid is a
> * superuser.
Just a small correction here, I believe the is_superuser GUC is meant
to indicate whether the current user id is a superuser, not the current
session user id. We only update is_superuser in SetSessionAuthorization
because we are also updating the current user id in SetSessionUserId.
For example,
test=# CREATE ROLE r1 SUPERUSER;
CREATE ROLE
test=# CREATE ROLE r2;
CREATE ROLE
test=# SET SESSION AUTHORIZATION r1;
SET
test=# SET ROLE r2;
SET
test=> SELECT session_user, current_user;
session_user | current_user
--------------+--------------
r1 | r2
(1 row)
test=> SHOW is_superuser;
is_superuser
--------------
off
(1 row)
Which has also made me realize that the comment on is_superuser in
guc_tables.c is incorrect:
> /* Not for general use --- used by SET SESSION AUTHORIZATION */
Additionally the C variable name for is_superuser is fairly misleading:
> session_auth_is_superuser
The documentation for this GUC in show.sgml is correct:
> True if the current role has superuser privileges.
As an aside, I'm starting to think we should consider removing this
GUC. It sometimes reports an incorrect value [0], and potentially is
not used internally for anything.
I've rebased my changes over your patch and attached them both.
[0]
https://www.postgresql.org/message-id/CAAvxfHcxH-hLndty6CRThGXL1hLsgCn%2BE3QuG_4Qi7GxrHmgKg%40mail.gmail.com
From 2e1689b5fe384d675043beb9df8eff49a0ff436e Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <kosh...@gmail.com>
Date: Sun, 9 Jul 2023 12:58:41 -0400
Subject: [PATCH 2/2] 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 | 19 +++++++++----------
2 files changed, 10 insertions(+), 11 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 f5548a0f47..1aa393f9fd 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 (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
@@ -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 some of the session user */
static bool SessionUserIsSuperuser = false;
static int SecurityRestrictionContext = 0;
@@ -583,13 +582,13 @@ GetAuthenticatedUserId(void)
}
/*
- * Return whether the authenticated user was superuser at connection start.
+ * Return whether the authenticated user is currently a superuser.
*/
bool
GetAuthenticatedUserIsSuperuser(void)
{
Assert(OidIsValid(AuthenticatedUserId));
- return AuthenticatedUserIsSuperuser;
+ return superuser_arg(AuthenticatedUserId);
}
@@ -741,6 +740,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 +780,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 +816,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 +828,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,7 +851,6 @@ InitializeSessionUserIdStandalone(void)
Assert(!OidIsValid(AuthenticatedUserId));
AuthenticatedUserId = BOOTSTRAP_SUPERUSERID;
- AuthenticatedUserIsSuperuser = true;
SetSessionUserId(BOOTSTRAP_SUPERUSERID, true);
}
--
2.34.1
From e24dfa608e69ebf62f6e94626b9c6f4af49b5524 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Sat, 8 Jul 2023 21:35:03 -0700
Subject: [PATCH 1/2] move session auth permission check
---
src/backend/commands/variable.c | 23 +++++++++++++++++++++++
src/backend/utils/init/miscinit.c | 29 ++++++++++-------------------
src/include/miscadmin.h | 1 +
3 files changed, 34 insertions(+), 19 deletions(-)
diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index f0f2e07655..f8e308f1d0 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -846,6 +846,29 @@ check_session_authorization(char **newval, void **extra, GucSource source)
ReleaseSysCache(roleTup);
+ /*
+ * 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.
+ */
+ 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..f5548a0f47 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
@@ -888,29 +898,10 @@ 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.
*/
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.34.1
