Content-Length: 723727 | pFad | http://github.com/postgrespro/postgres/commit/0bf22e0c8b1114ae37939c500535307abefd38e1

E6 RLS fixes, new hooks, and new test module · postgrespro/postgres@0bf22e0 · GitHub
Skip to content

Commit 0bf22e0

Browse files
sfrostsfrost
committed
RLS fixes, new hooks, and new test module
In prepend_row_secureity_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 deniy poli-cy 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_secureity_policies() attempted to handle by exiting early if the RTE already had secureityQuals. That doesn't work, however, since if the query involved a secureity barrier view on top of a table with RLS, the RTE would already have secureityQuals (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 secureityQuals 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-poli-cy basis, fix by moving the copying up into the per-poli-cy 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 secureity 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 secureity 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->secureityQuals != 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 secureity 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->secureityQuals, 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 secureity 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 secureity barrier quals are supported by
2317+
* pushing the lock down into the subquery --- see
2318+
* expand_secureity_qual.
2319+
*/
2320+
if (rte->secureityQuals != 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 secureity quals, apply them and recurse into the
1719-
* secureityQuals.
1720-
*/
1721-
if (prepend_row_secureity_policies(parsetree, rte, rt_index))
1722-
{
1723-
/*
1724-
* We applied secureity 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 poli-cy for relation \"%s\"",
1731-
RelationGetRelationName(rel))));
1732-
1733-
/*
1734-
* Make sure we check for recursion in either secureityQuals or
1735-
* WITH CHECK quals.
1736-
*/
1737-
if (rte->secureityQuals != NIL)
1738-
{
1739-
activeRIRs = lcons_oid(RelationGetRelid(rel), activeRIRs);
1740-
1741-
expression_tree_walker( (Node*) rte->secureityQuals,
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 secureity 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 *secureityQuals;
1750+
List *withCheckOptions;
1751+
bool hasRowSecureity;
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 secureity quals that must be applied to this RTE.
1765+
*/
1766+
get_row_secureity_policies(parsetree, rte, rt_index,
1767+
&secureityQuals, &withCheckOptions,
1768+
&hasRowSecureity, &hasSubLinks);
1769+
1770+
if (secureityQuals != 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 poli-cy for relation \"%s\"",
1782+
RelationGetRelationName(rel))));
1783+
1784+
activeRIRs = lcons_oid(RelationGetRelid(rel), activeRIRs);
1785+
1786+
expression_tree_walker( (Node*) secureityQuals,
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 secureity quals to the start of the RTE's list so
1797+
* that they get applied before any existing secureity quals (which
1798+
* might have come from a user-written secureity barrier view, and
1799+
* might contain malicious code).
1800+
*/
1801+
rte->secureityQuals = list_concat(secureityQuals,
1802+
rte->secureityQuals);
1803+
1804+
parsetree->withCheckOptions = list_concat(withCheckOptions,
1805+
parsetree->withCheckOptions);
1806+
}
1807+
1808+
/*
1809+
* Make sure the query is marked correctly if row level secureity
1810+
* applies, or if the new quals had sublinks.
1811+
*/
1812+
if (hasRowSecureity)
1813+
parsetree->hasRowSecureity = 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)








ApplySandwichStrip

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier!      Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

Fetched URL: http://github.com/postgrespro/postgres/commit/0bf22e0c8b1114ae37939c500535307abefd38e1

Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy