Skip to content

Commit 4b98742

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 78f79b6 commit 4b98742

File tree

10 files changed

+338
-66
lines changed

10 files changed

+338
-66
lines changed

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

Lines changed: 58 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,11 @@
2525
#include "lib/stringinfo.h"
2626
#include "miscadmin.h"
2727
#include "storage/bufmgr.h"
28+
#include "utils/acl.h"
2829
#include "utils/builtins.h"
2930
#include "utils/lsyscache.h"
3031
#include "utils/rel.h"
32+
#include "utils/syscache.h"
3133
#include "utils/tqual.h"
3234

3335

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

168225
initStringInfo(&buf);
169226
appendStringInfo(&buf, "(%s)=(",
170-
pg_get_indexdef_columns(RelationGetRelid(indexRelation),
171-
true));
227+
pg_get_indexdef_columns(indexrelid, true));
172228

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

‎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
errtableconstraint(heapRel,
398402
RelationGetRelationName(rel))));
399403
}

‎src/backend/commands/copy.c

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,7 @@ typedef struct CopyStateData
158158
int *defmap; /* array of default att numbers */
159159
ExprState **defexprs; /* array of default att expressions */
160160
bool volatile_defexprs; /* is any of defexprs volatile? */
161+
List *range_table;
161162

162163
/*
163164
* These variables are used to reduce overhead in textual COPY FROM.
@@ -782,6 +783,7 @@ DoCopy(const CopyStmt *stmt, const char *queryString, uint64 *processed)
782783
bool pipe = (stmt->filename == NULL);
783784
Relation rel;
784785
Oid relid;
786+
RangeTblEntry *rte;
785787

786788
/* Disallow COPY to/from file or program except to superusers. */
787789
if (!pipe && !superuser())
@@ -804,7 +806,6 @@ DoCopy(const CopyStmt *stmt, const char *queryString, uint64 *processed)
804806
{
805807
TupleDesc tupDesc;
806808
AclMode required_access = (is_from ? ACL_INSERT : ACL_SELECT);
807-
RangeTblEntry *rte;
808809
List *attnums;
809810
ListCell *cur;
810811

@@ -854,6 +855,7 @@ DoCopy(const CopyStmt *stmt, const char *queryString, uint64 *processed)
854855

855856
cstate = BeginCopyFrom(rel, stmt->filename, stmt->is_program,
856857
stmt->attlist, stmt->options);
858+
cstate->range_table = list_make1(rte);
857859
*processed = CopyFrom(cstate); /* copy from file to database */
858860
EndCopyFrom(cstate);
859861
}
@@ -862,6 +864,7 @@ DoCopy(const CopyStmt *stmt, const char *queryString, uint64 *processed)
862864
cstate = BeginCopyTo(rel, stmt->query, queryString,
863865
stmt->filename, stmt->is_program,
864866
stmt->attlist, stmt->options);
867+
cstate->range_table = list_make1(rte);
865868
*processed = DoCopyTo(cstate); /* copy from database to file */
866869
EndCopyTo(cstate);
867870
}
@@ -2135,6 +2138,7 @@ CopyFrom(CopyState cstate)
21352138
estate->es_result_relations = resultRelInfo;
21362139
estate->es_num_result_relations = 1;
21372140
estate->es_result_relation_info = resultRelInfo;
2141+
estate->es_range_table = cstate->range_table;
21382142

21392143
/* Set up a tuple slot too */
21402144
myslot = ExecInitExtraTupleSlot(estate);

‎src/backend/commands/trigger.c

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

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

0 commit comments

Comments
 (0)