Date: Mon, 2 Dec 2024 10:34:21 -0600
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
Message-ID: <Z03hjW3U9yERkDdG@nathan>
References: <Zz91RagtQg2s9497@nathan>
 <CAEZATCUDuzvQ8THEHNRRkg7Nia7OzZBA5PzwCm_c70M82pCnPg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="hOEVYYD8Jvq7AOUJ"
Content-Disposition: inline
In-Reply-To: 
 <CAEZATCUDuzvQ8THEHNRRkg7Nia7OzZBA5PzwCm_c70M82pCnPg@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/Z03hjW3U9yERkDdG%40nathan>
Precedence: bulk


--hOEVYYD8Jvq7AOUJ
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--hOEVYYD8Jvq7AOUJ
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename="v2-0001-revamp-row-security-tracking.patch"

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.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)


--hOEVYYD8Jvq7AOUJ--