Skip to content

Commit eedb068

Browse files
tglsfdctglsfdc
committed
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
1 parent 98f27aa commit eedb068

File tree

13 files changed

+237
-127
lines changed

13 files changed

+237
-127
lines changed

‎doc/src/sgml/ref/set_role.sgml

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
<!--
2-
$PostgreSQL: pgsql/doc/src/sgml/ref/set_role.sgml,v 1.4 2007/01/31 23:26:04 momjian Exp $
2+
$PostgreSQL: pgsql/doc/src/sgml/ref/set_role.sgml,v 1.5 2008/01/03 21:23:15 tgl Exp $
33
PostgreSQL documentation
44
-->
55

@@ -31,7 +31,7 @@ RESET ROLE
3131

3232
<para>
3333
This command sets the current user
34-
identifier of the current SQL-session context to be <replaceable
34+
identifier of the current SQL session to be <replaceable
3535
class="parameter">rolename</replaceable>. The role name can be
3636
written as either an identifier or a string literal.
3737
After <command>SET ROLE</>, permissions checking for SQL commands
@@ -89,6 +89,11 @@ RESET ROLE
8989
roles with <command>SET ROLE</> does not change the set of roles
9090
allowed to a later <command>SET ROLE</>.
9191
</para>
92+
93+
<para>
94+
<command>SET ROLE</> cannot be used within a
95+
<literal>SECURITY DEFINER</> function.
96+
</para>
9297
</refsect1>
9398

9499
<refsect1>

‎doc/src/sgml/ref/set_session_auth.sgml

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
<!-- $PostgreSQL: pgsql/doc/src/sgml/ref/set_session_auth.sgml,v 1.16 2007/01/31 23:26:04 momjian Exp $ -->
1+
<!-- $PostgreSQL: pgsql/doc/src/sgml/ref/set_session_auth.sgml,v 1.17 2008/01/03 21:23:15 tgl Exp $ -->
22
<refentry id="SQL-SET-SESSION-AUTHORIZATION">
33
<refmeta>
44
<refentrytitle id="sql-set-session-authorization-title">SET SESSION AUTHORIZATION</refentrytitle>
@@ -27,7 +27,7 @@ RESET SESSION AUTHORIZATION
2727

2828
<para>
2929
This command sets the session user identifier and the current user
30-
identifier of the current SQL-session context to be <replaceable
30+
identifier of the current SQL session to be <replaceable
3131
class="parameter">username</replaceable>. The user name can be
3232
written as either an identifier or a string literal. Using this
3333
command, it is possible, for example, to temporarily become an
@@ -38,7 +38,7 @@ RESET SESSION AUTHORIZATION
3838
The session user identifier is initially set to be the (possibly
3939
authenticated) user name provided by the client. The current user
4040
identifier is normally equal to the session user identifier, but
41-
might change temporarily in the context of <quote>setuid</quote>
41+
might change temporarily in the context of <literal>SECURITY DEFINER</>
4242
functions and similar mechanisms; it can also be changed by
4343
<xref linkend="sql-set-role" endterm="sql-set-role-title">.
4444
The current user identifier is relevant for permission checking.
@@ -64,6 +64,15 @@ RESET SESSION AUTHORIZATION
6464
</para>
6565
</refsect1>
6666

67+
<refsect1>
68+
<title>Notes</title>
69+
70+
<para>
71+
<command>SET SESSION AUTHORIZATION</> cannot be used within a
72+
<literal>SECURITY DEFINER</> function.
73+
</para>
74+
</refsect1>
75+
6776
<refsect1>
6877
<title>Examples</title>
6978

‎doc/src/sgml/ref/show.sgml

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
<!--
2-
$PostgreSQL: pgsql/doc/src/sgml/ref/show.sgml,v 1.44 2007/09/26 22:36:30 tgl Exp $
2+
$PostgreSQL: pgsql/doc/src/sgml/ref/show.sgml,v 1.45 2008/01/03 21:23:15 tgl Exp $
33
PostgreSQL documentation
44
-->
55

@@ -104,8 +104,7 @@ SHOW ALL
104104
<term><literal>IS_SUPERUSER</literal></term>
105105
<listitem>
106106
<para>
107-
True if the current session authorization identifier has
108-
superuser privileges.
107+
True if the current role has superuser privileges.
109108
</para>
110109
</listitem>
111110
</varlistentry>

‎src/backend/access/transam/xact.c

Lines changed: 25 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
*
1111
*
1212
* IDENTIFICATION
13-
* $PostgreSQL: pgsql/src/backend/access/transam/xact.c,v 1.255 2008/01/01 19:45:48 momjian Exp $
13+
* $PostgreSQL: pgsql/src/backend/access/transam/xact.c,v 1.256 2008/01/03 21:23:15 tgl Exp $
1414
*
1515
*-------------------------------------------------------------------------
1616
*/
@@ -123,7 +123,8 @@ typedef struct TransactionStateData
123123
MemoryContext curTransactionContext; /* my xact-lifetime context */
124124
ResourceOwner curTransactionOwner; /* my query resources */
125125
List *childXids; /* subcommitted child XIDs */
126-
Oid currentUser; /* subxact start current_user */
126+
Oid prevUser; /* previous CurrentUserId setting */
127+
bool prevSecDefCxt; /* previous SecurityDefinerContext setting */
127128
bool prevXactReadOnly; /* entry-time xact r/o state */
128129
struct TransactionStateData *parent; /* back link to parent */
129130
} TransactionStateData;
@@ -148,7 +149,8 @@ static TransactionStateData TopTransactionStateData = {
148149
NULL, /* cur transaction context */
149150
NULL, /* cur transaction resource owner */
150151
NIL, /* subcommitted child Xids */
151-
0, /* entry-time current userid */
152+
InvalidOid, /* previous CurrentUserId setting */
153+
false, /* previous SecurityDefinerContext setting */
152154
false, /* entry-time xact r/o state */
153155
NULL /* link to parent state block */
154156
};
@@ -1490,19 +1492,15 @@ StartTransaction(void)
14901492

14911493
/*
14921494
* initialize current transaction state fields
1495+
*
1496+
* note: prevXactReadOnly is not used at the outermost level
14931497
*/
14941498
s->nestingLevel = 1;
14951499
s->gucNestLevel = 1;
14961500
s->childXids = NIL;
1497-
1498-
/*
1499-
* You might expect to see "s->currentUser = GetUserId();" here, but you
1500-
* won't because it doesn't work during startup; the userid isn't set yet
1501-
* during a backend's first transaction start. We only use the
1502-
* currentUser field in sub-transaction state structs.
1503-
*
1504-
* prevXactReadOnly is also valid only in sub-transactions.
1505-
*/
1501+
GetUserIdAndContext(&s->prevUser, &s->prevSecDefCxt);
1502+
/* SecurityDefinerContext should never be set outside a transaction */
1503+
Assert(!s->prevSecDefCxt);
15061504

15071505
/*
15081506
* initialize other subsystems for new transaction
@@ -1963,17 +1961,16 @@ AbortTransaction(void)
19631961
s->state = TRANS_ABORT;
19641962

19651963
/*
1966-
* Reset user id which might have been changed transiently. We cannot use
1967-
* s->currentUser, since it may not be set yet; instead rely on internal
1968-
* state of miscinit.c.
1964+
* Reset user ID which might have been changed transiently. We need this
1965+
* to clean up in case control escaped out of a SECURITY DEFINER function
1966+
* or other local change of CurrentUserId; therefore, the prior value
1967+
* of SecurityDefinerContext also needs to be restored.
19691968
*
1970-
* (Note: it is not necessary to restore session authorization here
1971-
* because that can only be changed via GUC, and GUC will take care of
1972-
* rolling it back if need be. However, an error within a SECURITY
1973-
* DEFINER function could send control here with the wrong current
1974-
* userid.)
1969+
* (Note: it is not necessary to restore session authorization or role
1970+
* settings here because those can only be changed via GUC, and GUC will
1971+
* take care of rolling them back if need be.)
19751972
*/
1976-
AtAbort_UserId();
1973+
SetUserIdAndContext(s->prevUser, s->prevSecDefCxt);
19771974

