From: Tom Lane Date: Thu, 3 Jan 2008 21:25:34 +0000 (+0000) Subject: Make standard maintenance operations (including VACUUM, ANALYZE, REINDEX, X-Git-Tag: REL7_4_19~3 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=230d5cfc4739bc4963c8d67e4a7cd84abe53ef93;p=postgresql Make standard maintenance operations (including VACUUM, ANALYZE, REINDEX, and CLUSTER) execute as the table owner rather than the calling user, using the same privilege-switching mechanism already used for SECURITY DEFINER functions. The purpose of this change is to ensure that user-defined functions used in index definitions cannot acquire the privileges of a superuser account that is performing routine maintenance. While a function used in an index is supposed to be IMMUTABLE and thus not able to do anything very interesting, there are several easy ways around that restriction; and even if we could plug them all, there would remain a risk of reading sensitive information and broadcasting it through a covert channel such as CPU usage. To prevent bypassing this security measure, execution of SET SESSION AUTHORIZATION and SET ROLE is now forbidden within a SECURITY DEFINER context. Thanks to Itagaki Takahiro for reporting this vulnerability. Security: CVE-2007-6600 --- diff --git a/doc/src/sgml/ref/set_session_auth.sgml b/doc/src/sgml/ref/set_session_auth.sgml index 41c88a9f95..b6da266bd6 100644 --- a/doc/src/sgml/ref/set_session_auth.sgml +++ b/doc/src/sgml/ref/set_session_auth.sgml @@ -1,4 +1,4 @@ - + SET SESSION AUTHORIZATION @@ -27,7 +27,7 @@ RESET SESSION AUTHORIZATION This command sets the session user identifier and the current user - identifier of the current SQL-session context to be username. The user name may be written as either an identifier or a string literal. Using this command, it is possible, for example, to temporarily become an @@ -38,7 +38,7 @@ RESET SESSION AUTHORIZATION The session user identifier is initially set to be the (possibly authenticated) user name provided by the client. The current user identifier is normally equal to the session user identifier, but - may change temporarily in the context of setuid + might change temporarily in the context of SECURITY DEFINER functions and similar mechanisms. The current user identifier is relevant for permission checking. @@ -63,6 +63,15 @@ RESET SESSION AUTHORIZATION + + Notes + + + SET SESSION AUTHORIZATION cannot be used within a + SECURITY DEFINER function. + + + Examples diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index c8a68b5fb3..36bfc3f542 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.4 2007/04/26 23:25:40 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/access/transam/xact.c,v 1.156.2.5 2008/01/03 21:25:33 tgl Exp $ * * NOTES * Transaction aborts can now occur two ways: @@ -204,6 +204,8 @@ static TransactionStateData CurrentTransactionStateData = { static TransactionState CurrentTransactionState = &CurrentTransactionStateData; +static AclId prevUser; /* CurrentUserId at transaction start */ + /* * User-tweakable parameters */ @@ -849,6 +851,7 @@ static void StartTransaction(void) { TransactionState s = CurrentTransactionState; + bool prevSecDefCxt; /* * check the current transaction state @@ -882,6 +885,10 @@ StartTransaction(void) s->commandId = FirstCommandId; s->startTime = GetCurrentAbsoluteTimeUsec(&(s->startTimeUsec)); + GetUserIdAndContext(&prevUser, &prevSecDefCxt); + /* SecurityDefinerContext should never be set outside a transaction */ + Assert(!prevSecDefCxt); + /* * initialize the various transaction subsystems */ @@ -1074,9 +1081,16 @@ AbortTransaction(void) AtAbort_Memory(); /* - * Reset user id which might have been changed transiently + * 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. + * + * (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.) */ - SetUserId(GetSessionUserId()); + SetUserIdAndContext(prevUser, false); /* * do abort processing diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 073f06b064..3d782d2e5f 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.2 2007/11/08 23:23:23 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/catalog/index.c,v 1.219.2.3 2008/01/03 21:25:33 tgl Exp $ * * * INTERFACE ROUTINES @@ -1332,6 +1332,8 @@ index_build(Relation heapRelation, IndexInfo *indexInfo) { RegProcedure procedure; + AclId save_userid; + bool save_secdefcxt; /* * sanity checks @@ -1342,6 +1344,13 @@ index_build(Relation heapRelation, procedure = indexRelation->rd_am->ambuild; Assert(RegProcedureIsValid(procedure)); + /* + * Switch to the table owner's userid, so that any index functions are + * run as that user. + */ + GetUserIdAndContext(&save_userid, &save_secdefcxt); + SetUserIdAndContext(heapRelation->rd_rel->relowner, true); + /* * Call the access method's build procedure */ @@ -1349,6 +1358,9 @@ index_build(Relation heapRelation, PointerGetDatum(heapRelation), PointerGetDatum(indexRelation), PointerGetDatum(indexInfo)); + + /* Restore userid */ + SetUserIdAndContext(save_userid, save_secdefcxt); } diff --git a/src/backend/commands/schemacmds.c b/src/backend/commands/schemacmds.c index 2d184727a9..162dabf324 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 2003/08/04 02:39:58 momjian Exp $ + * $Header: /cvsroot/pgsql/src/backend/commands/schemacmds.c,v 1.16.4.1 2008/01/03 21:25:33 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -46,9 +46,10 @@ CreateSchemaCommand(CreateSchemaStmt *stmt) const char *owner_name; AclId owner_userid; AclId saved_userid; + bool saved_secdefcxt; AclResult aclresult; - saved_userid = GetUserId(); + GetUserIdAndContext(&saved_userid, &saved_secdefcxt); /* * Figure out user identities. @@ -71,7 +72,7 @@ CreateSchemaCommand(CreateSchemaStmt *stmt) * (This will revert to session user on error or at the end of * this routine.) */ - SetUserId(owner_userid); + SetUserIdAndContext(owner_userid, true); } else { @@ -151,7 +152,7 @@ CreateSchemaCommand(CreateSchemaStmt *stmt) PopSpecialNamespace(namespaceId); /* Reset current user */ - SetUserId(saved_userid); + SetUserIdAndContext(saved_userid, saved_secdefcxt); } diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index d3b832ad4a..7ce960b3c7 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.3 2007/03/14 18:49:26 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/commands/vacuum.c,v 1.263.2.4 2008/01/03 21:25:33 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -742,6 +742,8 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, char expected_relkind) LockRelId onerelid; Oid toast_relid; bool result; + AclId save_userid; + bool save_secdefcxt; /* Begin a transaction for vacuuming this relation */ StartTransactionCommand(); @@ -846,6 +848,14 @@ 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.) + */ + GetUserIdAndContext(&save_userid, &save_secdefcxt); + SetUserIdAndContext(onerel->rd_rel->relowner, true); + /* * Do the actual work --- either FULL or "lazy" vacuum */ @@ -856,6 +866,9 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, char expected_relkind) result = true; /* did the vacuum */ + /* Restore userid */ + SetUserIdAndContext(save_userid, save_secdefcxt); + /* all done with this class, but hold lock until commit */ relation_close(onerel, NoLock); diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c index d88f8059b7..6a856170d6 100644 --- a/src/backend/commands/variable.c +++ b/src/backend/commands/variable.c @@ -9,7 +9,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/commands/variable.c,v 1.88.2.3 2006/02/12 22:33:28 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/commands/variable.c,v 1.88.2.4 2008/01/03 21:25:33 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -773,6 +773,22 @@ assign_session_authorization(const char *value, bool doit, bool interactive) /* not a saved ID, so look it up */ HeapTuple userTup; + if (InSecurityDefinerContext()) + { + /* + * Disallow SET SESSION AUTHORIZATION inside a security definer + * context. We need to do this because when we exit the context, + * GUC won't be notified, leaving things out of sync. Note that + * this test is positioned so that restoring a previously saved + * setting isn't prevented. + */ + if (interactive) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot set session authorization within security-definer function"))); + return NULL; + } + if (!IsTransactionState()) { /* diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index 2164738b17..3b5a08ca1c 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.1 2004/10/13 22:22:03 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/utils/adt/ri_triggers.c,v 1.63.2.2 2008/01/03 21:25:33 tgl Exp $ * * ---------- */ @@ -2967,7 +2967,8 @@ ri_PlanCheck(const char *querystr, int nargs, Oid *argtypes, { void *qplan; Relation query_rel; - AclId save_uid; + AclId save_userid; + bool save_secdefcxt; /* * The query is always run against the FK table except when this is an @@ -2981,8 +2982,8 @@ ri_PlanCheck(const char *querystr, int nargs, Oid *argtypes, query_rel = fk_rel; /* Switch to proper UID to perform check as */ - save_uid = GetUserId(); - SetUserId(RelationGetForm(query_rel)->relowner); + GetUserIdAndContext(&save_userid, &save_secdefcxt); + SetUserIdAndContext(RelationGetForm(query_rel)->relowner, true); /* Create the plan */ qplan = SPI_prepare(querystr, nargs, argtypes); @@ -2991,7 +2992,7 @@ ri_PlanCheck(const char *querystr, int nargs, Oid *argtypes, elog(ERROR, "SPI_prepare returned %d for %s", SPI_result, querystr); /* Restore UID */ - SetUserId(save_uid); + SetUserIdAndContext(save_userid, save_secdefcxt); /* Save the plan if requested */ if (cache_plan) @@ -3019,7 +3020,8 @@ ri_PerformCheck(RI_QueryKey *qkey, void *qplan, bool useCurrentSnapshot; int limit; int spi_result; - AclId save_uid; + AclId save_userid; + bool save_secdefcxt; Datum vals[RI_MAX_NUMKEYS * 2]; char nulls[RI_MAX_NUMKEYS * 2]; @@ -3095,15 +3097,15 @@ ri_PerformCheck(RI_QueryKey *qkey, void *qplan, limit = (expect_OK == SPI_OK_SELECT) ? 1 : 0; /* Switch to proper UID to perform check as */ - save_uid = GetUserId(); - SetUserId(RelationGetForm(query_rel)->relowner); + GetUserIdAndContext(&save_userid, &save_secdefcxt); + SetUserIdAndContext(RelationGetForm(query_rel)->relowner, true); /* Finally we can run the query. */ spi_result = SPI_execp_current(qplan, vals, nulls, useCurrentSnapshot, limit); /* Restore UID */ - SetUserId(save_uid); + SetUserIdAndContext(save_userid, save_secdefcxt); /* Check result */ if (spi_result < 0) diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c index 2d5d8b3817..0ae4c621f3 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.1 2007/07/31 15:50:17 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/utils/fmgr/fmgr.c,v 1.76.2.2 2008/01/03 21:25:33 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -654,6 +654,7 @@ fmgr_security_definer(PG_FUNCTION_ARGS) FmgrInfo *save_flinfo; struct fmgr_security_definer_cache *fcache; AclId save_userid; + bool save_secdefcxt; HeapTuple tuple; if (!fcinfo->flinfo->fn_extra) @@ -679,16 +680,18 @@ fmgr_security_definer(PG_FUNCTION_ARGS) else fcache = fcinfo->flinfo->fn_extra; + GetUserIdAndContext(&save_userid, &save_secdefcxt); + SetUserIdAndContext(fcache->userid, true); + save_flinfo = fcinfo->flinfo; fcinfo->flinfo = &fcache->flinfo; - save_userid = GetUserId(); - SetUserId(fcache->userid); result = FunctionCallInvoke(fcinfo); - SetUserId(save_userid); fcinfo->flinfo = save_flinfo; + SetUserIdAndContext(save_userid, save_secdefcxt); + return result; } diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c index 2b9ab06618..dc8ae22851 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 2003/09/26 15:27:37 petere Exp $ + * $Header: /cvsroot/pgsql/src/backend/utils/init/miscinit.c,v 1.116.2.1 2008/01/03 21:25:33 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -235,6 +235,9 @@ SetDataDir(const char *dir) * are implemented. Conceptually there is a stack, whose bottom * 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. * ---------------------------------------------------------------- */ static AclId AuthenticatedUserId = 0; @@ -243,8 +246,13 @@ static AclId CurrentUserId = 0; static bool AuthenticatedUserIsSuperuser = false; +static bool SecurityDefinerContext = false; + + /* - * This function is relevant for all privilege checks. + * GetUserId - get the current effective user ID. + * + * Note: there's no SetUserId() anymore; use SetUserIdAndContext(). */ AclId GetUserId(void) @@ -254,14 +262,6 @@ GetUserId(void) } -void -SetUserId(AclId newid) -{ - AssertArg(AclIdIsValid(newid)); - CurrentUserId = newid; -} - - /* * This value is only relevant for informational purposes. */ @@ -273,17 +273,57 @@ GetSessionUserId(void) } -void +static void SetSessionUserId(AclId newid) { + AssertState(!SecurityDefinerContext); AssertArg(AclIdIsValid(newid)); SessionUserId = newid; - /* Current user defaults to session user. */ - if (!AclIdIsValid(CurrentUserId)) - CurrentUserId = newid; + CurrentUserId = newid; } +/* + * GetUserIdAndContext/SetUserIdAndContext - get/set the current user ID + * and the SecurityDefinerContext flag. + * + * Unlike GetUserId, GetUserIdAndContext does *not* Assert that the current + * value of CurrentUserId is valid; nor does SetUserIdAndContext 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, + * and during the first transaction within a backend, the value to be saved + * and perhaps restored is indeed invalid. We have to be able to get + * through AbortTransaction without asserting in case InitPostgres fails. + */ +void +GetUserIdAndContext(AclId *userid, bool *sec_def_context) +{ + *userid = CurrentUserId; + *sec_def_context = SecurityDefinerContext; +} + +void +SetUserIdAndContext(AclId userid, bool sec_def_context) +{ + CurrentUserId = userid; + SecurityDefinerContext = sec_def_context; +} + + +/* + * InSecurityDefinerContext - are we inside a SECURITY DEFINER context? + */ +bool +InSecurityDefinerContext(void) +{ + return SecurityDefinerContext; +} + + +/* + * Initialize user identity during normal backend startup + */ void InitializeSessionUserId(const char *username) { @@ -378,7 +418,6 @@ SetSessionAuthorization(AclId userid, bool is_superuser) errmsg("permission denied to set session authorization"))); SetSessionUserId(userid); - SetUserId(userid); SetConfigOption("is_superuser", is_superuser ? "on" : "off", diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index 1c1c1d3451..d9e2e027d8 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 2003/09/24 18:54:01 tgl Exp $ + * $Id: miscadmin.h,v 1.134.2.1 2008/01/03 21:25:34 tgl Exp $ * * NOTES * some of the information in this file should be moved to @@ -220,9 +220,10 @@ extern void SetDatabasePath(const char *path); extern char *GetUserNameFromId(AclId userid); extern AclId GetUserId(void); -extern void SetUserId(AclId userid); extern AclId GetSessionUserId(void); -extern void SetSessionUserId(AclId userid); +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);