Skip to content

Commit 0bf22e0

Browse files
sfrostsfrost
committed
RLS fixes, new hooks, and new test module
In prepend_row_security_policies(), defaultDeny was always true, so if there were any hook policies, the RLS policies on the table would just get discarded. Fixed to start off with defaultDeny as false and then properly set later if we detect that only the default deny policy exists for the internal policies. The infinite recursion detection in fireRIRrules() didn't properly manage the activeRIRs list in the case of WCOs, so it would incorrectly report infinite recusion if the same relation with RLS appeared more than once in the rtable, for example "UPDATE t ... FROM t ...". Further, the RLS expansion code in fireRIRrules() was handling RLS in the main loop through the rtable, which lead to RTEs being visited twice if they contained sublink subqueries, which prepend_row_security_policies() attempted to handle by exiting early if the RTE already had securityQuals. That doesn't work, however, since if the query involved a security barrier view on top of a table with RLS, the RTE would already have securityQuals (from the view) by the time fireRIRrules() was invoked, and so the table's RLS policies would be ignored. This is fixed in fireRIRrules() by handling RLS in a separate loop at the end, after dealing with any other sublink subqueries, thus ensuring that each RTE is only visited once for RLS expansion. The inheritance planner code didn't correctly handle non-target relations with RLS, which would get turned into subqueries during planning. Thus an update of the form "UPDATE t1 ... FROM t2 ..." where t1 has inheritance and t2 has RLS quals would fail. Fix by making sure to copy in and update the securityQuals when they exist for non-target relations. process_policies() was adding WCOs to non-target relations, which is unnecessary, and could lead to a lot of wasted time in the rewriter and the planner. Fix by only adding WCO policies when working on the result relation. Also in process_policies, we should be copying the USING policies to the WITH CHECK policies on a per-policy basis, fix by moving the copying up into the per-policy loop. Lastly, as noted by Dean, we were simply adding policies returned by the hook provided to the list of quals being AND'd, meaning that they would actually restrict records returned and there was no option to have internal policies and hook-based policies work together permissively (as all internal policies currently work). Instead, explicitly add support for both permissive and restrictive policies by having a hook for each and combining the results appropriately. To ensure this is all done correctly, add a new test module (test_rls_hooks) to test the various combinations of internal, permissive, and restrictive hook policies. Largely from Dean Rasheed (thanks!): CAEZATCVmFUfUOwwhnBTcgi6AquyjQ0-1fyKd0T3xBWJvn+xsFA@mail.gmail.com Author: Dean Rasheed, though I added the new hooks and test module.
1 parent 4ccc5bd commit 0bf22e0

File tree

16 files changed

+1271
-147
lines changed

16 files changed

+1271
-147
lines changed

‎src/backend/optimizer/plan/planner.c

Lines changed: 40 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -790,6 +790,7 @@ inheritance_planner(PlannerInfo *root)
790790
{
791791
Query *parse = root->parse;
792792
int parentRTindex = parse->resultRelation;
793+
Bitmapset *resultRTindexes = NULL;
793794
int nominalRelation = -1;
794795
List *final_rtable = NIL;
795796
int save_rel_array_size = 0;
@@ -815,7 +816,21 @@ inheritance_planner(PlannerInfo *root)
815816
* (1) would result in a rangetable of length O(N^2) for N targets, with
816817
* at least O(N^3) work expended here; and (2) would greatly complicate
817818
* management of the rowMarks list.
819+
*
820+
* Note that any RTEs with security barrier quals will be turned into
821+
* subqueries during planning, and so we must create copies of them too,
822+
* except where they are target relations, which will each only be used
823+
* in a single plan.
818824
*/
825+
resultRTindexes = bms_add_member(resultRTindexes, parentRTindex);
826+
foreach(lc, root->append_rel_list)
827+
{
828+
AppendRelInfo *appinfo = (AppendRelInfo *) lfirst(lc);
829+
if (appinfo->parent_relid == parentRTindex)
830+
resultRTindexes = bms_add_member(resultRTindexes,
831+
appinfo->child_relid);
832+
}
833+
819834
foreach(lc, root->append_rel_list)
820835
{
821836
AppendRelInfo *appinfo = (AppendRelInfo *) lfirst(lc);
@@ -886,21 +901,29 @@ inheritance_planner(PlannerInfo *root)
886901
{
887902
RangeTblEntry *rte = (RangeTblEntry *) lfirst(lr);
888903

889-
if (rte->rtekind == RTE_SUBQUERY)
904+
/*
905+
* Copy subquery RTEs and RTEs with security barrier quals
906+
* that will be turned into subqueries, except those that are
907+
* target relations.
908+
*/
909+
if (rte->rtekind == RTE_SUBQUERY ||
910+
(rte->securityQuals != NIL &&
911+
!bms_is_member(rti, resultRTindexes)))
890912
{
891913
Index newrti;
892914

893915
/*
894916
* The RTE can't contain any references to its own RT
895-
* index, so we can save a few cycles by applying
896-
* ChangeVarNodes before we append the RTE to the
897-
* rangetable.
917+
* index, except in the security barrier quals, so we can
918+
* save a few cycles by applying ChangeVarNodes before we
919+
* append the RTE to the rangetable.
898920
*/
899921
newrti = list_length(subroot.parse->rtable) + 1;
900922
ChangeVarNodes((Node *) subroot.parse, rti, newrti, 0);
901923
ChangeVarNodes((Node *) subroot.rowMarks, rti, newrti, 0);
902924
ChangeVarNodes((Node *) subroot.append_rel_list, rti, newrti, 0);
903925
rte = copyObject(rte);
926+
ChangeVarNodes((Node *) rte->securityQuals, rti, newrti, 0);
904927
subroot.parse->rtable = lappend(subroot.parse->rtable,
905928
rte);
906929
}
@@ -2283,7 +2306,19 @@ select_rowmark_type(RangeTblEntry *rte, LockClauseStrength strength)
22832306
switch (strength)
22842307
{
22852308
case LCS_NONE:
2286-
/* don't need tuple lock, only ability to re-fetch the row */
2309+
/*
2310+
* We don't need a tuple lock, only the ability to re-fetch
2311+
* the row. Regular tables support ROW_MARK_REFERENCE, but if
2312+
* this RTE has security barrier quals, it will be turned into
2313+
* a subquery during planning, so use ROW_MARK_COPY.
2314+
*
2315+
* This is only necessary for LCS_NONE, since real tuple locks
2316+
* on an RTE with security barrier quals are supported by
2317+
* pushing the lock down into the subquery --- see
2318+
* expand_security_qual.
2319+
*/
2320+
if (rte->securityQuals != NIL)
2321+
return ROW_MARK_COPY;
22872322
return ROW_MARK_REFERENCE;
22882323
break;
22892324
case LCS_FORKEYSHARE:

‎src/backend/rewrite/rewriteHandler.c

Lines changed: 82 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -1714,51 +1714,6 @@ fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown)
17141714
activeRIRs = list_delete_first(activeRIRs);
17151715
}
17161716
}
1717-
/*
1718-
* If the RTE has row security quals, apply them and recurse into the
1719-
* securityQuals.
1720-
*/
1721-
if (prepend_row_security_policies(parsetree, rte, rt_index))
1722-
{
1723-
/*
1724-
* We applied security quals, check for infinite recursion and
1725-
* then expand any nested queries.
1726-
*/
1727-
if (list_member_oid(activeRIRs, RelationGetRelid(rel)))
1728-
ereport(ERROR,
1729-
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
1730-
errmsg("infinite recursion detected in policy for relation \"%s\"",
1731-
RelationGetRelationName(rel))));
1732-
1733-
/*
1734-
* Make sure we check for recursion in either securityQuals or
1735-
* WITH CHECK quals.
1736-
*/
1737-
if (rte->securityQuals != NIL)
1738-
{
1739-
activeRIRs = lcons_oid(RelationGetRelid(rel), activeRIRs);
1740-
1741-
expression_tree_walker( (Node*) rte->securityQuals,
1742-
fireRIRonSubLink, (void*)activeRIRs );
1743-
1744-
activeRIRs = list_delete_first(activeRIRs);
1745-
}
1746-
1747-
if (parsetree->withCheckOptions != NIL)
1748-
{
1749-
WithCheckOption *wco;
1750-
List *quals = NIL;
1751-
1752-
wco = (WithCheckOption *) makeNode(WithCheckOption);
1753-
quals = lcons(wco->qual, quals);
1754-
1755-
activeRIRs = lcons_oid(RelationGetRelid(rel), activeRIRs);
1756-
1757-
expression_tree_walker( (Node*) quals, fireRIRonSubLink,
1758-
(void*)activeRIRs);
1759-
}
1760-
1761-
}
17621717

