]> git.ipfire.org Git - thirdparty/postgresql.git/commitdiff
Ensure cached plans are correctly marked as dependent on role.
authorNathan Bossart <nathan@postgresql.org>
Mon, 11 Nov 2024 15:00:00 +0000 (09:00 -0600)
committerNathan Bossart <nathan@postgresql.org>
Mon, 11 Nov 2024 15:00:00 +0000 (09:00 -0600)
If a CTE, subquery, sublink, security invoker view, or coercion
projection references a table with row-level security policies, we
neglected to mark the plan as potentially dependent on which role
is executing it.  This could lead to later executions in the same
session returning or hiding rows that should have been hidden or
returned instead.

Reported-by: Wolfgang Walther
Reviewed-by: Noah Misch
Security: CVE-2024-10976
Backpatch-through: 12

src/backend/executor/functions.c
src/backend/rewrite/rewriteHandler.c
src/test/regress/expected/rowsecurity.out
src/test/regress/sql/rowsecurity.sql
src/tools/pgindent/typedefs.list

index 63062a0b924000212a1e315eb79efdb7f4cc2a13..a686d87a02e41cd3211e2238d1876c1b7098bcd4 100644 (file)
@@ -1991,6 +1991,12 @@ tlist_coercion_finished:
                rtr->rtindex = 1;
                newquery->jointree = makeFromExpr(list_make1(rtr), NULL);
 
+               /*
+                * Make sure the new query is marked as having row security if the
+                * original one does.
+                */
+               newquery->hasRowSecurity = parse->hasRowSecurity;
+
                /* Replace original query in the correct element of the query list */
                lfirst(parse_cell) = newquery;
        }
index 7c715258be300039ad76aa99eaebf1e1219daac6..2b2f2da0f32a6a05aa5d18ff4308fbf59c8a5975 100644 (file)
@@ -59,6 +59,12 @@ typedef struct acquireLocksOnSubLinks_context
        bool            for_execute;    /* AcquireRewriteLocks' forExecute param */
 } acquireLocksOnSubLinks_context;
 
+typedef struct fireRIRonSubLink_context
+{
+       List       *activeRIRs;
+       bool            hasRowSecurity;
+} fireRIRonSubLink_context;
+
 static bool acquireLocksOnSubLinks(Node *node,
                                                                   acquireLocksOnSubLinks_context *context);
 static Query *rewriteRuleAction(Query *parsetree,
@@ -1860,6 +1866,12 @@ ApplyRetrieveRule(Query *parsetree,
         */
        rule_action = fireRIRrules(rule_action, activeRIRs);
 
+       /*
+        * Make sure the query is marked as having row security if the view query
+        * does.
+        */
+       parsetree->hasRowSecurity |= rule_action->hasRowSecurity;
+
        /*
         * Now, plug the view query in as a subselect, converting the relation's
         * original RTE to a subquery RTE.
@@ -1985,7 +1997,7 @@ markQueryForLocking(Query *qry, Node *jtnode,
  * the SubLink's subselect link with the possibly-rewritten subquery.
  */
 static bool
-fireRIRonSubLink(Node *node, List *activeRIRs)
+fireRIRonSubLink(Node *node, fireRIRonSubLink_context *context)
 {
        if (node == NULL)
                return false;
@@ -1995,7 +2007,13 @@ fireRIRonSubLink(Node *node, List *activeRIRs)
 
                /* Do what we came for */
                sub->subselect = (Node *) fireRIRrules((Query *) sub->subselect,
-                                                                                          activeRIRs);
+                                                                                          context->activeRIRs);
+
+               /*
+                * Remember if any of the sublinks have row security.
+                */
+               context->hasRowSecurity |= ((Query *) sub->subselect)->hasRowSecurity;
+
                /* Fall through to process lefthand args of SubLink */
        }
 
@@ -2004,7 +2022,7 @@ fireRIRonSubLink(Node *node, List *activeRIRs)
         * subselects of subselects for us.
         */
        return expression_tree_walker(node, fireRIRonSubLink,
-                                                                 (void *) activeRIRs);
+                                                                 (void *) context);
 }
 
 
