Skip to content

Commit d49f84b

Browse files
sfrostsfrost
committed
Fix column-privilege leak in error-message paths
While building error messages to return to the user, BuildIndexValueDescription, ExecBuildSlotValueDescription and ri_ReportViolation would happily include the entire key or entire row in the result returned to the user, even if the user didn't have access to view all of the columns being included. Instead, include only those columns which the user is providing or which the user has select rights on. If the user does not have any rights to view the table or any of the columns involved then no detail is provided and a NULL value is returned from BuildIndexValueDescription and ExecBuildSlotValueDescription. Note that, for key cases, the user must have access to all of the columns for the key to be shown; a partial key will not be returned. Back-patch all the way, as column-level privileges are now in all supported versions. This has been assigned CVE-2014-8161, but since the issue and the patch have already been publicized on pgsql-hackers, there's no point in trying to hide this commit.
1 parent 350f1e7 commit d49f84b

File tree

10 files changed

+344
-70
lines changed

10 files changed

+344
-70
lines changed

‎src/backend/access/index/genam.c

Lines changed: 58 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,11 @@
2424
#include "catalog/index.h"
2525
#include "miscadmin.h"
2626
#include "storage/bufmgr.h"
27+
#include "utils/acl.h"
2728
#include "utils/builtins.h"
2829
#include "utils/lsyscache.h"
2930
#include "utils/rel.h"
31+
#include "utils/syscache.h"
3032
#include "utils/tqual.h"
3133

3234

@@ -152,6 +154,11 @@ IndexScanEnd(IndexScanDesc scan)
152154
* form "(key_name, ...)=(key_value, ...)". This is currently used
153155
* for building unique-constraint and exclusion-constraint error messages.
154156
*
157+
* Note that if the user does not have permissions to view all of the
158+
* columns involved then a NULL is returned. Returning a partial key seems
159+
* unlikely to be useful and we have no way to know which of the columns the
160+
* user provided (unlike in ExecBuildSlotValueDescription).
161+
*
155162
* The passed-in values/nulls arrays are the "raw" input to the index AM,
156163
* e.g. results of FormIndexDatum --- this is not necessarily what is stored
157164
* in the index, but it's what the user perceives to be stored.
@@ -161,13 +168,62 @@ BuildIndexValueDescription(Relation indexRelation,
161168
Datum *values, bool *isnull)
162169
{
163170
StringInfoData buf;
171+
Form_pg_index idxrec;
172+
HeapTuple ht_idx;
164173
int natts = indexRelation->rd_rel->relnatts;
165174
int i;
175+
int keyno;
176+
Oid indexrelid = RelationGetRelid(indexRelation);
177+
Oid indrelid;
178+
AclResult aclresult;
179+
180+
/*
181+
* Check permissions- if the user does not have access to view all of the
182+
* key columns then return NULL to avoid leaking data.
183+
*
184+
* First we need to check table-level SELECT access and then, if
185+
* there is no access there, check column-level permissions.
186+
*/
187+
188+
/*
189+
* Fetch the pg_index tuple by the Oid of the index
190+
*/
191+
ht_idx = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexrelid));
192+
if (!HeapTupleIsValid(ht_idx))
193+
elog(ERROR, "cache lookup failed for index %u", indexrelid);
194+
idxrec = (Form_pg_index) GETSTRUCT(ht_idx);
195+
196+
indrelid = idxrec->indrelid;
197+
Assert(indexrelid == idxrec->indexrelid);
198+
199+
/* Table-level SELECT is enough, if the user has it */
200+
aclresult = pg_class_aclcheck(indrelid, GetUserId(), ACL_SELECT);
201+
if (aclresult != ACLCHECK_OK)
202+
{
203+
/*
204+
* No table-level access, so step through the columns in the
205+
* index and make sure the user has SELECT rights on all of them.
206+
*/
207+
for (keyno = 0; keyno < idxrec->indnatts; keyno++)
208+
{
209+
AttrNumber attnum = idxrec->indkey.values[keyno];
210+
211+
aclresult = pg_attribute_aclcheck(indrelid, attnum, GetUserId(),
212+
ACL_SELECT);
213+
214+
if (aclresult != ACLCHECK_OK)
215+
{
216+
/* No access, so clean up and return */
217+
ReleaseSysCache(ht_idx);
218+
return NULL;
219+
}
220+
}
221+
}
222+
ReleaseSysCache(ht_idx);
166223

