From: Nathan Bossart <nathandbossart@gmail.com>
To: Dean Rasheed <dean.a.rasheed@gmail.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: revamp row-security tracking
Date: Mon, 2 Dec 2024 10:34:21 -0600
Message-ID: <Z03hjW3U9yERkDdG@nathan> (raw)
In-Reply-To: <CAEZATCUDuzvQ8THEHNRRkg7Nia7OzZBA5PzwCm_c70M82pCnPg@mail.gmail.com>
References: <Zz91RagtQg2s9497@nathan>
<CAEZATCUDuzvQ8THEHNRRkg7Nia7OzZBA5PzwCm_c70M82pCnPg@mail.gmail.com>
On Fri, Nov 29, 2024 at 10:01:41AM +0000, Dean Rasheed wrote:
> On Thu, 21 Nov 2024 at 18:00, Nathan Bossart <nathandbossart@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 <nathan@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.cindex 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)
Attachments:
[text/plain] v2-0001-revamp-row-security-tracking.patch (5.8K, ../Z03hjW3U9yERkDdG@nathan/2-v2-0001-revamp-row-security-tracking.patch)
download | inline diff:
From 0c81e04c3e62cd178dff9fc5b5f671e41fd39f28 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@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.cindex 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)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, dean.a.rasheed@gmail.com
Subject: Re: revamp row-security tracking
In-Reply-To: <Z03hjW3U9yERkDdG@nathan>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox