* Robert Haas (robertmh...@gmail.com) wrote: > On Tue, Sep 15, 2015 at 10:22 AM, Stephen Frost <sfr...@snowman.net> wrote: > > Unless there are other concerns or issues raised, I'll push this later > > today. > > So does this mean that the first RLS open item is addressed? If so, > can it be moved to the "resolved after 9.5alpha2" section? Based on > commit 4f3b2a8883c47b6710152a8e157f8a02656d0e68 I *think* yes but...
I hadn't moved it because there was ongoing discussion and I had an open item (see: 20150923185403.gc3...@tamriel.snowman.net and the thread leading up to it). Attached is a patch to address exactly that issue. This is all in the commit message, of course, but the gist of it is: If SELECT rights are required then apply the SELECT policies, even if the actual command is an UPDATE or DELETE. This covers the RETURNING case which was discussed previously, so we don't need the explicit check for that, and further addresses the concern raised by Zhaomo about someone abusing the WHERE clause in an UPDATE or DELETE. Further, if UPDATE rights are required then apply the UPDATE policies, even if the actual command is a SELECT. This addresses the concern that a user might be able to lock rows they're not actually allowed to UPDATE through the UPDATE policies. Comments welcome, of course. Barring concerns, I'll get this pushed tomorrow. Thanks! Stephen
From b8ad8ca34ad473813b072cc871ad33c48ba68b42 Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfr...@snowman.net>
Date: Mon, 28 Sep 2015 13:25:28 -0400
Subject: [PATCH] Include policies based on ACLs needed
When considering which policies should be included, rather than look at
individual bits of the query (eg: if a RETURNING clause exists, or if a
WHERE clause exists which is referencing the table, or if it's a
FOR SHARE/UPDATE query), consider any case where we've determined
the user needs SELECT rights on the relation to be a case where we apply
SELECT policies, and any case where we've deteremind that the user needs
UPDATE rights on the relation to be a case where we apply UPDATE
policies.
This simplifies the logic and addresses concerns that a user could use
UPDATE or DELETE with a WHERE clauses to determine if rows exist, or
they could lock rows which they are not actually allowed to modify
through UPDATE policies.
Back-patch to 9.5 where RLS was added.
---
src/backend/rewrite/rowsecurity.c | 76 +++++++++-----
src/test/regress/expected/rowsecurity.out | 166 ++++++++++++++++++------------
2 files changed, 150 insertions(+), 92 deletions(-)
diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c
index c20a178..1a51c20 100644
--- a/src/backend/rewrite/rowsecurity.c
+++ b/src/backend/rewrite/rowsecurity.c
@@ -187,28 +187,55 @@ get_row_security_policies(Query *root, RangeTblEntry *rte, int rt_index,
hasSubLinks);
/*
- * For the target relation, when there is a returning list, we need to
- * collect up CMD_SELECT policies and add them via add_security_quals.
- * This is because, for the RETURNING case, we have to filter any records
- * which are not visible through an ALL or SELECT USING policy.
+ * For a SELECT, if UPDATE privileges are required (eg: the user has
+ * specified FOR [KEY] UPDATE/SHARE), then also add the UPDATE USING quals.
*
- * We don't need to worry about the non-target relation case because we are
- * checking the ALL and SELECT policies for those relations anyway (see
- * above).
+ * This way, we filter out any records from the SELECT FOR SHARE/UPDATE
+ * which the user does not have access to via the UPDATE USING policies,
+ * similar to how we require normal UPDATE rights for these queries.
*/
- if (root->returningList != NIL &&
- (commandType == CMD_UPDATE || commandType == CMD_DELETE))
+ if (commandType == CMD_SELECT && rte->requiredPerms & ACL_UPDATE)
{
- List *returning_permissive_policies;
- List *returning_restrictive_policies;
+ List *update_permissive_policies;
+ List *update_restrictive_policies;
+
+ get_policies_for_relation(rel, CMD_UPDATE, user_id,
+ &update_permissive_policies,
+ &update_restrictive_policies);
+
+ add_security_quals(rt_index,
+ update_permissive_policies,
+ update_restrictive_policies,
+ securityQuals,
+ hasSubLinks);
+ }
+
+ /*
+ * Similar to above, during an UPDATE or DELETE, if SELECT rights are also
+ * required (eg: when a RETURNING clause exists, or the user has provided
+ * a WHERE clause which involves columns from the relation), we collect up
+ * CMD_SELECT policies and add them via add_security_quals.
+ *
+ * This way, we filter out any records which are not visible through an ALL
+ * or SELECT USING policy.
+ *
+ * We don't need to worry about the non-target relation case here because
+ * we are checking the ALL and SELECT policies for those relations anyway
+ * (see above).
+ */
+ if ((commandType == CMD_UPDATE || commandType == CMD_DELETE) &&
+ rte->requiredPerms & ACL_SELECT)
+ {
+ List *select_permissive_policies;
+ List *select_restrictive_policies;
get_policies_for_relation(rel, CMD_SELECT, user_id,
- &returning_permissive_policies,
- &returning_restrictive_policies);
+ &select_permissive_policies,
+ &select_restrictive_policies);
add_security_quals(rt_index,
- returning_permissive_policies,
- returning_restrictive_policies,
+ select_permissive_policies,
+ select_restrictive_policies,
securityQuals,
hasSubLinks);
}
@@ -261,21 +288,22 @@ get_row_security_policies(Query *root, RangeTblEntry *rte, int rt_index,
hasSubLinks);
/*
- * Get and add ALL/SELECT policies, if there is a RETURNING clause,
- * also as WCO policies, again, to avoid silently dropping data.
+ * Get and add ALL/SELECT policies, if SELECT rights are required
+ * for this relation, also as WCO policies, again, to avoid
+ * silently dropping data. See above.
*/
- if (root->returningList != NIL)
+ if (rte->requiredPerms & ACL_SELECT)
{
- List *conflict_returning_permissive_policies = NIL;
- List *conflict_returning_restrictive_policies = NIL;
+ List *conflict_select_permissive_policies = NIL;
+ List *conflict_select_restrictive_policies = NIL;
get_policies_for_relation(rel, CMD_SELECT, user_id,
- &conflict_returning_permissive_policies,
- &conflict_returning_restrictive_policies);
+ &conflict_select_permissive_policies,
+ &conflict_select_restrictive_policies);
add_with_check_options(rel, rt_index,
WCO_RLS_CONFLICT_CHECK,
- conflict_returning_permissive_policies,
- conflict_returning_restrictive_policies,
+ conflict_select_permissive_policies,
+ conflict_select_restrictive_policies,
withCheckOptions,
hasSubLinks);
}
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
index 9d3540f..b4e64ab 100644
--- a/src/test/regress/expected/rowsecurity.out
+++ b/src/test/regress/expected/rowsecurity.out
@@ -1043,28 +1043,34 @@ EXPLAIN (COSTS OFF) EXECUTE p2(2);
--
SET SESSION AUTHORIZATION rls_regress_user1;
EXPLAIN (COSTS OFF) UPDATE t1 SET b = b || b WHERE f_leak(b);
- QUERY PLAN
--------------------------------------------
+ QUERY PLAN
+-------------------------------------------------
Update on t1 t1_3
Update on t1 t1_3
Update on t2 t1
Update on t3 t1
-> Subquery Scan on t1
Filter: f_leak(t1.b)
- -> LockRows
- -> Seq Scan on t1 t1_4
- Filter: ((a % 2) = 0)
+ -> Subquery Scan on t1_4
+ Filter: ((t1_4.a % 2) = 0)
+ -> LockRows
+ -> Seq Scan on t1 t1_5
+ Filter: ((a % 2) = 0)
-> Subquery Scan on t1_1
Filter: f_leak(t1_1.b)
- -> LockRows
- -> Seq Scan on t2
- Filter: ((a % 2) = 0)
+ -> Subquery Scan on t1_6
+ Filter: ((t1_6.a % 2) = 0)
+ -> LockRows
+ -> Seq Scan on t2
+ Filter: ((a % 2) = 0)
-> Subquery Scan on t1_2
Filter: f_leak(t1_2.b)
- -> LockRows
- -> Seq Scan on t3
- Filter: ((a % 2) = 0)
-(19 rows)
+ -> Subquery Scan on t1_7
+ Filter: ((t1_7.a % 2) = 0)
+ -> LockRows
+ -> Seq Scan on t3
+ Filter: ((a % 2) = 0)
+(25 rows)
UPDATE t1 SET b = b || b WHERE f_leak(b);
NOTICE: f_leak => bbb
@@ -1073,15 +1079,17 @@ NOTICE: f_leak => bcd
NOTICE: f_leak => def
NOTICE: f_leak => yyy
EXPLAIN (COSTS OFF) UPDATE only t1 SET b = b || '_updt' WHERE f_leak(b);
- QUERY PLAN
--------------------------------------------
+ QUERY PLAN
+-------------------------------------------------
Update on t1 t1_1
-> Subquery Scan on t1
Filter: f_leak(t1.b)
- -> LockRows
- -> Seq Scan on t1 t1_2
- Filter: ((a % 2) = 0)
-(6 rows)
+ -> Subquery Scan on t1_2
+ Filter: ((t1_2.a % 2) = 0)
+ -> LockRows
+ -> Seq Scan on t1 t1_3
+ Filter: ((a % 2) = 0)
+(8 rows)
UPDATE only t1 SET b = b || '_updt' WHERE f_leak(b);
NOTICE: f_leak => bbbbbb
@@ -1129,18 +1137,20 @@ NOTICE: f_leak => yyyyyy
-- updates with from clause
EXPLAIN (COSTS OFF) UPDATE t2 SET b=t2.b FROM t3
WHERE t2.a = 3 and t3.a = 2 AND f_leak(t2.b) AND f_leak(t3.b);
- QUERY PLAN
----------------------------------------------------------------
+ QUERY PLAN
+---------------------------------------------------------------------
Update on t2 t2_1
-> Nested Loop
-> Subquery Scan on t2
Filter: f_leak(t2.b)
- -> LockRows
- -> Seq Scan on t2 t2_2
- Filter: ((a = 3) AND ((a % 2) = 1))
+ -> Subquery Scan on t2_2
+ Filter: ((t2_2.a % 2) = 1)
+ -> LockRows
+ -> Seq Scan on t2 t2_3
+ Filter: ((a = 3) AND ((a % 2) = 1))
-> Seq Scan on t3
Filter: (f_leak(b) AND (a = 2))
-(9 rows)
+(11 rows)
UPDATE t2 SET b=t2.b FROM t3
WHERE t2.a = 3 and t3.a = 2 AND f_leak(t2.b) AND f_leak(t3.b);
@@ -1150,8 +1160,8 @@ NOTICE: f_leak => zzz
NOTICE: f_leak => yyyyyy
EXPLAIN (COSTS OFF) UPDATE t1 SET b=t1.b FROM t2
WHERE t1.a = 3 and t2.a = 3 AND f_leak(t1.b) AND f_leak(t2.b);
- QUERY PLAN
----------------------------------------------------------------
+ QUERY PLAN
+---------------------------------------------------------------------
Update on t1 t1_3
Update on t1 t1_3
Update on t2 t1
@@ -1159,9 +1169,11 @@ WHERE t1.a = 3 and t2.a = 3 AND f_leak(t1.b) AND f_leak(t2.b);
-> Nested Loop
-> Subquery Scan on t1
Filter: f_leak(t1.b)
- -> LockRows
- -> Seq Scan on t1 t1_4
- Filter: ((a = 3) AND ((a % 2) = 0))
+ -> Subquery Scan on t1_4
+ Filter: ((t1_4.a % 2) = 0)
+ -> LockRows
+ -> Seq Scan on t1 t1_5
+ Filter: ((a = 3) AND ((a % 2) = 0))
-> Subquery Scan on t2
Filter: f_leak(t2.b)
-> Seq Scan on t2 t2_3
@@ -1169,9 +1181,11 @@ WHERE t1.a = 3 and t2.a = 3 AND f_leak(t1.b) AND f_leak(t2.b);
-> Nested Loop
-> Subquery Scan on t1_1
Filter: f_leak(t1_1.b)
- -> LockRows
- -> Seq Scan on t2 t2_4
- Filter: ((a = 3) AND ((a % 2) = 0))
+ -> Subquery Scan on t1_6
+ Filter: ((t1_6.a % 2) = 0)
+ -> LockRows
+ -> Seq Scan on t2 t2_4
+ Filter: ((a = 3) AND ((a % 2) = 0))
-> Subquery Scan on t2_1
Filter: f_leak(t2_1.b)
-> Seq Scan on t2 t2_5
@@ -1179,14 +1193,16 @@ WHERE t1.a = 3 and t2.a = 3 AND f_leak(t1.b) AND f_leak(t2.b);
-> Nested Loop
-> Subquery Scan on t1_2
Filter: f_leak(t1_2.b)
- -> LockRows
- -> Seq Scan on t3
- Filter: ((a = 3) AND ((a % 2) = 0))
+ -> Subquery Scan on t1_7
+ Filter: ((t1_7.a % 2) = 0)
+ -> LockRows
+ -> Seq Scan on t3
+ Filter: ((a = 3) AND ((a % 2) = 0))
-> Subquery Scan on t2_2
Filter: f_leak(t2_2.b)
-> Seq Scan on t2 t2_6
Filter: ((a = 3) AND ((a % 2) = 1))
-(34 rows)
+(40 rows)
UPDATE t1 SET b=t1.b FROM t2
WHERE t1.a = 3 and t2.a = 3 AND f_leak(t1.b) AND f_leak(t2.b);
@@ -1198,20 +1214,22 @@ WHERE t1.a = 3 and t2.a = 3 AND f_leak(t1.b) AND f_leak(t2.b);
-> Nested Loop
-> Subquery Scan on t2
Filter: f_leak(t2.b)
- -> LockRows
- -> Seq Scan on t2 t2_2
- Filter: ((a = 3) AND ((a % 2) = 1))
+ -> Subquery Scan on t2_2
+ Filter: ((t2_2.a % 2) = 1)
+ -> LockRows
+ -> Seq Scan on t2 t2_3
+ Filter: ((a = 3) AND ((a % 2) = 1))
-> Subquery Scan on t1
Filter: f_leak(t1.b)
-> Result
-> Append
-> Seq Scan on t1 t1_1
Filter: ((a = 3) AND ((a % 2) = 0))
- -> Seq Scan on t2 t2_3
+ -> Seq Scan on t2 t2_4
Filter: ((a = 3) AND ((a % 2) = 0))
-> Seq Scan on t3
Filter: ((a = 3) AND ((a % 2) = 0))
-(17 rows)
+(19 rows)
UPDATE t2 SET b=t2.b FROM t1
WHERE t1.a = 3 and t2.a = 3 AND f_leak(t1.b) AND f_leak(t2.b);
@@ -1349,39 +1367,47 @@ SELECT * FROM t1 ORDER BY a,b;
SET SESSION AUTHORIZATION rls_regress_user1;
SET row_security TO ON;
EXPLAIN (COSTS OFF) DELETE FROM only t1 WHERE f_leak(b);
- QUERY PLAN
--------------------------------------------
+ QUERY PLAN
+-------------------------------------------------
Delete on t1 t1_1
-> Subquery Scan on t1
Filter: f_leak(t1.b)
- -> LockRows
- -> Seq Scan on t1 t1_2
- Filter: ((a % 2) = 0)
-(6 rows)
+ -> Subquery Scan on t1_2
+ Filter: ((t1_2.a % 2) = 0)
+ -> LockRows
+ -> Seq Scan on t1 t1_3
+ Filter: ((a % 2) = 0)
+(8 rows)
EXPLAIN (COSTS OFF) DELETE FROM t1 WHERE f_leak(b);
- QUERY PLAN
--------------------------------------------
+ QUERY PLAN
+-------------------------------------------------
Delete on t1 t1_3
Delete on t1 t1_3
Delete on t2 t1
Delete on t3 t1
-> Subquery Scan on t1
Filter: f_leak(t1.b)
- -> LockRows
- -> Seq Scan on t1 t1_4
- Filter: ((a % 2) = 0)
+ -> Subquery Scan on t1_4
+ Filter: ((t1_4.a % 2) = 0)
+ -> LockRows
+ -> Seq Scan on t1 t1_5
+ Filter: ((a % 2) = 0)
-> Subquery Scan on t1_1
Filter: f_leak(t1_1.b)
- -> LockRows
- -> Seq Scan on t2
- Filter: ((a % 2) = 0)
+ -> Subquery Scan on t1_6
+ Filter: ((t1_6.a % 2) = 0)
+ -> LockRows
+ -> Seq Scan on t2
+ Filter: ((a % 2) = 0)
-> Subquery Scan on t1_2
Filter: f_leak(t1_2.b)
- -> LockRows
- -> Seq Scan on t3
- Filter: ((a % 2) = 0)
-(19 rows)
+ -> Subquery Scan on t1_7
+ Filter: ((t1_7.a % 2) = 0)
+ -> LockRows
+ -> Seq Scan on t3
+ Filter: ((a % 2) = 0)
+(25 rows)
DELETE FROM only t1 WHERE f_leak(b) RETURNING oid, *, t1;
NOTICE: f_leak => bbbbbb_updt
@@ -1452,10 +1478,11 @@ EXPLAIN (COSTS OFF) UPDATE bv1 SET b = 'yyy' WHERE a = 4 AND f_leak(b);
-> Subquery Scan on b1
Filter: f_leak(b1.b)
-> Subquery Scan on b1_2
+ Filter: ((b1_2.a % 2) = 0)
-> LockRows
-> Seq Scan on b1 b1_3
Filter: ((a > 0) AND (a = 4) AND ((a % 2) = 0))
-(7 rows)
+(8 rows)
UPDATE bv1 SET b = 'yyy' WHERE a = 4 AND f_leak(b);
NOTICE: f_leak => a87ff679a2f3e71d9181a67b7542122c
@@ -1466,10 +1493,11 @@ EXPLAIN (COSTS OFF) DELETE FROM bv1 WHERE a = 6 AND f_leak(b);
-> Subquery Scan on b1
Filter: f_leak(b1.b)
-> Subquery Scan on b1_2
+ Filter: ((b1_2.a % 2) = 0)
-> LockRows
-> Seq Scan on b1 b1_3
Filter: ((a > 0) AND (a = 6) AND ((a % 2) = 0))
-(7 rows)
+(8 rows)
DELETE FROM bv1 WHERE a = 6 AND f_leak(b);
NOTICE: f_leak => 1679091c5a880faf6fb5e6087eb1b2dc
@@ -2743,15 +2771,17 @@ SELECT * FROM current_check;
-- Plan should be a subquery TID scan
EXPLAIN (COSTS OFF) UPDATE current_check SET payload = payload WHERE CURRENT OF current_check_cursor;
- QUERY PLAN
----------------------------------------------------------------
+ QUERY PLAN
+---------------------------------------------------------------------
Update on current_check current_check_1
-> Subquery Scan on current_check
- -> LockRows
- -> Tid Scan on current_check current_check_2
- TID Cond: CURRENT OF current_check_cursor
- Filter: (currentid = 4)
-(6 rows)
+ -> Subquery Scan on current_check_2
+ Filter: ((current_check_2.currentid % 2) = 0)
+ -> LockRows
+ -> Tid Scan on current_check current_check_3
+ TID Cond: CURRENT OF current_check_cursor
+ Filter: (currentid = 4)
+(8 rows)
-- Similarly can only delete row 4
FETCH ABSOLUTE 1 FROM current_check_cursor;
--
1.9.1
signature.asc
Description: Digital signature
