Skip to content

Commit 045a188

Browse files
deanrasheeddeanrasheed
committed
Always require SELECT permission for ON CONFLICT DO UPDATE.
The update path of an INSERT ... ON CONFLICT DO UPDATE requires SELECT permission on the columns of the arbiter index, but it failed to check for that in the case of an arbiter specified by constraint name. In addition, for a table with row level security enabled, it failed to check updated rows against the table's SELECT policies when the update path was taken (regardless of how the arbiter index was specified). Backpatch to 9.5 where ON CONFLICT DO UPDATE and RLS were introduced. Security: CVE-2017-15099
1 parent 014c5cd commit 045a188

File tree

8 files changed

+197
-13
lines changed

8 files changed

+197
-13
lines changed

‎src/backend/catalog/pg_constraint.c

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#include "access/genam.h"
1818
#include "access/heapam.h"
1919
#include "access/htup_details.h"
20+
#include "access/sysattr.h"
2021
#include "catalog/dependency.h"
2122
#include "catalog/indexing.h"
2223
#include "catalog/objectaccess.h"
@@ -811,6 +812,104 @@ get_relation_constraint_oid(Oid relid, const char *conname, bool missing_ok)
811812
return conOid;
812813
}
813814

815+
/*
816+
* get_relation_constraint_attnos
817+
* Find a constraint on the specified relation with the specified name
818+
* and return the constrained columns.
819+
*
820+
* Returns a Bitmapset of the column attnos of the constrained columns, with
821+
* attnos being offset by FirstLowInvalidHeapAttributeNumber so that system
822+
* columns can be represented.
823+
*
824+
* *constraintOid is set to the OID of the constraint, or InvalidOid on
825+
* failure.
826+
*/
827+
Bitmapset *
828+
get_relation_constraint_attnos(Oid relid, const char *conname,
829+
bool missing_ok, Oid *constraintOid)
830+
{
831+
Bitmapset *conattnos = NULL;
832+
Relation pg_constraint;
833+
HeapTuple tuple;
834+
SysScanDesc scan;
835+
ScanKeyData skey[1];
836+
837+
/* Set *constraintOid, to avoid complaints about uninitialized vars */
838+
*constraintOid = InvalidOid;
839+
840+
/*
841+
* Fetch the constraint tuple from pg_constraint. There may be more than
842+
* one match, because constraints are not required to have unique names;
843+
* if so, error out.
844+
*/
845+
pg_constraint = heap_open(ConstraintRelationId, AccessShareLock);
846+
847+
ScanKeyInit(&skey[0],
848+
Anum_pg_constraint_conrelid,
849+
BTEqualStrategyNumber, F_OIDEQ,
850+
ObjectIdGetDatum(relid));
851+
852+
scan = systable_beginscan(pg_constraint, ConstraintRelidIndexId, true,
853+
NULL, 1, skey);
854+
855+
while (HeapTupleIsValid(tuple = systable_getnext(scan)))
856+
{
857+
Form_pg_constraint con = (Form_pg_constraint) GETSTRUCT(tuple);
858+
Datum adatum;
859+
bool isNull;
860+
ArrayType *arr;
861+
int16 *attnums;
862+
int numcols;
863+
int i;
864+
865+
/* Check the constraint name */
866+
if (strcmp(NameStr(con->conname), conname) != 0)
867+
continue;
868+
if (OidIsValid(*constraintOid))
869+
ereport(ERROR,
870+
(errcode(ERRCODE_DUPLICATE_OBJECT),
871+
errmsg("table \"%s\" has multiple constraints named \"%s\"",
872+
get_rel_name(relid), conname)));
873+
874+
*constraintOid = HeapTupleGetOid(tuple);
875+
876+
/* Extract the conkey array, ie, attnums of constrained columns */
877+
adatum = heap_getattr(tuple, Anum_pg_constraint_conkey,
878+
RelationGetDescr(pg_constraint), &isNull);
879+
if (isNull)
880+
continue; /* no constrained columns */
881+
882+
arr = DatumGetArrayTypeP(adatum); /* ensure not toasted */
883+
numcols = ARR_DIMS(arr)[0];
884+
if (ARR_NDIM(arr) != 1 ||
885+
numcols < 0 ||
886+
ARR_HASNULL(arr) ||
887+
ARR_ELEMTYPE(arr) != INT2OID)
888+
elog(ERROR, "conkey is not a 1-D smallint array");
889+
attnums = (int16 *) ARR_DATA_PTR(arr);
890+
891+
/* Construct the result value */
892+
for (i = 0; i < numcols; i++)
893+
{
894+
conattnos = bms_add_member(conattnos,
895+
attnums[i] - FirstLowInvalidHeapAttributeNumber);
896+
}
897+
}
898+
899+
systable_endscan(scan);
900+
901+
/* If no such constraint exists, complain */
902+
if (!OidIsValid(*constraintOid) && !missing_ok)
903+
ereport(ERROR,
904+
(errcode(ERRCODE_UNDEFINED_OBJECT),
905+
errmsg("constraint \"%s\" for table \"%s\" does not exist",
906+
conname, get_rel_name(relid))));
907+
908+
heap_close(pg_constraint, AccessShareLock);
909+
910+
return conattnos;
911+
}
912+
814913
/*
815914
* get_domain_constraint_oid
816915
* Find a constraint on the specified domain with the specified name.

‎src/backend/parser/parse_clause.c

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2908,9 +2908,26 @@ transformOnConflictArbiter(ParseState *pstate,
29082908

29092909
pstate->p_namespace = save_namespace;
29102910

2911+
/*
2912+
* If the arbiter is specified by constraint name, get the constraint
2913+
* OID and mark the constrained columns as requiring SELECT privilege,
2914+
* in the same way as would have happened if the arbiter had been
2915+
* specified by explicit reference to the constraint's index columns.
2916+
*/
29112917
if (infer->conname)
2912-
*constraint = get_relation_constraint_oid(RelationGetRelid(pstate->p_target_relation),
2913-
infer->conname, false);
2918+
{
2919+
Oid relid = RelationGetRelid(pstate->p_target_relation);
2920+
RangeTblEntry *rte = pstate->p_target_rangetblentry;
2921+
Bitmapset *conattnos;
2922+
2923+
conattnos = get_relation_constraint_attnos(relid, infer->conname,
2924+
false, constraint);
2925+
2926+
/* Make sure the rel as a whole is marked for SELECT access */
2927+
rte->requiredPerms |= ACL_SELECT;
2928+
/* Mark the constrained columns as requiring SELECT access */
2929+
rte->selectedCols = bms_add_members(rte->selectedCols, conattnos);
2930+
}
29142931
}
29152932

29162933
/*

‎src/backend/rewrite/rowsecurity.c

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -307,6 +307,8 @@ get_row_security_policies(Query *root, RangeTblEntry *rte, int rt_index,
307307
{
308308
List *conflict_permissive_policies;
309309
List *conflict_restrictive_policies;
310+
List *conflict_select_permissive_policies = NIL;
311+
List *conflict_select_restrictive_policies = NIL;
310312

311313
/* Get the policies that apply to the auxiliary UPDATE */
312314
get_policies_for_relation(rel, CMD_UPDATE, user_id,
@@ -336,9 +338,6 @@ get_row_security_policies(Query *root, RangeTblEntry *rte, int rt_index,
336338
*/
337339
if (rte->requiredPerms & ACL_SELECT)
338340
{
339-
List *conflict_select_permissive_policies = NIL;
340-
List *conflict_select_restrictive_policies = NIL;
341-
342341
get_policies_for_relation(rel, CMD_SELECT, user_id,
343342
&conflict_select_permissive_policies,
344343
&conflict_select_restrictive_policies);
@@ -359,6 +358,21 @@ get_row_security_policies(Query *root, RangeTblEntry *rte, int rt_index,
359358
withCheckOptions,
360359
hasSubLinks,
361360
false);
361+
362+
/*
363+
* Add ALL/SELECT policies as WCO_RLS_UPDATE_CHECK WCOs, to ensure
364+
* that the final updated row is visible when taking the UPDATE
365+
* path of an INSERT .. ON CONFLICT DO UPDATE, if SELECT rights
366+
* are required for this relation.
367+
*/
368+
if (rte->requiredPerms & ACL_SELECT)
369+
add_with_check_options(rel, rt_index,
370+
WCO_RLS_UPDATE_CHECK,
371+
conflict_select_permissive_policies,
372+
conflict_select_restrictive_policies,
373+
withCheckOptions,
374+
hasSubLinks,
375+
true);
362376
}
363377
}
364378

‎src/include/catalog/pg_constraint.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -248,6 +248,8 @@ extern char *ChooseConstraintName(const char *name1, const char *name2,
248248
extern void AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
249249
Oid newNspId, bool isType, ObjectAddresses *objsMoved);
250250
extern Oid get_relation_constraint_oid(Oid relid, const char *conname, bool missing_ok);
251+
extern Bitmapset *get_relation_constraint_attnos(Oid relid, const char *conname,
252+
bool missing_ok, Oid *constraintOid);
251253
extern Oid get_domain_constraint_oid(Oid typid, const char *conname, bool missing_ok);
252254

253255
extern bool check_functional_grouping(Oid relid,

‎src/test/regress/expected/privileges.out

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -487,11 +487,23 @@ INSERT INTO atest5(two) VALUES (6) ON CONFLICT (two) DO UPDATE set one = 8; -- f
487487
ERROR: permission denied for relation atest5
488488
INSERT INTO atest5(three) VALUES (4) ON CONFLICT (two) DO UPDATE set three = 10; -- fails (due to INSERT)
489489
ERROR: permission denied for relation atest5
490-
-- Check that the the columns in the inference require select privileges
491-
-- Error. No privs on four
492-
INSERT INTO atest5(three) VALUES (4) ON CONFLICT (four) DO UPDATE set three = 10;
490+
-- Check that the columns in the inference require select privileges
491+
INSERT INTO atest5(four) VALUES (4); -- fail
493492
ERROR: permission denied for relation atest5
494493
SET SESSION AUTHORIZATION regressuser1;
494+
GRANT INSERT (four) ON atest5 TO regressuser4;
495+
SET SESSION AUTHORIZATION regressuser4;
496+
INSERT INTO atest5(four) VALUES (4) ON CONFLICT (four) DO UPDATE set three = 3; -- fails (due to SELECT)
497+
ERROR: permission denied for relation atest5
498+
INSERT INTO atest5(four) VALUES (4) ON CONFLICT ON CONSTRAINT atest5_four_key DO UPDATE set three = 3; -- fails (due to SELECT)
499+
ERROR: permission denied for relation atest5
500+
INSERT INTO atest5(four) VALUES (4); -- ok
501+
SET SESSION AUTHORIZATION regressuser1;
502+
GRANT SELECT (four) ON atest5 TO regressuser4;
503+
SET SESSION AUTHORIZATION regressuser4;
504+
INSERT INTO atest5(four) VALUES (4) ON CONFLICT (four) DO UPDATE set three = 3; -- ok
505+
INSERT INTO atest5(four) VALUES (4) ON CONFLICT ON CONSTRAINT atest5_four_key DO UPDATE set three = 3; -- ok
506+
SET SESSION AUTHORIZATION regressuser1;
495507
REVOKE ALL (one) ON atest5 FROM regressuser4;
496508
GRANT SELECT (one,two,blue) ON atest6 TO regressuser4;
497509
SET SESSION AUTHORIZATION regressuser4;

‎src/test/regress/expected/rowsecurity.out

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3363,9 +3363,10 @@ DROP TABLE r1;
33633363
--
33643364
SET SESSION AUTHORIZATION rls_regress_user0;
33653365
SET row_security = on;
3366-
CREATE TABLE r1 (a int);
3366+
CREATE TABLE r1 (a int PRIMARY KEY);
33673367
CREATE POLICY p1 ON r1 FOR SELECT USING (a < 20);
33683368
CREATE POLICY p2 ON r1 FOR UPDATE USING (a < 20) WITH CHECK (true);
3369+
CREATE POLICY p3 ON r1 FOR INSERT WITH CHECK (true);
33693370
INSERT INTO r1 VALUES (10);
33703371
ALTER TABLE r1 ENABLE ROW LEVEL SECURITY;
33713372
ALTER TABLE r1 FORCE ROW LEVEL SECURITY;
@@ -3392,6 +3393,18 @@ ALTER TABLE r1 FORCE ROW LEVEL SECURITY;
33923393
-- Error
33933394
UPDATE r1 SET a = 30 RETURNING *;
33943395
ERROR: new row violates row-level security policy for table "r1"
3396+
-- UPDATE path of INSERT ... ON CONFLICT DO UPDATE should also error out
3397+
INSERT INTO r1 VALUES (10)
3398+
ON CONFLICT (a) DO UPDATE SET a = 30 RETURNING *;
3399+
ERROR: new row violates row-level security policy for table "r1"
3400+
-- Should still error out without RETURNING (use of arbiter always requires
3401+
-- SELECT permissions)
3402+
INSERT INTO r1 VALUES (10)
3403+
ON CONFLICT (a) DO UPDATE SET a = 30;
3404+
ERROR: new row violates row-level security policy for table "r1"
3405+
INSERT INTO r1 VALUES (10)
3406+
ON CONFLICT ON CONSTRAINT r1_pkey DO UPDATE SET a = 30;
3407+
ERROR: new row violates row-level security policy for table "r1"
33953408
DROP TABLE r1;
33963409
-- Check dependency handling
33973410
RESET SESSION AUTHORIZATION;

‎src/test/regress/sql/privileges.sql

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -320,9 +320,24 @@ INSERT INTO atest5(two) VALUES (6) ON CONFLICT (two) DO UPDATE set three = EXCLU
320320
INSERT INTO atest5(two) VALUES (6) ON CONFLICT (two) DO UPDATE set three = EXCLUDED.three;
321321
INSERT INTO atest5(two) VALUES (6) ON CONFLICT (two) DO UPDATE set one = 8; -- fails (due to UPDATE)
322322
INSERT INTO atest5(three) VALUES (4) ON CONFLICT (two) DO UPDATE set three = 10; -- fails (due to INSERT)
323-
-- Check that the the columns in the inference require select privileges
324-
-- Error. No privs on four
325-
INSERT INTO atest5(three) VALUES (4) ON CONFLICT (four) DO UPDATE set three = 10;
323+
324+
-- Check that the columns in the inference require select privileges
325+
INSERT INTO atest5(four) VALUES (4); -- fail
326+
327+
SET SESSION AUTHORIZATION regressuser1;
328+
GRANT INSERT (four) ON atest5 TO regressuser4;
329+
SET SESSION AUTHORIZATION regressuser4;
330+
331+
INSERT INTO atest5(four) VALUES (4) ON CONFLICT (four) DO UPDATE set three = 3; -- fails (due to SELECT)
332+
INSERT INTO atest5(four) VALUES (4) ON CONFLICT ON CONSTRAINT atest5_four_key DO UPDATE set three = 3; -- fails (due to SELECT)
333+
INSERT INTO atest5(four) VALUES (4); -- ok
334+
335+
SET SESSION AUTHORIZATION regressuser1;
336+
GRANT SELECT (four) ON atest5 TO regressuser4;
337+
SET SESSION AUTHORIZATION regressuser4;
338+
339+
INSERT INTO atest5(four) VALUES (4) ON CONFLICT (four) DO UPDATE set three = 3; -- ok
340+
INSERT INTO atest5(four) VALUES (4) ON CONFLICT ON CONSTRAINT atest5_four_key DO UPDATE set three = 3; -- ok
326341

327342
SET SESSION AUTHORIZATION regressuser1;
328343
REVOKE ALL (one) ON atest5 FROM regressuser4;

‎src/test/regress/sql/rowsecurity.sql

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1487,10 +1487,11 @@ DROP TABLE r1;
14871487
--
14881488
SET SESSION AUTHORIZATION rls_regress_user0;
14891489
SET row_security = on;
1490-
CREATE TABLE r1 (a int);
1490+
CREATE TABLE r1 (a int PRIMARY KEY);
14911491

14921492
CREATE POLICY p1 ON r1 FOR SELECT USING (a < 20);
14931493
CREATE POLICY p2 ON r1 FOR UPDATE USING (a < 20) WITH CHECK (true);
1494+
CREATE POLICY p3 ON r1 FOR INSERT WITH CHECK (true);
14941495
INSERT INTO r1 VALUES (10);
14951496
ALTER TABLE r1 ENABLE ROW LEVEL SECURITY;
14961497
ALTER TABLE r1 FORCE ROW LEVEL SECURITY;
@@ -1512,6 +1513,17 @@ ALTER TABLE r1 FORCE ROW LEVEL SECURITY;
15121513
-- Error
15131514
UPDATE r1 SET a = 30 RETURNING *;
15141515

1516+
-- UPDATE path of INSERT ... ON CONFLICT DO UPDATE should also error out
1517+
INSERT INTO r1 VALUES (10)
1518+
ON CONFLICT (a) DO UPDATE SET a = 30 RETURNING *;
1519+
1520+
-- Should still error out without RETURNING (use of arbiter always requires
1521+
-- SELECT permissions)
1522+
INSERT INTO r1 VALUES (10)
1523+
ON CONFLICT (a) DO UPDATE SET a = 30;
1524+
INSERT INTO r1 VALUES (10)
1525+
ON CONFLICT ON CONSTRAINT r1_pkey DO UPDATE SET a = 30;
1526+
15151527
DROP TABLE r1;
15161528

15171529
-- Check dependency handling

0 commit comments

Comments
 (0)