19781975
/*
19791976
* do abort processing
@@ -3806,6 +3803,12 @@ AbortSubTransaction(void)
38063803

38073804
s->state = TRANS_ABORT;
38083805

3806+
/*
3807+
* Reset user ID which might have been changed transiently. (See notes
3808+
* in AbortTransaction.)
3809+
*/
3810+
SetUserIdAndContext(s->prevUser, s->prevSecDefCxt);
3811+
38093812
/*
38103813
* We can skip all this stuff if the subxact failed before creating a
38113814
* ResourceOwner...
@@ -3858,20 +3861,6 @@ AbortSubTransaction(void)
38583861
AtEOSubXact_PgStat(false, s->nestingLevel);
38593862
}
38603863

3861-
/*
3862-
* Reset user id which might have been changed transiently. Here we want
3863-
* to restore to the userid that was current at subxact entry. (As in
3864-
* AbortTransaction, we need not worry about the session userid.)
3865-
*
3866-
* Must do this after AtEOXact_GUC to handle the case where we entered the
3867-
* subxact inside a SECURITY DEFINER function (hence current and session
3868-
* userids were different) and then session auth was changed inside the
3869-
* subxact. GUC will reset both current and session userids to the
3870-
* entry-time session userid. This is right in every other scenario so it
3871-
* seems simplest to let GUC do that and fix it here.
3872-
*/
3873-
SetUserId(s->currentUser);
3874-
38753864
/*
38763865
* Restore the upper transaction's read-only state, too. This should be
38773866
* redundant with GUC's cleanup but we may as well do it for consistency
@@ -3926,13 +3915,6 @@ PushTransaction(void)
39263915
{
39273916
TransactionState p = CurrentTransactionState;
39283917
TransactionState s;
3929-
Oid currentUser;
3930-
3931-
/*
3932-
* At present, GetUserId cannot fail, but let's not assume that. Get the
3933-
* ID before entering the critical code sequence.
3934-
*/
3935-
currentUser = GetUserId();
39363918

39373919
/*
39383920
* We keep subtransaction state nodes in TopTransactionContext.
@@ -3966,7 +3948,7 @@ PushTransaction(void)
39663948
s->savepointLevel = p->savepointLevel;
39673949
s->state = TRANS_DEFAULT;
39683950
s->blockState = TBLOCK_SUBBEGIN;
3969-
s->currentUser = currentUser;
3951+
GetUserIdAndContext(&s->prevUser, &s->prevSecDefCxt);
39703952
s->prevXactReadOnly = XactReadOnly;
39713953

39723954
CurrentTransactionState = s;

‎src/backend/catalog/index.c

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/catalog/index.c,v 1.289 2008/01/01 19:45:48 momjian Exp $
11+
* $PostgreSQL: pgsql/src/backend/catalog/index.c,v 1.290 2008/01/03 21:23:15 tgl Exp $
1212
*
1313
*
1414
* INTERFACE ROUTINES
@@ -1339,6 +1339,8 @@ index_build(Relation heapRelation,
13391339
{
13401340
RegProcedure procedure;
13411341
IndexBuildResult *stats;
1342+
Oid save_userid;
1343+
bool save_secdefcxt;
13421344

13431345
/*
13441346
* sanity checks
@@ -1349,6 +1351,13 @@ index_build(Relation heapRelation,
13491351
procedure = indexRelation->rd_am->ambuild;
13501352
Assert(RegProcedureIsValid(procedure));
13511353

1354+
/*
1355+
* Switch to the table owner's userid, so that any index functions are
1356+
* run as that user.
1357+
*/
1358+
GetUserIdAndContext(&save_userid, &save_secdefcxt);
1359+
SetUserIdAndContext(heapRelation->rd_rel->relowner, true);
1360+
13521361
/*
13531362
* Call the access method's build procedure
13541363
*/
@@ -1359,6 +1368,9 @@ index_build(Relation heapRelation,
13591368
PointerGetDatum(indexInfo)));
13601369
Assert(PointerIsValid(stats));
13611370

