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

4 files changed, 132 insertions, 54 deletions

diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 05e840dae5f..ec0eeddc263 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -15,7 +15,7 @@
  *
  * Portions Copyright (c) 1996-2009, PostgreSQL Global Development Group
  *
- * $PostgreSQL: pgsql/src/backend/utils/adt/ri_triggers.c,v 1.113.2.1 2009/11/05 04:38:35 tgl Exp $
+ * $PostgreSQL: pgsql/src/backend/utils/adt/ri_triggers.c,v 1.113.2.2 2009/12/09 21:58:04 tgl Exp $
  *
  * ----------
  */
@@ -3209,7 +3209,7 @@ ri_PlanCheck(const char *querystr, int nargs, Oid *argtypes,
 	SPIPlanPtr	qplan;
 	Relation	query_rel;
 	Oid			save_userid;
-	bool		save_secdefcxt;
+	int			save_sec_context;
 
 	/*
 	 * The query is always run against the FK table except when this is an
@@ -3223,8 +3223,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);
@@ -3232,8 +3233,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)
@@ -3263,7 +3264,7 @@ ri_PerformCheck(RI_QueryKey *qkey, SPIPlanPtr qplan,
 	int			limit;
 	int			spi_result;
 	Oid			save_userid;
-	bool		save_secdefcxt;
+	int			save_sec_context;
 	Datum		vals[RI_MAX_NUMKEYS * 2];
 	char		nulls[RI_MAX_NUMKEYS * 2];
 
@@ -3343,8 +3344,9 @@ ri_PerformCheck(RI_QueryKey *qkey, SPIPlanPtr 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_execute_snapshot(qplan,
@@ -3352,8 +3354,8 @@ ri_PerformCheck(RI_QueryKey *qkey, SPIPlanPtr qplan,
 									  test_snapshot, crosscheck_snapshot,
 									  false, false, 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 1256e7c88d0..308d7a0a239 100644
--- a/src/backend/utils/fmgr/fmgr.c
+++ b/src/backend/utils/fmgr/fmgr.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/utils/fmgr/fmgr.c,v 1.126 2009/06/11 14:49:05 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/utils/fmgr/fmgr.c,v 1.126.2.1 2009/12/09 21:58:04 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -880,7 +880,7 @@ fmgr_security_definer(PG_FUNCTION_ARGS)
 	struct fmgr_security_definer_cache *volatile fcache;
 	FmgrInfo   *save_flinfo;
 	Oid			save_userid;
-	bool		save_secdefcxt;
+	int			save_sec_context;
 	volatile int save_nestlevel;
 	PgStat_FunctionCallUsage fcusage;
 
@@ -926,15 +926,16 @@ fmgr_security_definer(PG_FUNCTION_ARGS)
 	else
 		fcache = fcinfo->flinfo->fn_extra;
 
-	/* GetUserIdAndContext is cheap enough that no harm in a wasted call */
-	GetUserIdAndContext(&save_userid, &save_secdefcxt);
+	/* GetUserIdAndSecContext is cheap enough that no harm in a wasted call */
+	GetUserIdAndSecContext(&save_userid, &save_sec_context);
 	if (fcache->proconfig)		/* Need a new GUC nesting level */
 		save_nestlevel = NewGUCNestLevel();
 	else
 		save_nestlevel = 0;		/* keep compiler quiet */
 
 	if (OidIsValid(fcache->userid))
-		SetUserIdAndContext(fcache->userid, true);
+		SetUserIdAndSecContext(fcache->userid,
+							   save_sec_context | SECURITY_LOCAL_USERID_CHANGE);
 
 	if (fcache->proconfig)
 	{
@@ -981,7 +982,7 @@ fmgr_security_definer(PG_FUNCTION_ARGS)
 	if (fcache->proconfig)
 		AtEOXact_GUC(true, save_nestlevel);
 	if (OidIsValid(fcache->userid))
-		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 7bdfb67204e..55b395f9a2b 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/utils/init/miscinit.c,v 1.175 2009/06/11 14:49:05 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/utils/init/miscinit.c,v 1.175.2.1 2009/12/09 21:58:04 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -272,8 +272,10 @@ make_absolute_path(const char *path)
  * be the same as OuterUserId, but it changes during calls to SECURITY
  * DEFINER functions, as well as locally in some specialized commands.
  *
- * 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 Oid	AuthenticatedUserId = InvalidOid;
@@ -285,7 +287,7 @@ static Oid	CurrentUserId = InvalidOid;
 static bool AuthenticatedUserIsSuperuser = false;
 static bool SessionUserIsSuperuser = false;
 
-static bool SecurityDefinerContext = false;
+static int	SecurityRestrictionContext = 0;
 
 /* We also remember if a SET ROLE is currently active */
 static bool SetRoleIsActive = false;
@@ -294,7 +296,7 @@ static bool SetRoleIsActive = false;
 /*
  * GetUserId - get the current effective user ID.
  *
- * Note: there's no SetUserId() anymore; use SetUserIdAndContext().
+ * Note: there's no SetUserId() anymore; use SetUserIdAndSecContext().
  */
 Oid
 GetUserId(void)
@@ -318,7 +320,7 @@ GetOuterUserId(void)
 static void
 SetOuterUserId(Oid userid)
 {
-	AssertState(!SecurityDefinerContext);
+	AssertState(SecurityRestrictionContext == 0);
 	AssertArg(OidIsValid(userid));
 	OuterUserId = userid;
 
@@ -341,7 +343,7 @@ GetSessionUserId(void)
 static void
 SetSessionUserId(Oid userid, bool is_superuser)
 {
-	AssertState(!SecurityDefinerContext);
+	AssertState(SecurityRestrictionContext == 0);
 	AssertArg(OidIsValid(userid));
 	SessionUserId = userid;
 	SessionUserIsSuperuser = is_superuser;
@@ -354,11 +356,29 @@ SetSessionUserId(Oid userid, bool is_superuser)
 
 
 /*
- * 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,
@@ -367,27 +387,66 @@ SetSessionUserId(Oid userid, bool is_superuser)
  * through AbortTransaction without asserting in case InitPostgres fails.
  */
 void
-GetUserIdAndContext(Oid *userid, bool *sec_def_context)
+GetUserIdAndSecContext(Oid *userid, int *sec_context)
 {
 	*userid = CurrentUserId;
-	*sec_def_context = SecurityDefinerContext;
+	*sec_context = SecurityRestrictionContext;
 }
 
 void
-SetUserIdAndContext(Oid userid, bool sec_def_context)
+SetUserIdAndSecContext(Oid 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(Oid *userid, bool *sec_def_context)
+{
+	*userid = CurrentUserId;
+	*sec_def_context = InLocalUserIdChange();
+}
+
+void
+SetUserIdAndContext(Oid 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 fc7c2e5e1ce..8f9a96722e9 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -10,7 +10,7 @@
  * Written by Peter Eisentraut <peter_e@gmx.net>.
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/utils/misc/guc.c,v 1.505.2.2 2009/09/03 22:08:14 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/utils/misc/guc.c,v 1.505.2.3 2009/12/09 21:58:04 tgl Exp $
  *
  *--------------------------------------------------------------------
  */
@@ -2305,7 +2305,7 @@ static struct config_string ConfigureNamesString[] =
 		{"role", PGC_USERSET, UNGROUPED,
 			gettext_noop("Sets the current role."),
 			NULL,
-			GUC_IS_NAME | 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_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_NOT_WHILE_SEC_REST
 		},
 		&role_string,
 		"none", assign_role, show_role
@@ -2316,7 +2316,7 @@ static struct config_string ConfigureNamesString[] =
 		{"session_authorization", PGC_USERSET, UNGROUPED,
 			gettext_noop("Sets 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_REST
 		},
 		&session_authorization_string,
 		NULL, assign_session_authorization, show_session_authorization
@@ -4636,29 +4636,45 @@ 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_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
+	 * Note: variables marked GUC_NOT_WHILE_SEC_REST should usually be marked
 	 * GUC_NO_RESET_ALL as well, because ResetAllOptions() doesn't check this.
+	 * An exception might be made if the reset value is assumed to be "safe".
 	 *
 	 * Note: this flag is currently used for "session_authorization" and
-	 * "role".  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.
+	 * "role".  We need to prohibit changing these inside a local userid
+	 * context because when we exit it, GUC won't be notified, leaving things
+	 * out of sync.  (This could be fixed by forcing a new GUC nesting level,
+	 * but that would change behavior in possibly-undesirable ways.)  Also,
+	 * we prohibit changing these in a security-restricted operation because
+	 * otherwise RESET could be used to regain the session user's privileges.
 	 */
-	if ((record->flags & GUC_NOT_WHILE_SEC_DEF) && InSecurityDefinerContext())
+	if (record->flags & GUC_NOT_WHILE_SEC_REST)
 	{
-		ereport(elevel,
-				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-				 errmsg("cannot set parameter \"%s\" within security-definer function",
-						name)));
-		return false;
+		if (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 (InSecurityRestrictedOperation())
+		{
+			ereport(elevel,
+					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+					 errmsg("cannot set parameter \"%s\" within security-restricted operation",
+							name)));
+			return false;
+		}
 	}
 
 	/*