@@ -2065,6 +2083,13 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
                if (rte->rtekind == RTE_SUBQUERY)
                {
                        rte->subquery = fireRIRrules(rte->subquery, activeRIRs);
+
+                       /*
+                        * 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;
+
                        continue;
                }
 
@@ -2178,6 +2203,12 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
 
                cte->ctequery = (Node *)
                        fireRIRrules((Query *) cte->ctequery, activeRIRs);
+
+               /*
+                * 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;
        }
 
        /*
@@ -2185,9 +2216,22 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
         * the rtable and cteList.
         */
        if (parsetree->hasSubLinks)
-               query_tree_walker(parsetree, fireRIRonSubLink, (void *) activeRIRs,
+       {
+               fireRIRonSubLink_context context;
+
+               context.activeRIRs = activeRIRs;
+               context.hasRowSecurity = false;
+
+               query_tree_walker(parsetree, fireRIRonSubLink, (void *) &context,
                                                  QTW_IGNORE_RC_SUBQUERIES);
 
+               /*
+                * Make sure the query is marked as having row security if any of its
+                * sublinks do.
+                */
+               parsetree->hasRowSecurity |= context.hasRowSecurity;
+       }
+
        /*
         * Apply any row-level security policies.  We do this last because it
         * requires special recursion detection if the new quals have sublink
@@ -2226,6 +2270,7 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
                        if (hasSubLinks)
                        {
                                acquireLocksOnSubLinks_context context;
+                               fireRIRonSubLink_context fire_context;
 
                                /*
                                 * Recursively process the new quals, checking for infinite
@@ -2256,11 +2301,21 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
                                 * Now that we have the locks on anything added by
                                 * get_row_security_policies, fire any RIR rules for them.
                                 */
+                               fire_context.activeRIRs = activeRIRs;
+                               fire_context.hasRowSecurity = false;
+
                                expression_tree_walker((Node *) securityQuals,
-                                                                          fireRIRonSubLink, (void *) activeRIRs);
+                                                                          fireRIRonSubLink, (void *) &fire_context);
 
                                expression_tree_walker((Node *) withCheckOptions,
-                                                                          fireRIRonSubLink, (void *) activeRIRs);
+                                                                          fireRIRonSubLink, (void *) &fire_context);
+
+                               /*
+                                * We can ignore the value of fire_context.hasRowSecurity
+                                * since we only reach this code in cases where hasRowSecurity
+                                * is already true.
+                                */
+                               Assert(hasRowSecurity);
 
                                activeRIRs = list_delete_last(activeRIRs);
                        }
index 2d99b89759449331d8886d3d9d49793c99866523..2bab32d0b2a85613769448b26a995f3b144934e8 100644 (file)
@@ -4511,8 +4511,108 @@ execute q;
 --------------+---
 (0 rows)
 
+-- make sure RLS dependencies in CTEs are handled
+reset role;
+create or replace function rls_f() returns setof rls_t
+  stable language sql
+  as $$ with cte as (select * from rls_t) select * from cte $$;
+prepare r as select current_user, * from rls_f();
+set role regress_rls_alice;
+execute r;
+   current_user    |        c         
+-------------------+------------------
+ regress_rls_alice | invisible to bob
+(1 row)
+
+set role regress_rls_bob;
+execute r;
+ current_user | c 
+--------------+---
+(0 rows)
+
+-- make sure RLS dependencies in subqueries are handled
+reset role;
+create or replace function rls_f() returns setof rls_t
+  stable language sql
+  as $$ select * from (select * from rls_t) _ $$;
+prepare s as select current_user, * from rls_f();
+set role regress_rls_alice;
+execute s;
+   current_user    |        c         
+-------------------+------------------
+ regress_rls_alice | invisible to bob
+(1 row)
+
+set role regress_rls_bob;
+execute s;
+ current_user | c 
+--------------+---
+(0 rows)
+
+-- make sure RLS dependencies in sublinks are handled
+reset role;
+create or replace function rls_f() returns setof rls_t
+  stable language sql
+  as $$ select exists(select * from rls_t)::text $$;
+prepare t as select current_user, * from rls_f();
+set role regress_rls_alice;
+execute t;
+   current_user    |  c   
+-------------------+------
+ regress_rls_alice | true
+(1 row)
+
+set role regress_rls_bob;
+execute t;
+  current_user   |   c   
+-----------------+-------
+ regress_rls_bob | false
+(1 row)
+
+-- make sure RLS dependencies are handled when coercion projections are inserted
+reset role;
+create or replace function rls_f() returns setof rls_t
+  stable language sql
+  as $$ select * from (select array_agg(c) as cs from rls_t) _ group by cs $$;
+prepare u as select current_user, * from rls_f();
+set role regress_rls_alice;
+execute u;
+   current_user    |          c           
+-------------------+----------------------
+ regress_rls_alice | {"invisible to bob"}
+(1 row)
+
+set role regress_rls_bob;
+execute u;
+  current_user   | c 
+-----------------+---
+ regress_rls_bob | 
+(1 row)
+
+-- make sure RLS dependencies in security invoker views are handled
+reset role;
+create view rls_v with (security_invoker) as select * from rls_t;
+grant select on rls_v to regress_rls_alice, regress_rls_bob;
+create or replace function rls_f() returns setof rls_t
+  stable language sql
+  as $$ select * from rls_v $$;
+prepare v as select current_user, * from rls_f();
+set role regress_rls_alice;
+execute v;
+   current_user    |        c         
+-------------------+------------------
+ regress_rls_alice | invisible to bob
+(1 row)
+
+set role regress_rls_bob;
+execute v;
+ current_user | c 
+--------------+---
+(0 rows)
+
 RESET ROLE;
 DROP FUNCTION rls_f();
