Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 5a2fed9

Browse files
committed
Fix improper interactions between session_authorization and role.
The SQL spec mandates that SET SESSION AUTHORIZATION implies SET ROLE NONE. We tried to implement that within the lowest-level functions that manipulate these settings, but that was a bad idea. In particular, guc.c assumes that it doesn't matter in what order it applies GUC variable updates, but that was not the case for these two variables. This problem, compounded by some hackish attempts to work around it, led to some security-grade issues: * Rolling back a transaction that had done SET SESSION AUTHORIZATION would revert to SET ROLE NONE, even if that had not been the previous state, so that the effective user ID might now be different from what it had been. * The same for SET SESSION AUTHORIZATION in a function SET clause. * If a parallel worker inspected current_setting('role'), it saw "none" even when it should see something else. Also, although the parallel worker startup code intended to cope with the current role's pg_authid row having disappeared, its implementation of that was incomplete so it would still fail. Fix by fully separating the miscinit.c functions that assign session_authorization from those that assign role. To implement the spec's requirement, teach set_config_option itself to perform "SET ROLE NONE" when it sets session_authorization. (This is undoubtedly ugly, but the alternatives seem worse. In particular, there's no way to do it within assign_session_authorization without incompatible changes in the API for GUC assign hooks.) Also, improve ParallelWorkerMain to directly set all the relevant user-ID variables instead of relying on some of them to get set indirectly. That allows us to survive not finding the pg_authid row during worker startup. In v16 and earlier, this includes back-patching 9987a7b which fixed a violation of GUC coding rules: SetSessionAuthorization is not an appropriate place to be throwing errors from. Security: CVE-2024-10978
1 parent cd7ab57 commit 5a2fed9

File tree

7 files changed

+313
-114
lines changed

7 files changed

+313
-114
lines changed

src/backend/access/transam/parallel.c

Lines changed: 27 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -83,12 +83,14 @@ typedef struct FixedParallelState
8383
/* Fixed-size state that workers must restore. */
8484
Oid database_id;
8585
Oid authenticated_user_id;
86-
Oid current_user_id;
86+
Oid session_user_id;
8787
Oid outer_user_id;
88+
Oid current_user_id;
8889
Oid temp_namespace_id;
8990
Oid temp_toast_namespace_id;
9091
int sec_context;
91-
bool is_superuser;
92+
bool session_user_is_superuser;
93+
bool role_is_superuser;
9294
PGPROC *parallel_leader_pgproc;
9395
pid_t parallel_leader_pid;
9496
ProcNumber parallel_leader_proc_number;
@@ -336,9 +338,11 @@ InitializeParallelDSM(ParallelContext *pcxt)
336338
shm_toc_allocate(pcxt->toc, sizeof(FixedParallelState));
337339
fps->database_id = MyDatabaseId;
338340
fps->authenticated_user_id = GetAuthenticatedUserId();
341+
fps->session_user_id = GetSessionUserId();
339342
fps->outer_user_id = GetCurrentRoleId();
340-
fps->is_superuser = current_role_is_superuser;
341343
GetUserIdAndSecContext(&fps->current_user_id, &fps->sec_context);
344+
fps->session_user_is_superuser = GetSessionUserIsSuperuser();
345+
fps->role_is_superuser = current_role_is_superuser;
342346
GetTempNamespaceState(&fps->temp_namespace_id,
343347
&fps->temp_toast_namespace_id);
344348
fps->parallel_leader_pgproc = MyProc;
@@ -1404,6 +1408,17 @@ ParallelWorkerMain(Datum main_arg)
14041408

14051409
entrypt = LookupParallelWorkerFunction(library_name, function_name);
14061410

