From: Tom Lane Date: Wed, 9 Dec 2009 21:59:07 +0000 (+0000) Subject: Prevent indirect security attacks via changing session-local state within X-Git-Tag: REL7_4_27~2 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=9d97bb7f2712a1348cca0503ce606e73d14c9a43;p=postgresql Prevent indirect security attacks via changing session-local state within an allegedly immutable index function. It was previously recognized that we had to prevent such a function from executing SET/RESET ROLE/SESSION AUTHORIZATION, or it could trivially obtain the privileges of the session user. However, since there is in general no privilege checking for changes of session-local state, it is also possible for such a function to change settings in a way that might subvert later operations in the same session. Examples include changing search_path to cause an unexpected function to be called, or replacing an existing prepared statement with another one that will execute a function of the attacker's choosing. The present patch secures VACUUM, ANALYZE, and CREATE INDEX/REINDEX against these threats, which are the same places previously deemed to need protection against the SET ROLE issue. GUC changes are still allowed, since there are many useful cases for that, but we prevent security problems by forcing a rollback of any GUC change after completing the operation. Other cases are handled by throwing an error if any change is attempted; these include temp table creation, closing a cursor, and creating or deleting a prepared statement. (In 7.4, the infrastructure to roll back GUC changes doesn't exist, so we settle for rejecting changes of "search_path" in these contexts.) Original report and patch by Gurjeet Singh, additional analysis by Tom Lane. Security: CVE-2009-4136 --- diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 36bfc3f542..90c94fa511 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/access/transam/xact.c,v 1.156.2.5 2008/01/03 21:25:33 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/access/transam/xact.c,v 1.156.2.6 2009/12/09 21:59:06 tgl Exp $ * * NOTES * Transaction aborts can now occur two ways: @@ -851,7 +851,7 @@ static void StartTransaction(void) { TransactionState s = CurrentTransactionState; - bool prevSecDefCxt; + int prevSecContext; /* * check the current transaction state @@ -885,9 +885,9 @@ StartTransaction(void) s->commandId = FirstCommandId; s->startTime = GetCurrentAbsoluteTimeUsec(&(s->startTimeUsec)); - GetUserIdAndContext(&prevUser, &prevSecDefCxt); - /* SecurityDefinerContext should never be set outside a transaction */ - Assert(!prevSecDefCxt); + GetUserIdAndSecContext(&prevUser, &prevSecContext); + /* SecurityRestrictionContext should never be set outside a transaction */ + Assert(prevSecContext == 0); /* * initialize the various transaction subsystems @@ -1084,13 +1084,13 @@ AbortTransaction(void) * Reset user ID which might have been changed transiently. We need this * to clean up in case control escaped out of a SECURITY DEFINER function * or other local change of CurrentUserId; therefore, the prior value - * of SecurityDefinerContext also needs to be restored. + * of SecurityRestrictionContext also needs to be restored. * * (Note: it is not necessary to restore session authorization * setting here because that can only be changed via GUC, and GUC will * take care of rolling it back if need be.) */ - SetUserIdAndContext(prevUser, false); + SetUserIdAndSecContext(prevUser, 0); /* * do abort processing diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 041fdfb70b..4ece0ebf5e 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/catalog/index.c,v 1.219.2.4 2008/05/27 21:14:00 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/catalog/index.c,v 1.219.2.5 2009/12/09 21:59:06 tgl Exp $ * * * INTERFACE ROUTINES @@ -1335,7 +1335,7 @@ index_build(Relation heapRelation, { RegProcedure procedure; AclId save_userid; - bool save_secdefcxt; + int save_sec_context; /* * sanity checks @@ -1347,11 +1347,12 @@ index_build(Relation heapRelation, Assert(RegProcedureIsValid(procedure)); /* - * Switch to the table owner's userid, so that any index functions are - * run as that user. + * Switch to the table owner's userid, so that any index functions are run + * as that user. Also lock down security-restricted operations. */ - GetUserIdAndContext(&save_userid, &save_secdefcxt); - SetUserIdAndContext(heapRelation->rd_rel->relowner, true); + GetUserIdAndSecContext(&save_userid, &save_sec_context); + SetUserIdAndSecContext(heapRelation->rd_rel->relowner, + save_sec_context | SECURITY_RESTRICTED_OPERATION); /* * Call the access method's build procedure @@ -1361,8 +1362,8 @@ index_build(Relation heapRelation, PointerGetDatum(indexRelation), PointerGetDatum(indexInfo)); - /* Restore userid */ - SetUserIdAndContext(save_userid, save_secdefcxt); + /* Restore userid and security context */ + SetUserIdAndSecContext(save_userid, save_sec_context); } diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c index b0f559ab08..4cb4414a9b 100644 --- a/src/backend/commands/analyze.c +++ b/src/backend/commands/analyze.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/commands/analyze.c,v 1.64 2003/10/18 15:38:06 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/commands/analyze.c,v 1.64.2.1 2009/12/09 21:59:06 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -157,6 +157,8 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt) numrows; double totalrows; HeapTuple *rows; + AclId save_userid; + int save_sec_context; if (vacstmt->verbose) elevel = INFO; @@ -294,6 +296,14 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt) return; } + /* + * Switch to the table owner's userid, so that any index functions are run + * as that user. Also lock down security-restricted operations. + */ + GetUserIdAndSecContext(&save_userid, &save_sec_context); + SetUserIdAndSecContext(onerel->rd_rel->relowner, + save_sec_context | SECURITY_RESTRICTED_OPERATION); + /* * Determine how many rows we need to sample, using the worst case * from all analyzable columns. We use a lower bound of 100 rows to @@ -377,6 +387,9 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt) * entries we made in pg_statistic.) */ relation_close(onerel, NoLock); + + /* Restore userid and security context */ + SetUserIdAndSecContext(save_userid, save_sec_context); } /* diff --git a/src/backend/commands/schemacmds.c b/src/backend/commands/schemacmds.c index 162dabf324..1e354e2ead 100644 --- a/src/backend/commands/schemacmds.c +++ b/src/backend/commands/schemacmds.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/commands/schemacmds.c,v 1.16.4.1 2008/01/03 21:25:33 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/commands/schemacmds.c,v 1.16.4.2 2009/12/09 21:59:06 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -46,10 +46,10 @@ CreateSchemaCommand(CreateSchemaStmt *stmt) const char *owner_name; AclId owner_userid; AclId saved_userid; - bool saved_secdefcxt; + int save_sec_context; AclResult aclresult; - GetUserIdAndContext(&saved_userid, &saved_secdefcxt); + GetUserIdAndSecContext(&saved_userid, &save_sec_context); /* * Figure out user identities. @@ -72,7 +72,8 @@ CreateSchemaCommand(CreateSchemaStmt *stmt) * (This will revert to session user on error or at the end of * this routine.) */ - SetUserIdAndContext(owner_userid, true); + SetUserIdAndSecContext(owner_userid, + save_sec_context | SECURITY_LOCAL_USERID_CHANGE); } else { @@ -151,8 +152,8 @@ CreateSchemaCommand(CreateSchemaStmt *stmt) /* Reset search path to normal state */ PopSpecialNamespace(namespaceId); - /* Reset current user */ - SetUserIdAndContext(saved_userid, saved_secdefcxt); + /* Reset current user and security context */ + SetUserIdAndSecContext(saved_userid, save_sec_context); } diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index ccc25c88f4..ab63e6343b 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/commands/tablecmds.c,v 1.91.2.4 2008/05/27 21:14:00 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/commands/tablecmds.c,v 1.91.2.5 2009/12/09 21:59:06 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -150,6 +150,16 @@ DefineRelation(CreateStmt *stmt, char relkind) (errcode(ERRCODE_INVALID_TABLE_DEFINITION), errmsg("ON COMMIT can only be used on temporary tables"))); + /* + * Security check: disallow creating temp tables from security-restricted + * code. This is needed because calling code might not expect untrusted + * tables to appear in pg_temp at the front of its search path. + */ + if (stmt->relation->istemp && InSecurityRestrictedOperation()) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("cannot create temporary table within security-restricted operation"))); + /* * Look up the namespace in which we are supposed to create the * relation. Check we have permission to create there. Skip check if diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index afb2ad7516..34d8510272 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -13,7 +13,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/commands/vacuum.c,v 1.263.2.6 2009/11/10 18:01:46 alvherre Exp $ + * $Header: /cvsroot/pgsql/src/backend/commands/vacuum.c,v 1.263.2.7 2009/12/09 21:59:07 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -743,7 +743,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, char expected_relkind) Oid toast_relid; bool result; AclId save_userid; - bool save_secdefcxt; + int save_sec_context; bool heldoff; /* Begin a transaction for vacuuming this relation */ @@ -850,12 +850,13 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, char expected_relkind) toast_relid = onerel->rd_rel->reltoastrelid; /* - * Switch to the table owner's userid, so that any index functions are - * run as that user. (This is unnecessary, but harmless, for lazy - * VACUUM.) + * Switch to the table owner's userid, so that any index functions are run + * as that user. Also lock down security-restricted operations. + * (This is unnecessary, but harmless, for lazy VACUUM.) */ - GetUserIdAndContext(&save_userid, &save_secdefcxt); - SetUserIdAndContext(onerel->rd_rel->relowner, true); + GetUserIdAndSecContext(&save_userid, &save_sec_context); + SetUserIdAndSecContext(onerel->rd_rel->relowner, + save_sec_context | SECURITY_RESTRICTED_OPERATION); /* * Do the actual work --- either FULL or "lazy" vacuum @@ -867,8 +868,8 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, char expected_relkind) result = true; /* did the vacuum */ - /* Restore userid */ - SetUserIdAndContext(save_userid, save_secdefcxt); + /* Restore userid and security context */ + SetUserIdAndSecContext(save_userid, save_sec_context); /* all done with this class, but hold lock until commit */ relation_close(onerel, NoLock); diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index ac952ca623..9cc846ba6a 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -26,7 +26,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/executor/execMain.c,v 1.220.2.4 2006/01/12 21:49:32 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/executor/execMain.c,v 1.220.2.5 2009/12/09 21:59:07 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -781,6 +781,17 @@ InitPlan(QueryDesc *queryDesc, bool explainOnly) Oid intoRelationId; TupleDesc tupdesc; + /* + * Security check: disallow creating temp tables from + * security-restricted code. This is needed because calling code + * might not expect untrusted tables to appear in pg_temp at the front + * of its search path. + */ + if (parseTree->into->istemp && InSecurityRestrictedOperation()) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("cannot create temporary table within security-restricted operation"))); + /* * find namespace to create in, check permissions */ diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index 577da0cbeb..33f42a06fe 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -10,7 +10,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/tcop/utility.c,v 1.208.2.1 2005/01/24 17:46:41 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/tcop/utility.c,v 1.208.2.2 2009/12/09 21:59:07 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -266,6 +266,25 @@ check_xact_readonly(Node *parsetree) } +/* + * CheckRestrictedOperation: throw error for hazardous command if we're + * inside a security restriction context. + * + * This is needed to protect session-local state for which there is not any + * better-defined protection mechanism, such as ownership. + */ +static void +CheckRestrictedOperation(const char *cmdname) +{ + if (InSecurityRestrictedOperation()) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + /* translator: %s is name of a SQL command, eg PREPARE */ + errmsg("cannot execute %s within security-restricted operation", + cmdname))); +} + + /* * ProcessUtility * general utility function invoker @@ -355,6 +374,7 @@ ProcessUtility(Node *parsetree, { ClosePortalStmt *stmt = (ClosePortalStmt *) parsetree; + CheckRestrictedOperation("CLOSE"); PerformPortalClose(stmt->portalname); } break; @@ -481,6 +501,7 @@ ProcessUtility(Node *parsetree, break; case T_PrepareStmt: + CheckRestrictedOperation("PREPARE"); PrepareQuery((PrepareStmt *) parsetree); break; @@ -489,6 +510,7 @@ ProcessUtility(Node *parsetree, break; case T_DeallocateStmt: + CheckRestrictedOperation("DEALLOCATE"); DeallocateQuery((DeallocateStmt *) parsetree); break; @@ -798,6 +820,7 @@ ProcessUtility(Node *parsetree, { ListenStmt *stmt = (ListenStmt *) parsetree; + CheckRestrictedOperation("LISTEN"); Async_Listen(stmt->relation->relname, MyProcPid); } break; @@ -806,6 +829,7 @@ ProcessUtility(Node *parsetree, { UnlistenStmt *stmt = (UnlistenStmt *) parsetree; + CheckRestrictedOperation("UNLISTEN"); Async_Unlisten(stmt->relation->relname, MyProcPid); } break; diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index 3b5a08ca1c..95f5472b7e 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -17,7 +17,7 @@ * * Portions Copyright (c) 1996-2003, PostgreSQL Global Development Group * - * $Header: /cvsroot/pgsql/src/backend/utils/adt/ri_triggers.c,v 1.63.2.2 2008/01/03 21:25:33 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/utils/adt/ri_triggers.c,v 1.63.2.3 2009/12/09 21:59:07 tgl Exp $ * * ---------- */ @@ -2968,7 +2968,7 @@ ri_PlanCheck(const char *querystr, int nargs, Oid *argtypes, void *qplan; Relation query_rel; AclId save_userid; - bool save_secdefcxt; + int save_sec_context; /* * The query is always run against the FK table except when this is an @@ -2982,8 +2982,9 @@ ri_PlanCheck(const char *querystr, int nargs, Oid *argtypes, query_rel = fk_rel; /* Switch to proper UID to perform check as */ - GetUserIdAndContext(&save_userid, &save_secdefcxt); - SetUserIdAndContext(RelationGetForm(query_rel)->relowner, true); + GetUserIdAndSecContext(&save_userid, &save_sec_context); + SetUserIdAndSecContext(RelationGetForm(query_rel)->relowner, + save_sec_context | SECURITY_LOCAL_USERID_CHANGE); /* Create the plan */ qplan = SPI_prepare(querystr, nargs, argtypes); @@ -2991,8 +2992,8 @@ ri_PlanCheck(const char *querystr, int nargs, Oid *argtypes, if (qplan == NULL) elog(ERROR, "SPI_prepare returned %d for %s", SPI_result, querystr); - /* Restore UID */ - SetUserIdAndContext(save_userid, save_secdefcxt); + /* Restore UID and security context */ + SetUserIdAndSecContext(save_userid, save_sec_context); /* Save the plan if requested */ if (cache_plan) @@ -3021,7 +3022,7 @@ ri_PerformCheck(RI_QueryKey *qkey, void *qplan, int limit; int spi_result; AclId save_userid; - bool save_secdefcxt; + int save_sec_context; Datum vals[RI_MAX_NUMKEYS * 2]; char nulls[RI_MAX_NUMKEYS * 2]; @@ -3097,15 +3098,16 @@ ri_PerformCheck(RI_QueryKey *qkey, void *qplan, limit = (expect_OK == SPI_OK_SELECT) ? 1 : 0; /* Switch to proper UID to perform check as */ - GetUserIdAndContext(&save_userid, &save_secdefcxt); - SetUserIdAndContext(RelationGetForm(query_rel)->relowner, true); + GetUserIdAndSecContext(&save_userid, &save_sec_context); + SetUserIdAndSecContext(RelationGetForm(query_rel)->relowner, + save_sec_context | SECURITY_LOCAL_USERID_CHANGE); /* Finally we can run the query. */ spi_result = SPI_execp_current(qplan, vals, nulls, useCurrentSnapshot, limit); - /* Restore UID */ - SetUserIdAndContext(save_userid, save_secdefcxt); + /* Restore UID and security context */ + SetUserIdAndSecContext(save_userid, save_sec_context); /* Check result */ if (spi_result < 0) diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c index 0ae4c621f3..0180fbda0c 100644 --- a/src/backend/utils/fmgr/fmgr.c +++ b/src/backend/utils/fmgr/fmgr.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/utils/fmgr/fmgr.c,v 1.76.2.2 2008/01/03 21:25:33 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/utils/fmgr/fmgr.c,v 1.76.2.3 2009/12/09 21:59:07 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -654,7 +654,7 @@ fmgr_security_definer(PG_FUNCTION_ARGS) FmgrInfo *save_flinfo; struct fmgr_security_definer_cache *fcache; AclId save_userid; - bool save_secdefcxt; + int save_sec_context; HeapTuple tuple; if (!fcinfo->flinfo->fn_extra) @@ -680,8 +680,9 @@ fmgr_security_definer(PG_FUNCTION_ARGS) else fcache = fcinfo->flinfo->fn_extra; - GetUserIdAndContext(&save_userid, &save_secdefcxt); - SetUserIdAndContext(fcache->userid, true); + GetUserIdAndSecContext(&save_userid, &save_sec_context); + SetUserIdAndSecContext(fcache->userid, + save_sec_context | SECURITY_LOCAL_USERID_CHANGE); save_flinfo = fcinfo->flinfo; fcinfo->flinfo = &fcache->flinfo; @@ -690,7 +691,7 @@ fmgr_security_definer(PG_FUNCTION_ARGS) fcinfo->flinfo = save_flinfo; - SetUserIdAndContext(save_userid, save_secdefcxt); + SetUserIdAndSecContext(save_userid, save_sec_context); return result; } diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c index dc8ae22851..614f0a205f 100644 --- a/src/backend/utils/init/miscinit.c +++ b/src/backend/utils/init/miscinit.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/utils/init/miscinit.c,v 1.116.2.1 2008/01/03 21:25:33 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/utils/init/miscinit.c,v 1.116.2.2 2009/12/09 21:59:07 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -236,8 +236,10 @@ SetDataDir(const char *dir) * is the session user. You are yourself responsible to save and * restore the current user id if you need to change it. * - * SecurityDefinerContext is TRUE if we are within a SECURITY DEFINER function - * or another context that temporarily changes CurrentUserId. + * SecurityRestrictionContext holds flags indicating reason(s) for changing + * CurrentUserId. In some cases we need to lock down operations that are + * not directly controlled by privilege settings, and this provides a + * convenient way to do it. * ---------------------------------------------------------------- */ static AclId AuthenticatedUserId = 0; @@ -246,13 +248,13 @@ static AclId CurrentUserId = 0; static bool AuthenticatedUserIsSuperuser = false; -static bool SecurityDefinerContext = false; +static int SecurityRestrictionContext = 0; /* * GetUserId - get the current effective user ID. * - * Note: there's no SetUserId() anymore; use SetUserIdAndContext(). + * Note: there's no SetUserId() anymore; use SetUserIdAndSecContext(). */ AclId GetUserId(void) @@ -276,7 +278,7 @@ GetSessionUserId(void) static void SetSessionUserId(AclId newid) { - AssertState(!SecurityDefinerContext); + AssertState(SecurityRestrictionContext == 0); AssertArg(AclIdIsValid(newid)); SessionUserId = newid; CurrentUserId = newid; @@ -284,11 +286,29 @@ SetSessionUserId(AclId newid) /* - * GetUserIdAndContext/SetUserIdAndContext - get/set the current user ID - * and the SecurityDefinerContext flag. + * GetUserIdAndSecContext/SetUserIdAndSecContext - get/set the current user ID + * and the SecurityRestrictionContext flags. * - * Unlike GetUserId, GetUserIdAndContext does *not* Assert that the current - * value of CurrentUserId is valid; nor does SetUserIdAndContext require + * Currently there are two valid bits in SecurityRestrictionContext: + * + * SECURITY_LOCAL_USERID_CHANGE indicates that we are inside an operation + * that is temporarily changing CurrentUserId via these functions. This is + * needed to indicate that the actual value of CurrentUserId is not in sync + * with guc.c's internal state, so SET ROLE has to be disallowed. + * + * SECURITY_RESTRICTED_OPERATION indicates that we are inside an operation + * that does not wish to trust called user-defined functions at all. This + * bit prevents not only SET ROLE, but various other changes of session state + * that normally is unprotected but might possibly be used to subvert the + * calling session later. An example is replacing an existing prepared + * statement with new code, which will then be executed with the outer + * session's permissions when the prepared statement is next used. Since + * these restrictions are fairly draconian, we apply them only in contexts + * where the called functions are really supposed to be side-effect-free + * anyway, such as VACUUM/ANALYZE/REINDEX. + * + * Unlike GetUserId, GetUserIdAndSecContext does *not* Assert that the current + * value of CurrentUserId is valid; nor does SetUserIdAndSecContext require * the new value to be valid. In fact, these routines had better not * ever throw any kind of error. This is because they are used by * StartTransaction and AbortTransaction to save/restore the settings, @@ -297,27 +317,66 @@ SetSessionUserId(AclId newid) * through AbortTransaction without asserting in case InitPostgres fails. */ void -GetUserIdAndContext(AclId *userid, bool *sec_def_context) +GetUserIdAndSecContext(AclId *userid, int *sec_context) { *userid = CurrentUserId; - *sec_def_context = SecurityDefinerContext; + *sec_context = SecurityRestrictionContext; } void -SetUserIdAndContext(AclId userid, bool sec_def_context) +SetUserIdAndSecContext(AclId userid, int sec_context) { CurrentUserId = userid; - SecurityDefinerContext = sec_def_context; + SecurityRestrictionContext = sec_context; } /* - * InSecurityDefinerContext - are we inside a SECURITY DEFINER context? + * InLocalUserIdChange - are we inside a local change of CurrentUserId? */ bool -InSecurityDefinerContext(void) +InLocalUserIdChange(void) { - return SecurityDefinerContext; + return (SecurityRestrictionContext & SECURITY_LOCAL_USERID_CHANGE) != 0; +} + +/* + * InSecurityRestrictedOperation - are we inside a security-restricted command? + */ +bool +InSecurityRestrictedOperation(void) +{ + return (SecurityRestrictionContext & SECURITY_RESTRICTED_OPERATION) != 0; +} + + +/* + * These are obsolete versions of Get/SetUserIdAndSecContext that are + * only provided for bug-compatibility with some rather dubious code in + * pljava. We allow the userid to be set, but only when not inside a + * security restriction context. + */ +void +GetUserIdAndContext(AclId *userid, bool *sec_def_context) +{ + *userid = CurrentUserId; + *sec_def_context = InLocalUserIdChange(); +} + +void +SetUserIdAndContext(AclId userid, bool sec_def_context) +{ + /* We throw the same error SET ROLE would. */ + if (InSecurityRestrictedOperation()) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("cannot set parameter \"%s\" within security-restricted operation", + "role"))); + CurrentUserId = userid; + if (sec_def_context) + SecurityRestrictionContext |= SECURITY_LOCAL_USERID_CHANGE; + else + SecurityRestrictionContext &= ~SECURITY_LOCAL_USERID_CHANGE; } diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 5e72d57891..d5da9b77a1 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -10,7 +10,7 @@ * Written by Peter Eisentraut . * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/utils/misc/guc.c,v 1.164.2.6 2009/09/03 22:09:05 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/utils/misc/guc.c,v 1.164.2.7 2009/12/09 21:59:07 tgl Exp $ * *-------------------------------------------------------------------- */ @@ -1478,7 +1478,7 @@ static struct config_string ConfigureNamesString[] = {"search_path", PGC_USERSET, CLIENT_CONN_STATEMENT, gettext_noop("Sets the schema search order for names that are not schema-qualified."), NULL, - GUC_LIST_INPUT | GUC_LIST_QUOTE + GUC_LIST_INPUT | GUC_LIST_QUOTE | GUC_NOT_WHILE_SEC_REST }, &namespace_search_path, "$user,public", assign_search_path, NULL @@ -1511,7 +1511,7 @@ static struct config_string ConfigureNamesString[] = {"session_authorization", PGC_USERSET, UNGROUPED, gettext_noop("Shows the session user name."), NULL, - GUC_IS_NAME | GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_NOT_WHILE_SEC_DEF + GUC_IS_NAME | GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_NOT_WHILE_SEC_DEF | GUC_NOT_WHILE_SEC_REST }, &session_authorization_string, NULL, assign_session_authorization, show_session_authorization @@ -2515,30 +2515,33 @@ set_config_option(const char *name, const char *value, } /* - * Disallow changing GUC_NOT_WHILE_SEC_DEF values if we are inside a - * security-definer function. We can reject this regardless of - * the context or source, mainly because sources that it might be + * Disallow changing GUC_NOT_WHILE_SEC_DEF/REST values if we are inside a + * security restriction context. We can reject this regardless of + * the GUC context or source, mainly because sources that it might be * reasonable to override for won't be seen while inside a function. - * - * Note: variables marked GUC_NOT_WHILE_SEC_DEF should probably be marked - * GUC_NO_RESET_ALL as well, because ResetAllOptions() doesn't check this. - * - * Note: this flag is currently used for "session_authorization". - * We need to prohibit this because when we exit the sec-def - * context, GUC won't be notified, leaving things out of sync. - * - * XXX it would be nice to allow these cases in future, with the behavior - * being that the SET's effects end when the security definer context is - * exited. */ - if ((record->flags & GUC_NOT_WHILE_SEC_DEF) && InSecurityDefinerContext()) - { - ereport(elevel, - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("cannot set parameter \"%s\" within security-definer function", - name))); - return false; - } + if (record->flags & GUC_NOT_WHILE_SEC_DEF && + InLocalUserIdChange()) + { + /* + * Phrasing of this error message is historical, but it's the + * most common case. + */ + ereport(elevel, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("cannot set parameter \"%s\" within security-definer function", + name))); + return false; + } + if (record->flags & GUC_NOT_WHILE_SEC_REST && + InSecurityRestrictedOperation()) + { + ereport(elevel, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("cannot set parameter \"%s\" within security-restricted operation", + name))); + return false; + } /* Should we report errors interactively? */ interactive = (source >= PGC_S_SESSION); diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index d9e2e027d8..b0ec831ec1 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -12,7 +12,7 @@ * Portions Copyright (c) 1996-2003, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $Id: miscadmin.h,v 1.134.2.1 2008/01/03 21:25:34 tgl Exp $ + * $Id: miscadmin.h,v 1.134.2.2 2009/12/09 21:59:07 tgl Exp $ * * NOTES * some of the information in this file should be moved to @@ -209,6 +209,10 @@ extern char *VirtualHost; * POSTGRES directory path definitions. * *****************************************************************************/ +/* flags to be OR'd to form sec_context */ +#define SECURITY_LOCAL_USERID_CHANGE 0x0001 +#define SECURITY_RESTRICTED_OPERATION 0x0002 + extern char *DatabasePath; /* in utils/misc/database.c */ @@ -221,9 +225,12 @@ extern void SetDatabasePath(const char *path); extern char *GetUserNameFromId(AclId userid); extern AclId GetUserId(void); extern AclId GetSessionUserId(void); +extern void GetUserIdAndSecContext(AclId *userid, int *sec_context); +extern void SetUserIdAndSecContext(AclId userid, int sec_context); +extern bool InLocalUserIdChange(void); +extern bool InSecurityRestrictedOperation(void); extern void GetUserIdAndContext(AclId *userid, bool *sec_def_context); extern void SetUserIdAndContext(AclId userid, bool sec_def_context); -extern bool InSecurityDefinerContext(void); extern void InitializeSessionUserId(const char *username); extern void InitializeSessionUserIdStandalone(void); extern void SetSessionAuthorization(AclId userid, bool is_superuser); diff --git a/src/include/utils/guc_tables.h b/src/include/utils/guc_tables.h index d04dc7a41d..fa45f9885d 100644 --- a/src/include/utils/guc_tables.h +++ b/src/include/utils/guc_tables.h @@ -7,7 +7,7 @@ * * Portions Copyright (c) 1996-2003, PostgreSQL Global Development Group * - * $Id: guc_tables.h,v 1.6.4.2 2009/09/03 22:09:06 tgl Exp $ + * $Id: guc_tables.h,v 1.6.4.3 2009/12/09 21:59:07 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -99,7 +99,8 @@ struct config_generic #define GUC_DISALLOW_IN_FILE 0x0040 /* can't set in postgresql.conf */ #define GUC_IS_NAME 0x0080 /* limit string to NAMEDATALEN-1 */ -#define GUC_NOT_WHILE_SEC_DEF 0x8000 /* can't change inside sec-def func */ +#define GUC_NOT_WHILE_SEC_DEF 0x4000 /* can't set in sec-def function */ +#define GUC_NOT_WHILE_SEC_REST 0x8000 /* can't set if security restricted */ /* bit values in status field */ #define GUC_HAVE_TENTATIVE 0x0001 /* tentative value is defined */