+DROP VIEW rls_v;
 DROP TABLE rls_t;
 --
 -- Clean up objects
index 34ea204560334ed3acc2a2a6a90e9d67ccd56958..e928e1cb67a5a8bd31db0ee3691a858a8ddf9c02 100644 (file)
@@ -2201,8 +2201,66 @@ execute q;
 set role regress_rls_bob;
 execute q;
 
+-- make sure RLS dependencies in CTEs are handled
+reset role;
+create or replace function rls_f() returns setof rls_t
+  stable language sql
+  as $$ with cte as (select * from rls_t) select * from cte $$;
+prepare r as select current_user, * from rls_f();
+set role regress_rls_alice;
+execute r;
+set role regress_rls_bob;
+execute r;
+
+-- make sure RLS dependencies in subqueries are handled
+reset role;
+create or replace function rls_f() returns setof rls_t
+  stable language sql
+  as $$ select * from (select * from rls_t) _ $$;
+prepare s as select current_user, * from rls_f();
+set role regress_rls_alice;
+execute s;
+set role regress_rls_bob;
+execute s;
+
+-- make sure RLS dependencies in sublinks are handled
+reset role;
+create or replace function rls_f() returns setof rls_t
+  stable language sql
+  as $$ select exists(select * from rls_t)::text $$;
+prepare t as select current_user, * from rls_f();
+set role regress_rls_alice;
+execute t;
+set role regress_rls_bob;
+execute t;
+
+-- make sure RLS dependencies are handled when coercion projections are inserted
+reset role;
+create or replace function rls_f() returns setof rls_t
+  stable language sql
+  as $$ select * from (select array_agg(c) as cs from rls_t) _ group by cs $$;
+prepare u as select current_user, * from rls_f();
+set role regress_rls_alice;
+execute u;
+set role regress_rls_bob;
+execute u;
+
+-- make sure RLS dependencies in security invoker views are handled
+reset role;
+create view rls_v with (security_invoker) as select * from rls_t;
+grant select on rls_v to regress_rls_alice, regress_rls_bob;
+create or replace function rls_f() returns setof rls_t
+  stable language sql
+  as $$ select * from rls_v $$;
+prepare v as select current_user, * from rls_f();
+set role regress_rls_alice;
+execute v;
+set role regress_rls_bob;
+execute v;
+
 RESET ROLE;
 DROP FUNCTION rls_f();
+DROP VIEW rls_v;
 DROP TABLE rls_t;
 
 --
index 387f2234243a6cb7d65949f5767ef1124609df48..a98ec7aa7f607b82f9667b1fd35a97c3ae600068 100644 (file)
@@ -3254,6 +3254,7 @@ fill_string_relopt
 finalize_primnode_context
 find_dependent_phvs_context
 find_expr_references_context
+fireRIRonSubLink_context
 fix_join_expr_context
 fix_scan_expr_context
 fix_upper_expr_context