On 10 January 2015 at 21:38, Dean Rasheed <dean.a.rash...@gmail.com> wrote:
> OK, I'll take a look at it. I started reading the existing code that
> expands RLS policies and spotted a couple of bugs that should be fixed
> first:-
>
> 1). In prepend_row_security_policies(), defaultDeny should be
> initialised to false at the top.
>
> 2). In fireRIRrules(), activeRIRs isn't being reset properly after
> recursing, which will cause a self-join to fail.
>
So as I starting looking into these bugs, which looked fairly trivial,
the test case I came up with revealed another more subtle bug with
RLS, using a more obscure query -- given an update of the form UPDATE
t1 ... FROM t2 ..., if t1 is part of an inheritance hierarchy and both
t1 and t2 have RLS enabled, the inheritance planner was doing the
wrong thing. There is code in there to copy subquery RTEs into each
child plan, which needed to do the same for non-target RTEs with
security barrier quals, which haven't yet been turned into subqueries.
Similarly the rowmark preprocessing code needed to be taught about
(non-target) RTEs with security barrier quals to handle this kind of
UPDATE with a FROM clause.
The attached patch fixes those issues.
This bug can't happen with updatable security barrier views, since
non-target security barrier views are expanded in the rewriter, so
technically this doesn't need bach-patching to 9.4. However, I think
the planner changes should probably be back-patched anyway, to keep
the code in the 2 branches in sync, and more maintainable. Also it
feels like the planner ought to be able to handle any legal query
thrown at it, even if it looks like the 9.4 rewriter couldn't generate
such a query.
Regards,
Dean
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
new file mode 100644
index 9cbbcfb..119a016
*** a/src/backend/optimizer/plan/planner.c
--- b/src/backend/optimizer/plan/planner.c
*************** inheritance_planner(PlannerInfo *root)
*** 790,795 ****
--- 790,796 ----
{
Query *parse = root->parse;
int parentRTindex = parse->resultRelation;
+ Bitmapset *resultRTindexes = NULL;
List *final_rtable = NIL;
int save_rel_array_size = 0;
RelOptInfo **save_rel_array = NULL;
*************** inheritance_planner(PlannerInfo *root)
*** 814,820 ****
--- 815,835 ----
* (1) would result in a rangetable of length O(N^2) for N targets, with
* at least O(N^3) work expended here; and (2) would greatly complicate
* management of the rowMarks list.
+ *
+ * Note that any RTEs with security barrier quals will be turned into
+ * subqueries during planning, and so we must create copies of them too,
+ * except where they are target relations, which will each only be used
+ * in a single plan.
*/
+ resultRTindexes = bms_add_member(resultRTindexes, parentRTindex);
+ foreach(lc, root->append_rel_list)
+ {
+ AppendRelInfo *appinfo = (AppendRelInfo *) lfirst(lc);
+ if (appinfo->parent_relid == parentRTindex)
+ resultRTindexes = bms_add_member(resultRTindexes,
+ appinfo->child_relid);
+ }
+
foreach(lc, root->append_rel_list)
{
AppendRelInfo *appinfo = (AppendRelInfo *) lfirst(lc);
*************** inheritance_planner(PlannerInfo *root)
*** 885,905 ****
{
RangeTblEntry *rte = (RangeTblEntry *) lfirst(lr);
! if (rte->rtekind == RTE_SUBQUERY)
{
Index newrti;
/*
* The RTE can't contain any references to its own RT
! * index, so we can save a few cycles by applying
! * ChangeVarNodes before we append the RTE to the
! * rangetable.
*/
newrti = list_length(subroot.parse->rtable) + 1;
ChangeVarNodes((Node *) subroot.parse, rti, newrti, 0);
ChangeVarNodes((Node *) subroot.rowMarks, rti, newrti, 0);
ChangeVarNodes((Node *) subroot.append_rel_list, rti, newrti, 0);
rte = copyObject(rte);
subroot.parse->rtable = lappend(subroot.parse->rtable,
rte);
}
--- 900,928 ----
{
RangeTblEntry *rte = (RangeTblEntry *) lfirst(lr);
! /*
! * Copy subquery RTEs and RTEs with security barrier quals
! * that will be turned into subqueries, except those that are
! * target relations.
! */
! if (rte->rtekind == RTE_SUBQUERY ||
! (rte->securityQuals != NIL &&
! !bms_is_member(rti, resultRTindexes)))
{
Index newrti;
/*
* The RTE can't contain any references to its own RT
! * index, except in the security barrier quals, so we can
! * save a few cycles by applying ChangeVarNodes before we
! * append the RTE to the rangetable.
*/
newrti = list_length(subroot.parse->rtable) + 1;
ChangeVarNodes((Node *) subroot.parse, rti, newrti, 0);
ChangeVarNodes((Node *) subroot.rowMarks, rti, newrti, 0);
ChangeVarNodes((Node *) subroot.append_rel_list, rti, newrti, 0);
rte = copyObject(rte);
+ ChangeVarNodes((Node *) rte->securityQuals, rti, newrti, 0);
subroot.parse->rtable = lappend(subroot.parse->rtable,
rte);
}
*************** preprocess_rowmarks(PlannerInfo *root)
*** 2198,2206 ****
* Ignore RowMarkClauses for subqueries; they aren't real tables and
* can't support true locking. Subqueries that got flattened into the
* main query should be ignored completely. Any that didn't will get
! * ROW_MARK_COPY items in the next loop.
*/
! if (rte->rtekind != RTE_RELATION)
continue;
/*
--- 2221,2231 ----
* Ignore RowMarkClauses for subqueries; they aren't real tables and
* can't support true locking. Subqueries that got flattened into the
* main query should be ignored completely. Any that didn't will get
! * ROW_MARK_COPY items in the next loop. Any RTEs with security
! * barrier quals will get turned into subqueries later, and so fall
! * into this same category.
*/
! if (rte->rtekind != RTE_RELATION || rte->securityQuals != NIL)
continue;
/*
*************** preprocess_rowmarks(PlannerInfo *root)
*** 2256,2261 ****
--- 2281,2287 ----
newrc->rowmarkId = ++(root->glob->lastRowMarkId);
/* real tables support REFERENCE, anything else needs COPY */
if (rte->rtekind == RTE_RELATION &&
+ rte->securityQuals == NIL &&
rte->relkind != RELKIND_FOREIGN_TABLE)
newrc->markType = ROW_MARK_REFERENCE;
else
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
new file mode 100644
index b8e6e7a..002f6dd
*** a/src/backend/rewrite/rewriteHandler.c
--- b/src/backend/rewrite/rewriteHandler.c
*************** fireRIRrules(Query *parsetree, List *act
*** 1756,1761 ****
--- 1756,1763 ----
expression_tree_walker( (Node*) quals, fireRIRonSubLink,
(void*)activeRIRs);
+
+ activeRIRs = list_delete_first(activeRIRs);
}
}
diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c
new file mode 100644
index 35790a9..4eb8d10
*** a/src/backend/rewrite/rowsecurity.c
--- b/src/backend/rewrite/rowsecurity.c
***************
*** 57,63 ****
static List *pull_row_security_policies(CmdType cmd, Relation relation,
Oid user_id);
! static void process_policies(List *policies, int rt_index,
Expr **final_qual,
Expr **final_with_check_qual,
bool *hassublinks);
--- 57,63 ----
static List *pull_row_security_policies(CmdType cmd, Relation relation,
Oid user_id);
! static void process_policies(Query* root, List *policies, int rt_index,
Expr **final_qual,
Expr **final_with_check_qual,
bool *hassublinks);
*************** prepend_row_security_policies(Query* roo
*** 95,101 ****
Oid user_id;
int sec_context;
int rls_status;
! bool defaultDeny = true;
bool hassublinks = false;
/* This is just to get the security context */
--- 95,101 ----
Oid user_id;
int sec_context;
int rls_status;
! bool defaultDeny = false;
bool hassublinks = false;
/* This is just to get the security context */
*************** prepend_row_security_policies(Query* roo
*** 166,172 ****
defaultDeny = true;
/* Now that we have our policies, build the expressions from them. */
! process_policies(rowsec_policies, rt_index, &rowsec_expr,
&rowsec_with_check_expr, &hassublinks);
/*
--- 166,172 ----
defaultDeny = true;
/* Now that we have our policies, build the expressions from them. */
! process_policies(root, rowsec_policies, rt_index, &rowsec_expr,
&rowsec_with_check_expr, &hassublinks);
/*
*************** prepend_row_security_policies(Query* roo
*** 196,202 ****
hook_policies = (*row_security_policy_hook)(root->commandType, rel);
/* Build the expression from any policies returned. */
! process_policies(hook_policies, rt_index, &hook_expr,
&hook_with_check_expr, &hassublinks);
}
--- 196,202 ----
hook_policies = (*row_security_policy_hook)(root->commandType, rel);
/* Build the expression from any policies returned. */
! process_policies(root, hook_policies, rt_index, &hook_expr,
&hook_with_check_expr, &hassublinks);
}
*************** pull_row_security_policies(CmdType cmd,
*** 383,389 ****
* qual_eval, with_check_eval, and hassublinks are output variables
*/
static void
! process_policies(List *policies, int rt_index, Expr **qual_eval,
Expr **with_check_eval, bool *hassublinks)
{
ListCell *item;
--- 383,389 ----
* qual_eval, with_check_eval, and hassublinks are output variables
*/
static void
! process_policies(Query* root, List *policies, int rt_index, Expr **qual_eval,
Expr **with_check_eval, bool *hassublinks)
{
ListCell *item;
*************** process_policies(List *policies, int rt_
*** 392,398 ****
/*
* Extract the USING and WITH CHECK quals from each of the policies
! * and add them to our lists.
*/
foreach(item, policies)
{
--- 392,399 ----
/*
* Extract the USING and WITH CHECK quals from each of the policies
! * and add them to our lists. We only want WITH CHECK quals if this
! * RTE is the query's result relation.
*/
foreach(item, policies)
{
*************** process_policies(List *policies, int rt_
*** 401,407 ****
if (policy->qual != NULL)
quals = lcons(copyObject(policy->qual), quals);
! if (policy->with_check_qual != NULL)
with_check_quals = lcons(copyObject(policy->with_check_qual),
with_check_quals);
--- 402,409 ----
if (policy->qual != NULL)
quals = lcons(copyObject(policy->qual), quals);
! if (policy->with_check_qual != NULL &&
! rt_index == root->resultRelation)
with_check_quals = lcons(copyObject(policy->with_check_qual),
with_check_quals);
*************** process_policies(List *policies, int rt_
*** 421,427 ****
* If we end up with only USING quals, then use those as
* WITH CHECK quals also.
*/
! if (with_check_quals == NIL)
with_check_quals = copyObject(quals);
/*
--- 423,429 ----
* If we end up with only USING quals, then use those as
* WITH CHECK quals also.
*/
! if (with_check_quals == NIL && rt_index == root->resultRelation)
with_check_quals = copyObject(quals);
/*
*************** process_policies(List *policies, int rt_
*** 450,457 ****
*/
if (list_length(with_check_quals) > 1)
*with_check_eval = makeBoolExpr(OR_EXPR, with_check_quals, -1);
! else
*with_check_eval = (Expr*) linitial(with_check_quals);
return;
}
--- 452,461 ----
*/
if (list_length(with_check_quals) > 1)
*with_check_eval = makeBoolExpr(OR_EXPR, with_check_quals, -1);
! else if (with_check_quals != NIL)
*with_check_eval = (Expr*) linitial(with_check_quals);
+ else
+ *with_check_eval = NULL;
return;
}
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
new file mode 100644
index 430da55..494c1e4
*** a/src/test/regress/expected/rowsecurity.out
--- b/src/test/regress/expected/rowsecurity.out
*************** SELECT * FROM t1 FOR SHARE;
*** 585,604 ****
(5 rows)
EXPLAIN (COSTS OFF) SELECT * FROM t1 FOR SHARE;
! QUERY PLAN
! -------------------------------------------------------
LockRows
-> Subquery Scan on t1
! -> LockRows
! -> Result
! -> Append
! -> Seq Scan on t1 t1_1
! Filter: ((a % 2) = 0)
! -> Seq Scan on t2
! Filter: ((a % 2) = 0)
! -> Seq Scan on t3
! Filter: ((a % 2) = 0)
! (11 rows)
SELECT * FROM t1 WHERE f_leak(b) FOR SHARE;
NOTICE: f_leak => bbb
--- 585,602 ----
(5 rows)
EXPLAIN (COSTS OFF) SELECT * FROM t1 FOR SHARE;
! QUERY PLAN
! -------------------------------------------
LockRows
-> Subquery Scan on t1
! -> Append
! -> Seq Scan on t1 t1_1
! Filter: ((a % 2) = 0)
! -> Seq Scan on t2
! Filter: ((a % 2) = 0)
! -> Seq Scan on t3
! Filter: ((a % 2) = 0)
! (9 rows)
SELECT * FROM t1 WHERE f_leak(b) FOR SHARE;
NOTICE: f_leak => bbb
*************** NOTICE: f_leak => yyy
*** 616,636 ****
(5 rows)
EXPLAIN (COSTS OFF) SELECT * FROM t1 WHERE f_leak(b) FOR SHARE;
! QUERY PLAN
! -------------------------------------------------------
LockRows
-> Subquery Scan on t1
Filter: f_leak(t1.b)
! -> LockRows
! -> Result
! -> Append
! -> Seq Scan on t1 t1_1
! Filter: ((a % 2) = 0)
! -> Seq Scan on t2
! Filter: ((a % 2) = 0)
! -> Seq Scan on t3
! Filter: ((a % 2) = 0)
! (12 rows)
-- superuser is allowed to bypass RLS checks
RESET SESSION AUTHORIZATION;
--- 614,632 ----
(5 rows)
EXPLAIN (COSTS OFF) SELECT * FROM t1 WHERE f_leak(b) FOR SHARE;
! QUERY PLAN
! -------------------------------------------
LockRows
-> Subquery Scan on t1
Filter: f_leak(t1.b)
! -> Append
! -> Seq Scan on t1 t1_1
! Filter: ((a % 2) = 0)
! -> Seq Scan on t2
! Filter: ((a % 2) = 0)
! -> Seq Scan on t3
! Filter: ((a % 2) = 0)
! (10 rows)
-- superuser is allowed to bypass RLS checks
RESET SESSION AUTHORIZATION;
*************** NOTICE: f_leak => yyyyyy
*** 1111,1116 ****
--- 1107,1181 ----
302 | 2 | yyyyyy | (2,yyyyyy)
(5 rows)
+ -- update with from clause self join
+ EXPLAIN (COSTS OFF) UPDATE t1 t1_1 SET b = t1_2.b FROM t1 t1_2
+ WHERE t1_1.a = 4 AND t1_2.a = t1_1.a AND t1_2.b = t1_1.b
+ AND f_leak(t1_1.b) AND f_leak(t1_2.b) RETURNING *, t1_1, t1_2;
+ QUERY PLAN
+ ---------------------------------------------------------------
+ Update on t1 t1_1_3
+ -> Nested Loop
+ Join Filter: (t1_1.b = t1_2.b)
+ -> Subquery Scan on t1_1
+ Filter: f_leak(t1_1.b)
+ -> Seq Scan on t1 t1_1_4
+ Filter: ((a = 4) AND ((a % 2) = 0))
+ -> Subquery Scan on t1_2
+ Filter: f_leak(t1_2.b)
+ -> Append
+ -> Seq Scan on t1 t1_2_3
+ Filter: ((a = 4) AND ((a % 2) = 0))
+ -> Seq Scan on t2 t1_2_4
+ Filter: ((a = 4) AND ((a % 2) = 0))
+ -> Seq Scan on t3 t1_2_5
+ Filter: ((a = 4) AND ((a % 2) = 0))
+ -> Nested Loop
+ Join Filter: (t1_1_1.b = t1_2_1.b)
+ -> Subquery Scan on t1_1_1
+ Filter: f_leak(t1_1_1.b)
+ -> Seq Scan on t2 t1_1_5
+ Filter: ((a = 4) AND ((a % 2) = 0))
+ -> Subquery Scan on t1_2_1
+ Filter: f_leak(t1_2_1.b)
+ -> Append
+ -> Seq Scan on t1 t1_2_6
+ Filter: ((a = 4) AND ((a % 2) = 0))
+ -> Seq Scan on t2 t1_2_7
+ Filter: ((a = 4) AND ((a % 2) = 0))
+ -> Seq Scan on t3 t1_2_8
+ Filter: ((a = 4) AND ((a % 2) = 0))
+ -> Nested Loop
+ Join Filter: (t1_1_2.b = t1_2_2.b)
+ -> Subquery Scan on t1_1_2
+ Filter: f_leak(t1_1_2.b)
+ -> Seq Scan on t3 t1_1_6
+ Filter: ((a = 4) AND ((a % 2) = 0))
+ -> Subquery Scan on t1_2_2
+ Filter: f_leak(t1_2_2.b)
+ -> Append
+ -> Seq Scan on t1 t1_2_9
+ Filter: ((a = 4) AND ((a % 2) = 0))
+ -> Seq Scan on t2 t1_2_10
+ Filter: ((a = 4) AND ((a % 2) = 0))
+ -> Seq Scan on t3 t1_2_11
+ Filter: ((a = 4) AND ((a % 2) = 0))
+ (46 rows)
+
+ UPDATE t1 t1_1 SET b = t1_2.b FROM t1 t1_2
+ WHERE t1_1.a = 4 AND t1_2.a = t1_1.a AND t1_2.b = t1_1.b
+ AND f_leak(t1_1.b) AND f_leak(t1_2.b) RETURNING *, t1_1, t1_2;
+ NOTICE: f_leak => dddddd_updt
+ NOTICE: f_leak => dddddd_updt
+ NOTICE: f_leak => defdef
+ NOTICE: f_leak => defdef
+ NOTICE: f_leak => dddddd_updt
+ NOTICE: f_leak => defdef
+ a | b | a | b | t1_1 | t1_2
+ ---+-------------+---+-------------+-----------------+-----------------
+ 4 | dddddd_updt | 4 | dddddd_updt | (4,dddddd_updt) | (4,dddddd_updt)
+ 4 | defdef | 4 | defdef | (4,defdef) | (4,defdef)
+ (2 rows)
+
RESET SESSION AUTHORIZATION;
SET row_security TO OFF;
SELECT * FROM t1;
diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql
new file mode 100644
index ee28a2c..274a3c4
*** a/src/test/regress/sql/rowsecurity.sql
--- b/src/test/regress/sql/rowsecurity.sql
*************** UPDATE only t1 SET b = b WHERE f_leak(b)
*** 423,428 ****
--- 423,437 ----
UPDATE t1 SET b = b WHERE f_leak(b) RETURNING *;
UPDATE t1 SET b = b WHERE f_leak(b) RETURNING oid, *, t1;
+ -- update with from clause self join
+ EXPLAIN (COSTS OFF) UPDATE t1 t1_1 SET b = t1_2.b FROM t1 t1_2
+ WHERE t1_1.a = 4 AND t1_2.a = t1_1.a AND t1_2.b = t1_1.b
+ AND f_leak(t1_1.b) AND f_leak(t1_2.b) RETURNING *, t1_1, t1_2;
+
+ UPDATE t1 t1_1 SET b = t1_2.b FROM t1 t1_2
+ WHERE t1_1.a = 4 AND t1_2.a = t1_1.a AND t1_2.b = t1_1.b
+ AND f_leak(t1_1.b) AND f_leak(t1_2.b) RETURNING *, t1_1, t1_2;
+
RESET SESSION AUTHORIZATION;
SET row_security TO OFF;
SELECT * FROM t1;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