17631718
heap_close(rel, NoLock);
17641719
}
@@ -1780,6 +1735,88 @@ fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown)
17801735
query_tree_walker(parsetree, fireRIRonSubLink, (void *) activeRIRs,
17811736
QTW_IGNORE_RC_SUBQUERIES);
17821737

1738+
/*
1739+
* Apply any row level security policies. We do this last because it
1740+
* requires special recursion detection if the new quals have sublink
1741+
* subqueries, and if we did it in the loop above query_tree_walker
1742+
* would then recurse into those quals a second time.
1743+
*/
1744+
rt_index = 0;
1745+
foreach(lc, parsetree->rtable)
1746+
{
1747+
RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc);
1748+
Relation rel;
1749+
List *securityQuals;
1750+
List *withCheckOptions;
1751+
bool hasRowSecurity;
1752+
bool hasSubLinks;
1753+
1754+
++rt_index;
1755+
1756+
/* Only normal relations can have RLS policies */
1757+
if (rte->rtekind != RTE_RELATION ||
1758+
rte->relkind != RELKIND_RELATION)
1759+
continue;
1760+
1761+
rel = heap_open(rte->relid, NoLock);
1762+
1763+
/*
1764+
* Fetch any new security quals that must be applied to this RTE.
1765+
*/
1766+
get_row_security_policies(parsetree, rte, rt_index,
1767+
&securityQuals, &withCheckOptions,
1768+
&hasRowSecurity, &hasSubLinks);
1769+
1770+
if (securityQuals != NIL || withCheckOptions != NIL)
1771+
{
1772+
if (hasSubLinks)
1773+
{
1774+
/*
1775+
* Recursively process the new quals, checking for infinite
1776+
* recursion.
1777+
*/
1778+
if (list_member_oid(activeRIRs, RelationGetRelid(rel)))
1779+
ereport(ERROR,
1780+
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
1781+
errmsg("infinite recursion detected in policy for relation \"%s\"",
1782+
RelationGetRelationName(rel))));
1783+
1784+
activeRIRs = lcons_oid(RelationGetRelid(rel), activeRIRs);
1785+
1786+
expression_tree_walker( (Node*) securityQuals,
1787+
fireRIRonSubLink, (void*)activeRIRs );
1788+
1789+
expression_tree_walker( (Node*) withCheckOptions,
1790+
fireRIRonSubLink, (void*)activeRIRs );
1791+
1792+
activeRIRs = list_delete_first(activeRIRs);
1793+
}
1794+
1795+
/*
1796+
* Add the new security quals to the start of the RTE's list so
1797+
* that they get applied before any existing security quals (which
1798+
* might have come from a user-written security barrier view, and
1799+
* might contain malicious code).
1800+
*/
1801+
rte->securityQuals = list_concat(securityQuals,
1802+
rte->securityQuals);
1803+
1804+
parsetree->withCheckOptions = list_concat(withCheckOptions,
1805+
parsetree->withCheckOptions);
1806+
}
1807+
1808+
/*
1809+
* Make sure the query is marked correctly if row level security
1810+
* applies, or if the new quals had sublinks.
1811+
*/
1812+
if (hasRowSecurity)
1813+
parsetree->hasRowSecurity = true;
1814+
if (hasSubLinks)
1815+
parsetree->hasSubLinks = true;
1816+
1817+
heap_close(rel, NoLock);
1818+
}
1819+
17831820
return parsetree;
17841821
}
17851822

0 commit comments

Comments
 (0)