1371+
/* Restore userid */
1372+
SetUserIdAndContext(save_userid, save_secdefcxt);
1373+
13621374
/*
13631375
* If we found any potentially broken HOT chains, mark the index as not
13641376
* being usable until the current transaction is below the event horizon.
@@ -1857,6 +1869,8 @@ validate_index(Oid heapId, Oid indexId, Snapshot snapshot)
18571869
IndexInfo *indexInfo;
18581870
IndexVacuumInfo ivinfo;
18591871
v_i_state state;
1872+
Oid save_userid;
1873+
bool save_secdefcxt;
18601874

18611875
/* Open and lock the parent heap relation */
18621876
heapRelation = heap_open(heapId, ShareUpdateExclusiveLock);
@@ -1873,6 +1887,13 @@ validate_index(Oid heapId, Oid indexId, Snapshot snapshot)
18731887
/* mark build is concurrent just for consistency */
18741888
indexInfo->ii_Concurrent = true;
18751889

1890+
/*
1891+
* Switch to the table owner's userid, so that any index functions are
1892+
* run as that user.
1893+
*/
1894+
GetUserIdAndContext(&save_userid, &save_secdefcxt);
1895+
SetUserIdAndContext(heapRelation->rd_rel->relowner, true);
1896+
18761897
/*
18771898
* Scan the index and gather up all the TIDs into a tuplesort object.
18781899
*/
@@ -1910,6 +1931,9 @@ validate_index(Oid heapId, Oid indexId, Snapshot snapshot)
19101931
"validate_index found %.0f heap tuples, %.0f index tuples; inserted %.0f missing tuples",
19111932
state.htups, state.itups, state.tups_inserted);
19121933

1934+
/* Restore userid */
1935+
SetUserIdAndContext(save_userid, save_secdefcxt);
1936+
19131937
/* Close rels, but keep locks */
19141938
index_close(indexRelation, NoLock);
19151939
heap_close(heapRelation, NoLock);

‎src/backend/commands/analyze.c

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/commands/analyze.c,v 1.113 2008/01/01 19:45:48 momjian Exp $
11+
* $PostgreSQL: pgsql/src/backend/commands/analyze.c,v 1.114 2008/01/03 21:23:15 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -119,6 +119,8 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
119119
HeapTuple *rows;
120120
PGRUsage ru0;
121121
TimestampTz starttime = 0;
122+
Oid save_userid;
123+
bool save_secdefcxt;
122124

123125
if (vacstmt->verbose)
124126
elevel = INFO;
@@ -202,6 +204,18 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
202204
return;
203205
}
204206

207+
ereport(elevel,
208+
(errmsg("analyzing \"%s.%s\"",
209+
get_namespace_name(RelationGetNamespace(onerel)),
210+
RelationGetRelationName(onerel))));
211+
212+
/*
213+
* Switch to the table owner's userid, so that any index functions are
214+
* run as that user.
215+
*/
216+
GetUserIdAndContext(&save_userid, &save_secdefcxt);
217+
SetUserIdAndContext(onerel->rd_rel->relowner, true);
218+
205219
/* let others know what I'm doing */
206220
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
207221
MyProc->vacuumFlags |= PROC_IN_ANALYZE;
@@ -215,11 +229,6 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
215229
starttime = GetCurrentTimestamp();
216230
}
217231

218-
ereport(elevel,
219-
(errmsg("analyzing \"%s.%s\"",
220-
get_namespace_name(RelationGetNamespace(onerel)),
221-
RelationGetRelationName(onerel))));
222-
223232
/*
224233
* Determine which columns to analyze
225234
*
@@ -344,9 +353,7 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
344353
onerel->rd_rel->relisshared,
345354
0, 0);
346355

347-
vac_close_indexes(nindexes, Irel, AccessShareLock);
348-
relation_close(onerel, ShareUpdateExclusiveLock);
349-
return;
356+
goto cleanup;
350357
}
351358

352359
/*
@@ -466,6 +473,9 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
466473
totalrows, totaldeadrows);
467474
}
468475

476+
/* We skip to here if there were no analyzable columns */
477+
cleanup:
478+
469479
/* Done with indexes */
470480
vac_close_indexes(nindexes, Irel, NoLock);
471481

@@ -498,6 +508,9 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
498508
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
499509
MyProc->vacuumFlags &= ~PROC_IN_ANALYZE;
500510
LWLockRelease(ProcArrayLock);
511+
512+
/* Restore userid */
513+
SetUserIdAndContext(save_userid, save_secdefcxt);
501514
}
502515

503516
/*

0 commit comments

Comments
 (0)