On Fri, Nov 29, 2024 at 10:01:41AM +0000, Dean Rasheed wrote:
> On Thu, 21 Nov 2024 at 18:00, Nathan Bossart <nathandboss...@gmail.com> wrote:
>> The attached patch accomplishes this by establishing a global queue of
>> row-security "nest levels" that the aforementioned higher-level callers can
>> use.
>
> I'm not convinced that this is an improvement.
Thanks for reviewing.
> The code in check_sql_fn_retval() is building a Query struct from
> scratch, so it seems perfectly natural for it to be responsible for
> setting all the required fields, based on the information it has
> available. With this patch, check_sql_fn_retval() is returning a
> potentially incorrectly marked Query at the end of the querytree list,
> which the caller is responsible for fixing up, which doesn't seem
> ideal.
While it is indeed natural for the code that builds a Query to be
responsible for setting it correctly, unfortunately there's no backstop if
someone forgets to do so (as was the case in the recent CVE). I don't
think my v1 patch would necessarily prevent all such problems, but I do
think it would help prevent some.
> There is exactly one place where RLS policies are applied, and it
> seems much more natural for it to have responsibility for setting this
> flag. I think that a slightly neater way for it to handle that would
> be to modify fireRIRrules(), adding an extra parameter "bool
> *hasRowSecurity" that it would set to true if RLS is enabled for the
> query it is rewriting. Doing that forces all callers to think about
> whether or not that affects some outer query. For example,
> ApplyRetrieveRule() would then do:
>
> rule_action = fireRIRrules(rule_action, activeRIRs,
> &parsetree->hasRowSecurity);
>
> rather than having a separate second step to update the flag on
> "parsetree", and similarly for fireRIRrules()'s recursive calls to
> itself. If, in the future, it becomes necessary to invoke
> fireRIRrules() on more parts of a Query, it's then much more likely
> that the new code won't forget to update the parent query's flag.
I've attempted this in the attached v2 patch. I do think this is an
improvement over the status quo, but I worry that it doesn't go far enough.
--
nathan
>From 0c81e04c3e62cd178dff9fc5b5f671e41fd39f28 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Mon, 2 Dec 2024 10:24:49 -0600
Subject: [PATCH v2 1/1] revamp row security tracking
---
src/backend/rewrite/rewriteHandler.c | 42 +++++++++++++++++-----------
1 file changed, 25 insertions(+), 17 deletions(-)
diff --git a/src/backend/rewrite/rewriteHandler.c
b/src/backend/rewrite/rewriteHandler.c
index ab2e2cd647..c9ad429703 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -94,7 +94,7 @@ static void markQueryForLocking(Query *qry, Node *jtnode,
bool
pushedDown);
static List *matchLocks(CmdType event, Relation relation,
int varno, Query *parsetree,
bool *hasUpdate);
-static Query *fireRIRrules(Query *parsetree, List *activeRIRs);
+static Query *fireRIRrules(Query *parsetree, List *activeRIRs, bool
*hasRowSecurity);
static Bitmapset *adjust_view_column_set(Bitmapset *cols, List *targetlist);
@@ -1730,6 +1730,7 @@ ApplyRetrieveRule(Query *parsetree,
RangeTblEntry *rte;
RowMarkClause *rc;
int numCols;
+ bool hasRowSecurity = false;
if (list_length(rule->actions) != 1)
elog(ERROR, "expected just one rule action");
@@ -1843,13 +1844,13 @@ ApplyRetrieveRule(Query *parsetree,
/*
* Recursively expand any view references inside the view.
*/
- rule_action = fireRIRrules(rule_action, activeRIRs);
+ rule_action = fireRIRrules(rule_action, activeRIRs, &hasRowSecurity);
/*
* Make sure the query is marked as having row security if the view
query
* does.
*/
- parsetree->hasRowSecurity |= rule_action->hasRowSecurity;
+ parsetree->hasRowSecurity |= hasRowSecurity;
/*
* Now, plug the view query in as a subselect, converting the relation's
@@ -1971,15 +1972,17 @@ fireRIRonSubLink(Node *node, fireRIRonSubLink_context
*context)
if (IsA(node, SubLink))
{
SubLink *sub = (SubLink *) node;
+ bool hasRowSecurity = false;
/* Do what we came for */
sub->subselect = (Node *) fireRIRrules((Query *) sub->subselect,
-
context->activeRIRs);
+
context->activeRIRs,
+
&hasRowSecurity);
/*
* Remember if any of the sublinks have row security.
*/
- context->hasRowSecurity |= ((Query *)
sub->subselect)->hasRowSecurity;
+ context->hasRowSecurity |= hasRowSecurity;
/* Fall through to process lefthand args of SubLink */
}
@@ -2000,7 +2003,7 @@ fireRIRonSubLink(Node *node, fireRIRonSubLink_context
*context)
* rules for, used to detect/reject recursion.
*/
static Query *
-fireRIRrules(Query *parsetree, List *activeRIRs)
+fireRIRrules(Query *parsetree, List *activeRIRs, bool *hasRowSecurity)
{
int origResultRelation = parsetree->resultRelation;
int rt_index;
@@ -2048,13 +2051,15 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
*/
if (rte->rtekind == RTE_SUBQUERY)
{
- rte->subquery = fireRIRrules(rte->subquery, activeRIRs);
+ bool hasRowSecurity_internal = false;
+
+ rte->subquery = fireRIRrules(rte->subquery, activeRIRs,
&hasRowSecurity_internal);
/*
* While we are here, make sure the query is marked as
having row
* security if any of its subqueries do.
*/
- parsetree->hasRowSecurity |=
rte->subquery->hasRowSecurity;
+ parsetree->hasRowSecurity |= hasRowSecurity_internal;
continue;
}
@@ -2166,15 +2171,16 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
foreach(lc, parsetree->cteList)
{
CommonTableExpr *cte = (CommonTableExpr *) lfirst(lc);
+ bool hasRowSecurity_internal = false;
cte->ctequery = (Node *)
- fireRIRrules((Query *) cte->ctequery, activeRIRs);
+ fireRIRrules((Query *) cte->ctequery, activeRIRs,
&hasRowSecurity_internal);
/*
* While we are here, make sure the query is marked as having
row
* security if any of its CTEs do.
*/
- parsetree->hasRowSecurity |= ((Query *)
cte->ctequery)->hasRowSecurity;
+ parsetree->hasRowSecurity |= hasRowSecurity_internal;
}
/*
@@ -2211,7 +2217,7 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
Relation rel;
List *securityQuals;
List *withCheckOptions;
- bool hasRowSecurity;
+ bool hasRowSecurity_internal = false;
bool hasSubLinks;
++rt_index;
@@ -2229,7 +2235,7 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
*/
get_row_security_policies(parsetree, rte, rt_index,
&securityQuals, &withCheckOptions,
-
&hasRowSecurity, &hasSubLinks);
+
&hasRowSecurity_internal, &hasSubLinks);
if (securityQuals != NIL || withCheckOptions != NIL)
{
@@ -2278,10 +2284,10 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
/*
* We can ignore the value of
fire_context.hasRowSecurity
- * since we only reach this code in cases where
hasRowSecurity
- * is already true.
+ * since we only reach this code in cases where
+ * hasRowSecurity_internal is already true.
*/
- Assert(hasRowSecurity);
+ Assert(hasRowSecurity_internal);
activeRIRs = list_delete_last(activeRIRs);
}
@@ -2303,7 +2309,7 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
* Make sure the query is marked correctly if row-level security
* applies, or if the new quals had sublinks.
*/
- if (hasRowSecurity)
+ if (hasRowSecurity_internal)
parsetree->hasRowSecurity = true;
if (hasSubLinks)
parsetree->hasSubLinks = true;
@@ -2311,6 +2317,7 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
table_close(rel, NoLock);
}
+ *hasRowSecurity = parsetree->hasRowSecurity;
return parsetree;
}
@@ -4463,8 +4470,9 @@ QueryRewrite(Query *parsetree)
foreach(l, querylist)
{
Query *query = (Query *) lfirst(l);
+ bool hasRowSecurity pg_attribute_unused() = false;
- query = fireRIRrules(query, NIL);
+ query = fireRIRrules(query, NIL, &hasRowSecurity);
query->queryId = input_query_id;
--
2.39.5 (Apple Git-154)
