Date: Sat, 8 Jul 2023 21:47:08 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Joseph Koshakow <koshy44@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Preventing non-superusers from altering session authorization
Message-ID: <20230709044708.GA4102497@nathanxps13>
References: <CAAvxfHc-HHzONQ2oXdvhFF9ayRnidPwK+fVBhRzaBWYYLVQL-g@mail.gmail.com>
 <20230622034818.GA1077640@nathanxps13>
 <CAAvxfHdj5H88W99anUoFf4OaH+Oozs=e7kF1mhsegxpWOsvQFQ@mail.gmail.com>
 <20230623175416.GA1268820@nathanxps13>
 <CAAvxfHfSbyHp62bqtovsEKu4M3oCQ-mehfq0OJ7Fk3OqkLwXtw@mail.gmail.com>
 <CAAvxfHfzRhoD98UXKE4DZpR5iwznMiehwsuqMzYhfQcnTwQ0Mw@mail.gmail.com>
 <CAAvxfHdJdAb4QVj5nGv5tz8=QuhvxE4Y4B8yRLDdAEz1doU9jA@mail.gmail.com>
 <20230708220904.GB4044380@nathanxps13>
 <CAAvxfHeUKFE-=biJO15mMD5SNFSqDJy1zX2G-4=tDQkwfaBosg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="45Z9DzgjV8m4Oswq"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAAvxfHeUKFE-=biJO15mMD5SNFSqDJy1zX2G-4=tDQkwfaBosg@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/20230709044708.GA4102497%40nathanxps13>
Precedence: bulk


--45Z9DzgjV8m4Oswq
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On Sat, Jul 08, 2023 at 07:08:35PM -0400, Joseph Koshakow wrote:
> On Sat, Jul 8, 2023 at 6:09 PM Nathan Bossart <nathandbossart@gmail.com>
> wrote:
> 
>>> I think the issue here is that if a session loses the ability to set
>>> their session authorization in the middle of a transaction, then
>>> rolling back the transaction may fail and cause the server to panic.
>>> That's probably what the deleted comment mean when it said:
>>>
>>>> * It's OK because the check does not require catalog access and can't
>>>> * fail during an end-of-transaction GUC reversion
>>
>> Yeah.  IIUC the ERROR longjmps to a block that calls AbortTransaction(),
>> which ERRORs again when resetting the session authorization, which causes
>> us to call AbortTransaction() again, etc., etc.

src/backend/utils/misc/README has the following relevant text:

	Note that there is no provision for a failure result code.  assign_hooks
	should never fail except under the most dire circumstances, since a failure
	may for example result in GUC settings not being rolled back properly during
	transaction abort.  In general, try to do anything that could conceivably
	fail in a check_hook instead, and pass along the results in an "extra"
	struct, so that the assign hook has little to do beyond copying the data to
	someplace.  This applies particularly to catalog lookups: any required
	lookups must be done in the check_hook, since the assign_hook may be
	executed during transaction rollback when lookups will be unsafe.

> Everything seems to work fine if the privilege check is moved to
> check_session_authorization. Which is maybe what the comment meant
> instead of assign_session_authorization.

Ah, that does make more sense.

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?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

--45Z9DzgjV8m4Oswq
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v4-0001-move-session-auth-permission-check.patch"

From 4972f29cef6dfbb948e843517ce5ff413628c2f1 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Sat, 8 Jul 2023 21:35:03 -0700
Subject: [PATCH v4 1/1] 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.25.1


--45Z9DzgjV8m4Oswq--