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

1 files changed, 25 insertions, 17 deletions

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 5b94cda2037..b549bc0803f 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -10,7 +10,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/access/transam/xact.c,v 1.229.2.5 2009/11/23 09:59:11 heikki Exp $
+ *	  $PostgreSQL: pgsql/src/backend/access/transam/xact.c,v 1.229.2.6 2009/12/09 21:58:28 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -121,12 +121,13 @@ typedef struct TransactionStateData
 	int			savepointLevel; /* savepoint level */
 	TransState	state;			/* low-level state */
 	TBlockState blockState;		/* high-level state */
-	int			nestingLevel;	/* nest depth */
+	int			nestingLevel;	/* transaction nesting depth */
+	int			gucNestLevel;	/* GUC context nesting depth */
 	MemoryContext curTransactionContext;		/* my xact-lifetime context */
 	ResourceOwner curTransactionOwner;	/* my query resources */
 	List	   *childXids;		/* subcommitted child XIDs */
 	Oid			prevUser;		/* previous CurrentUserId setting */
-	bool		prevSecDefCxt;	/* previous SecurityDefinerContext setting */
+	int			prevSecContext;	/* previous SecurityRestrictionContext */
 	bool		prevXactReadOnly;		/* entry-time xact r/o state */
 	struct TransactionStateData *parent;		/* back link to parent */
 } TransactionStateData;
@@ -146,12 +147,13 @@ static TransactionStateData TopTransactionStateData = {
 	TRANS_DEFAULT,				/* transaction state */
 	TBLOCK_DEFAULT,				/* transaction block state from the client
 								 * perspective */
-	0,							/* nesting level */
+	0,							/* transaction nesting depth */
+	0,							/* GUC context nesting depth */
 	NULL,						/* cur transaction context */
 	NULL,						/* cur transaction resource owner */
 	NIL,						/* subcommitted child Xids */
 	InvalidOid,					/* previous CurrentUserId setting */
-	false,						/* previous SecurityDefinerContext setting */
+	0,							/* previous SecurityRestrictionContext */
 	false,						/* entry-time xact r/o state */
 	NULL						/* link to parent state block */
 };
@@ -1433,14 +1435,16 @@ StartTransaction(void)
 	 * note: prevXactReadOnly is not used at the outermost level
 	 */
 	s->nestingLevel = 1;
+	s->gucNestLevel = 1;
 	s->childXids = NIL;
-	GetUserIdAndContext(&s->prevUser, &s->prevSecDefCxt);
-	/* SecurityDefinerContext should never be set outside a transaction */
-	Assert(!s->prevSecDefCxt);
+	GetUserIdAndSecContext(&s->prevUser, &s->prevSecContext);
+	/* SecurityRestrictionContext should never be set outside a transaction */
+	Assert(s->prevSecContext == 0);
 
 	/*
 	 * initialize other subsystems for new transaction
 	 */
+	AtStart_GUC();
 	AtStart_Inval();
 	AtStart_Cache();
 	AfterTriggerBeginXact();
@@ -1627,7 +1631,7 @@ CommitTransaction(void)
 	/* Check we've released all catcache entries */
 	AtEOXact_CatCache(true);
 
-	AtEOXact_GUC(true, false);
+	AtEOXact_GUC(true, 1);
 	AtEOXact_SPI(true);
 	AtEOXact_on_commit_actions(true);
 	AtEOXact_Namespace(true);
@@ -1647,6 +1651,7 @@ CommitTransaction(void)
 	s->transactionId = InvalidTransactionId;
 	s->subTransactionId = InvalidSubTransactionId;
 	s->nestingLevel = 0;
+	s->gucNestLevel = 0;
 	s->childXids = NIL;
 
 	/*
@@ -1864,7 +1869,7 @@ PrepareTransaction(void)
 	AtEOXact_CatCache(true);
 
 	/* PREPARE acts the same as COMMIT as far as GUC is concerned */
-	AtEOXact_GUC(true, false);
+	AtEOXact_GUC(true, 1);
 	AtEOXact_SPI(true);
 	AtEOXact_on_commit_actions(true);
 	AtEOXact_Namespace(true);
@@ -1883,6 +1888,7 @@ PrepareTransaction(void)
 	s->transactionId = InvalidTransactionId;
 	s->subTransactionId = InvalidSubTransactionId;
 	s->nestingLevel = 0;
+	s->gucNestLevel = 0;
 	s->childXids = NIL;
 
 	/*
@@ -1946,13 +1952,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 or role
 	 * settings here because those can only be changed via GUC, and GUC will
 	 * take care of rolling them back if need be.)
 	 */
-	SetUserIdAndContext(s->prevUser, s->prevSecDefCxt);
+	SetUserIdAndSecContext(s->prevUser, s->prevSecContext);
 
 	/*
 	 * do abort processing
@@ -2015,7 +2021,7 @@ AbortTransaction(void)
 						 false, true);
 	AtEOXact_CatCache(false);
 
-	AtEOXact_GUC(false, false);
+	AtEOXact_GUC(false, 1);
 	AtEOXact_SPI(false);
 	AtEOXact_on_commit_actions(false);
 	AtEOXact_Namespace(false);
@@ -2062,6 +2068,7 @@ CleanupTransaction(void)
 	s->transactionId = InvalidTransactionId;
 	s->subTransactionId = InvalidSubTransactionId;
 	s->nestingLevel = 0;
+	s->gucNestLevel = 0;
 	s->childXids = NIL;
 
 	/*
@@ -3726,7 +3733,7 @@ CommitSubTransaction(void)
 						 RESOURCE_RELEASE_AFTER_LOCKS,
 						 true, false);
 
-	AtEOXact_GUC(true, true);
+	AtEOXact_GUC(true, s->gucNestLevel);
 	AtEOSubXact_SPI(true, s->subTransactionId);
 	AtEOSubXact_on_commit_actions(true, s->subTransactionId,
 								  s->parent->subTransactionId);
@@ -3801,7 +3808,7 @@ AbortSubTransaction(void)
 	 * Reset user ID which might have been changed transiently.  (See notes
 	 * in AbortTransaction.)
 	 */
-	SetUserIdAndContext(s->prevUser, s->prevSecDefCxt);
+	SetUserIdAndSecContext(s->prevUser, s->prevSecContext);
 
 	/*
 	 * We can skip all this stuff if the subxact failed before creating a
@@ -3844,7 +3851,7 @@ AbortSubTransaction(void)
 							 RESOURCE_RELEASE_AFTER_LOCKS,
 							 false, false);
 
-		AtEOXact_GUC(false, true);
+		AtEOXact_GUC(false, s->gucNestLevel);
 		AtEOSubXact_SPI(false, s->subTransactionId);
 		AtEOSubXact_on_commit_actions(false, s->subTransactionId,
 									  s->parent->subTransactionId);
@@ -3938,10 +3945,11 @@ PushTransaction(void)
 	s->subTransactionId = currentSubTransactionId;
 	s->parent = p;
 	s->nestingLevel = p->nestingLevel + 1;
+	s->gucNestLevel = NewGUCNestLevel();
 	s->savepointLevel = p->savepointLevel;
 	s->state = TRANS_DEFAULT;
 	s->blockState = TBLOCK_SUBBEGIN;
-	GetUserIdAndContext(&s->prevUser, &s->prevSecDefCxt);
+	GetUserIdAndSecContext(&s->prevUser, &s->prevSecContext);
 	s->prevXactReadOnly = XactReadOnly;
 
 	CurrentTransactionState = s;