1411+
/*
1412+
* Restore current session authorization and role id. No verification
1413+
* happens here, we just blindly adopt the leader's state. Note that this
1414+
* has to happen before InitPostgres, since InitializeSessionUserId will
1415+
* not set these variables.
1416+
*/
1417+
SetAuthenticatedUserId(fps->authenticated_user_id);
1418+
SetSessionAuthorization(fps->session_user_id,
1419+
fps->session_user_is_superuser);
1420+
SetCurrentRoleId(fps->outer_user_id, fps->role_is_superuser);
1421+
14071422
/* Restore database connection. */
14081423
BackgroundWorkerInitializeConnectionByOid(fps->database_id,
14091424
fps->authenticated_user_id,
@@ -1483,19 +1498,21 @@ ParallelWorkerMain(Datum main_arg)
14831498
/*
14841499
* Restore GUC values from launching backend. We can't do this earlier,
14851500
* because GUC check hooks that do catalog lookups need to see the same
1486-
* database state as the leader.
1501+
* database state as the leader. Also, the check hooks for
1502+
* session_authorization and role assume we already set the correct role
1503+
* OIDs.
14871504
*/
14881505
gucspace = shm_toc_lookup(toc, PARALLEL_KEY_GUC, false);
14891506
RestoreGUCState(gucspace);
14901507

14911508
/*
1492-
* Restore current role id. Skip verifying whether session user is
1493-
* allowed to become this role and blindly restore the leader's state for
1494-
* current role.
1509+
* Restore current user ID and security context. No verification happens
1510+
* here, we just blindly adopt the leader's state. We can't do this till
1511+
* after restoring GUCs, else we'll get complaints about restoring
1512+
* session_authorization and role. (In effect, we're assuming that all
1513+
* the restored values are okay to set, even if we are now inside a
1514+
* restricted context.)
14951515
*/
1496-
SetCurrentRoleId(fps->outer_user_id, fps->is_superuser);
1497-
1498-
/* Restore user ID and security context. */
14991516
SetUserIdAndSecContext(fps->current_user_id, fps->sec_context);
15001517

15011518
/* Restore temp-namespace state to ensure search path matches leader's. */

src/backend/commands/variable.c

Lines changed: 70 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -822,63 +822,77 @@ check_session_authorization(char **newval, void **extra, GucSource source)
822822
if (*newval == NULL)
823823
return true;
824824

825-
if (!IsTransactionState())
825+
if (InitializingParallelWorker)
826826
{
827827
/*
828-
* Can't do catalog lookups, so fail. The result of this is that
829-
* session_authorization cannot be set in postgresql.conf, which seems
830-
* like a good thing anyway, so we don't work hard to avoid it.
828+
* In parallel worker initialization, we want to copy the leader's
829+
* state even if it no longer matches the catalogs. ParallelWorkerMain
830+
* already installed the correct role OID and superuser state.
831831
*/
832-
return false;
832+
roleid = GetSessionUserId();
833+
is_superuser = GetSessionUserIsSuperuser();
833834
}
835+
else
836+
{
837+
if (!IsTransactionState())
838+
{
839+
/*
840+
* Can't do catalog lookups, so fail. The result of this is that
841+
* session_authorization cannot be set in postgresql.conf, which
842+
* seems like a good thing anyway, so we don't work hard to avoid
843+
* it.
844+
*/
845+
return false;
846+
}
834847

835-
/*
836-
* When source == PGC_S_TEST, we don't throw a hard error for a
837-
* nonexistent user name or insufficient privileges, only a NOTICE. See
838-
* comments in guc.h.
839-
*/
848+
/*
849+
* When source == PGC_S_TEST, we don't throw a hard error for a
850+
* nonexistent user name or insufficient privileges, only a NOTICE.
851+
* See comments in guc.h.
852+
*/
840853

841-
/* Look up the username */
842-
roleTup = SearchSysCache1(AUTHNAME, PointerGetDatum(*newval));
843-
if (!HeapTupleIsValid(roleTup))
844-
{
845-
if (source == PGC_S_TEST)
854+
/* Look up the username */
855+
roleTup = SearchSysCache1(AUTHNAME, PointerGetDatum(*newval));
856+
if (!HeapTupleIsValid(roleTup))
846857
{
847-
ereport(NOTICE,
848-
(errcode(ERRCODE_UNDEFINED_OBJECT),
849-
errmsg("role \"%s\" does not exist", *newval)));
850-
return true;
858+
if (source == PGC_S_TEST)
859+
{
860+
ereport(NOTICE,
861+
(errcode(ERRCODE_UNDEFINED_OBJECT),
862+
errmsg("role \"%s\" does not exist", *newval)));
863+
return true;
864+
}
865+
GUC_check_errmsg("role \"%s\" does not exist", *newval);
866+
return false;
851867
}
852-
GUC_check_errmsg("role \"%s\" does not exist", *newval);
853-
return false;
854-
}
855868

856-
roleform = (Form_pg_authid) GETSTRUCT(roleTup);
857-
roleid = roleform->oid;
858-
is_superuser = roleform->rolsuper;
869+
roleform = (Form_pg_authid) GETSTRUCT(roleTup);
870+
roleid = roleform->oid;
871+
is_superuser = roleform->rolsuper;
859872

860-
ReleaseSysCache(roleTup);
873+
ReleaseSysCache(roleTup);
861874

862-
/*
863-
* Only superusers may SET SESSION AUTHORIZATION a role other than itself.
864-
* Note that in case of multiple SETs in a single session, the original
865-
* authenticated user's superuserness is what matters.
866-
*/
867-
if (roleid != GetAuthenticatedUserId() &&
868-
!superuser_arg(GetAuthenticatedUserId()))
869-
{
870-
if (source == PGC_S_TEST)
875+
/*
876+
* Only superusers may SET SESSION AUTHORIZATION a role other than
877+
* itself. Note that in case of multiple SETs in a single session, the
878+
* original authenticated user's superuserness is what matters.
879+
*/
880+
if (roleid != GetAuthenticatedUserId() &&
881+
!superuser_arg(GetAuthenticatedUserId()))
871882
{
872-
ereport(NOTICE,
873-
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
874-
errmsg("permission will be denied to set session authorization \"%s\"",
875-
*newval)));
876-
return true;
883+
if (source == PGC_S_TEST)
884+
{
885+
ereport(NOTICE,
886+
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
887+
errmsg("permission will be denied to set session authorization \"%s\"",
888+
*newval)));
889+
return true;
890+
}
891+
GUC_check_errcode(ERRCODE_INSUFFICIENT_PRIVILEGE);
892+
GUC_check_errmsg("permission denied to set session authorization \"%s\"",
893+
*newval);
894+
return false;
877895
}
878-
GUC_check_errcode(ERRCODE_INSUFFICIENT_PRIVILEGE);
879-
GUC_check_errmsg("permission denied to set session authorization \"%s\"",
880-
*newval);
881-
return false;
882896
}
883897