167224
initStringInfo(&buf);
168225
appendStringInfo(&buf, "(%s)=(",
169-
pg_get_indexdef_columns(RelationGetRelid(indexRelation),
170-
true));
226+
pg_get_indexdef_columns(indexrelid, true));
171227

172228
for (i = 0; i < natts; i++)
173229
{

‎src/backend/access/nbtree/nbtinsert.c

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -384,16 +384,20 @@ _bt_check_unique(Relation rel, IndexTuple itup, Relation heapRel,
384384
{
385385
Datum values[INDEX_MAX_KEYS];
386386
bool isnull[INDEX_MAX_KEYS];
387+
char *key_desc;
387388

388389
index_deform_tuple(itup, RelationGetDescr(rel),
389390
values, isnull);
391+
392+
key_desc = BuildIndexValueDescription(rel, values,
393+
isnull);
394+
390395
ereport(ERROR,
391396
(errcode(ERRCODE_UNIQUE_VIOLATION),
392397
errmsg("duplicate key value violates unique constraint \"%s\"",
393398
RelationGetRelationName(rel)),
394-
errdetail("Key %s already exists.",
395-
BuildIndexValueDescription(rel,
396-
values, isnull))));
399+
key_desc ? errdetail("Key %s already exists.",
400+
key_desc) : 0));
397401
}
398402
}
399403
else if (all_dead)

‎src/backend/commands/copy.c

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,7 @@ typedef struct CopyStateData
151151
int *defmap; /* array of default att numbers */
152152
ExprState **defexprs; /* array of default att expressions */
153153
bool volatile_defexprs; /* is any of defexprs volatile? */
154+
List *range_table;
154155

155156
/*
156157
* These variables are used to reduce overhead in textual COPY FROM.
@@ -747,6 +748,7 @@ DoCopy(const CopyStmt *stmt, const char *queryString)
747748
bool pipe = (stmt->filename == NULL);
748749
Relation rel;
749750
uint64 processed;
751+
RangeTblEntry *rte;
750752

751753
/* Disallow file COPY except to superusers. */
752754
if (!pipe && !superuser())
@@ -760,7 +762,6 @@ DoCopy(const CopyStmt *stmt, const char *queryString)
760762
{
761763
TupleDesc tupDesc;
762764
AclMode required_access = (is_from ? ACL_INSERT : ACL_SELECT);
763-
RangeTblEntry *rte;
764765
List *attnums;
765766
ListCell *cur;
766767

@@ -807,13 +808,15 @@ DoCopy(const CopyStmt *stmt, const char *queryString)
807808

808809
cstate = BeginCopyFrom(rel, stmt->filename,
809810
stmt->attlist, stmt->options);
811+
cstate->range_table = list_make1(rte);
810812
processed = CopyFrom(cstate); /* copy from file to database */
811813
EndCopyFrom(cstate);
812814
}
813815
else
814816
{
815817
cstate = BeginCopyTo(rel, stmt->query, queryString, stmt->filename,
816818
stmt->attlist, stmt->options);
819+
cstate->range_table = list_make1(rte);
817820
processed = DoCopyTo(cstate); /* copy from database to file */
818821
EndCopyTo(cstate);
819822
}
@@ -1957,6 +1960,7 @@ CopyFrom(CopyState cstate)
19571960
estate->es_result_relations = resultRelInfo;
19581961
estate->es_num_result_relations = 1;
19591962
estate->es_result_relation_info = resultRelInfo;
1963+
estate->es_range_table = cstate->range_table;
19601964

19611965
/* Set up a tuple slot too */
19621966
myslot = ExecInitExtraTupleSlot(estate);

‎src/backend/commands/trigger.c

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,12 @@ int SessionReplicationRole = SESSION_REPLICATION_ROLE_ORIGIN;
6363
/* How many levels deep into trigger execution are we? */
6464
static int MyTriggerDepth = 0;
6565

66+
/*
67+
* Note that this macro also exists in executor/execMain.c. There does not
68+
* appear to be any good header to put it into, given the structures that
69+
* it uses, so we let them be duplicated. Be sure to update both if one needs
70+
* to be changed, however.
71+
*/
6672
#define GetModifiedColumns(relinfo, estate) \
6773
(rt_fetch((relinfo)->ri_RangeTableIndex, (estate)->es_range_table)->modifiedCols)
6874

0 commit comments

Comments
 (0)