884898
/* Set up "extra" struct for assign_session_authorization to use */
@@ -928,6 +942,16 @@ check_role(char **newval, void **extra, GucSource source)
928942
roleid = InvalidOid;
929943
is_superuser = false;
930944
}
945+
else if (InitializingParallelWorker)
946+
{
947+
/*
948+
* In parallel worker initialization, we want to copy the leader's
949+
* state even if it no longer matches the catalogs. ParallelWorkerMain
950+
* already installed the correct role OID and superuser state.
951+
*/
952+
roleid = GetCurrentRoleId();
953+
is_superuser = current_role_is_superuser;
954+
}
931955
else
932956
{
933957
if (!IsTransactionState())
@@ -967,13 +991,8 @@ check_role(char **newval, void **extra, GucSource source)
967991

968992
ReleaseSysCache(roleTup);
969993

970-
/*
971-
* Verify that session user is allowed to become this role, but skip
972-
* this in parallel mode, where we must blindly recreate the parallel
973-
* leader's state.
974-
*/
975-
if (!InitializingParallelWorker &&
976-
!member_can_set_role(GetSessionUserId(), roleid))
994+
/* Verify that session user is allowed to become this role */
995+
if (!member_can_set_role(GetSessionUserId(), roleid))
977996
{
978997
if (source == PGC_S_TEST)
979998
{

0 commit comments

Comments
 (0)