Skip to content

Commit edcda9b

Browse files
nathan-bossartnathan-bossart
committed
Ensure cached plans are correctly marked as dependent on role.
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
1 parent 3ebcfa5 commit edcda9b

File tree

5 files changed

+226
-6
lines changed

5 files changed

+226
-6
lines changed

‎src/backend/executor/functions.c

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1972,6 +1972,12 @@ check_sql_fn_retval(List *queryTreeLists,
19721972
rtr->rtindex = 1;
19731973
newquery->jointree = makeFromExpr(list_make1(rtr), NULL);
19741974

1975+
/*
1976+
* Make sure the new query is marked as having row security if the
1977+
* original one does.
1978+
*/
1979+
newquery->hasRowSecurity = parse->hasRowSecurity;
1980+
19751981
/* Replace original query in the correct element of the query list */
19761982
lfirst(parse_cell) = newquery;
19771983
}

‎src/backend/rewrite/rewriteHandler.c

Lines changed: 61 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,12 @@ typedef struct acquireLocksOnSubLinks_context
5858
bool for_execute; /* AcquireRewriteLocks' forExecute param */
5959
} acquireLocksOnSubLinks_context;
6060

61+
typedef struct fireRIRonSubLink_context
62+
{
63+
List *activeRIRs;
64+
bool hasRowSecurity;
65+
} fireRIRonSubLink_context;
66+
6167
static bool acquireLocksOnSubLinks(Node *node,
6268
acquireLocksOnSubLinks_context *context);
6369
static Query *rewriteRuleAction(Query *parsetree,
@@ -1839,6 +1845,12 @@ ApplyRetrieveRule(Query *parsetree,
18391845
*/
18401846
rule_action = fireRIRrules(rule_action, activeRIRs);
18411847

1848+
/*
1849+
* Make sure the query is marked as having row security if the view query
1850+
* does.
1851+
*/
1852+
parsetree->hasRowSecurity |= rule_action->hasRowSecurity;
1853+
18421854
/*
18431855
* Now, plug the view query in as a subselect, converting the relation's
18441856
* original RTE to a subquery RTE.
@@ -1952,7 +1964,7 @@ markQueryForLocking(Query *qry, Node *jtnode,
19521964
* the SubLink's subselect link with the possibly-rewritten subquery.
19531965
*/
19541966
static bool
1955-
fireRIRonSubLink(Node *node, List *activeRIRs)
1967+
fireRIRonSubLink(Node *node, fireRIRonSubLink_context *context)
19561968
{
19571969
if (node == NULL)
19581970
return false;
@@ -1962,7 +1974,13 @@ fireRIRonSubLink(Node *node, List *activeRIRs)
19621974

19631975
/* Do what we came for */
19641976
sub->subselect = (Node *) fireRIRrules((Query *) sub->subselect,
1965-
activeRIRs);
1977+
context->activeRIRs);
1978+
1979+
/*
1980+
* Remember if any of the sublinks have row security.
1981+
*/
1982+
context->hasRowSecurity |= ((Query *) sub->subselect)->hasRowSecurity;
1983+
19661984
/* Fall through to process lefthand args of SubLink */
19671985
}
19681986

@@ -1971,7 +1989,7 @@ fireRIRonSubLink(Node *node, List *activeRIRs)
19711989
* subselects of subselects for us.
19721990
*/
19731991
return expression_tree_walker(node, fireRIRonSubLink,
1974-
(void *) activeRIRs);
1992+
(void *) context);
19751993
}
19761994

19771995

@@ -2032,6 +2050,13 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
20322050
if (rte->rtekind == RTE_SUBQUERY)
20332051
{
20342052
rte->subquery = fireRIRrules(rte->subquery, activeRIRs);
2053+
2054+
/*
2055+
* While we are here, make sure the query is marked as having row
2056+
* security if any of its subqueries do.
2057+
*/
2058+
parsetree->hasRowSecurity |= rte->subquery->hasRowSecurity;
2059+
20352060
continue;
20362061
}
20372062

@@ -2145,16 +2170,35 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
21452170

21462171
cte->ctequery = (Node *)
21472172
fireRIRrules((Query *) cte->ctequery, activeRIRs);
2173+
2174+
/*
2175+
* While we are here, make sure the query is marked as having row
2176+
* security if any of its CTEs do.
2177+
*/
2178+
parsetree->hasRowSecurity |= ((Query *) cte->ctequery)->hasRowSecurity;
21482179
}
21492180

21502181
/*
21512182
* Recurse into sublink subqueries, too. But we already did the ones in
21522183
* the rtable and cteList.
21532184
*/
21542185
if (parsetree->hasSubLinks)
2155-
query_tree_walker(parsetree, fireRIRonSubLink, (void *) activeRIRs,
2186+
{
2187+
fireRIRonSubLink_context context;
2188+
2189+
context.activeRIRs = activeRIRs;
2190+
context.hasRowSecurity = false;
2191+
2192+
query_tree_walker(parsetree, fireRIRonSubLink, (void *) &context,
21562193
QTW_IGNORE_RC_SUBQUERIES);
21572194

2195+
/*
2196+
* Make sure the query is marked as having row security if any of its
2197+
* sublinks do.
2198+
*/
2199+
parsetree->hasRowSecurity |= context.hasRowSecurity;
2200+
}
2201+
21582202
/*
21592203
* Apply any row-level security policies. We do this last because it
21602204
* requires special recursion detection if the new quals have sublink
@@ -2193,6 +2237,7 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
21932237
if (hasSubLinks)
21942238
{
21952239
acquireLocksOnSubLinks_context context;
2240+
fireRIRonSubLink_context fire_context;
21962241

21972242
/*
21982243
* Recursively process the new quals, checking for infinite
@@ -2223,11 +2268,21 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
22232268
* Now that we have the locks on anything added by
22242269
* get_row_security_policies, fire any RIR rules for them.
22252270
*/
2271+
fire_context.activeRIRs = activeRIRs;
2272+
fire_context.hasRowSecurity = false;
2273+
22262274
expression_tree_walker((Node *) securityQuals,
2227-
fireRIRonSubLink, (void *) activeRIRs);
2275+
fireRIRonSubLink, (void *) &fire_context);
22282276

22292277
expression_tree_walker((Node *) withCheckOptions,
2230-
fireRIRonSubLink, (void *) activeRIRs);
2278+
fireRIRonSubLink, (void *) &fire_context);
2279+
2280+
/*
2281+
* We can ignore the value of fire_context.hasRowSecurity
2282+
* since we only reach this code in cases where hasRowSecurity
2283+
* is already true.
2284+
*/
2285+
Assert(hasRowSecurity);
22312286

22322287
activeRIRs = list_delete_last(activeRIRs);
22332288
}

‎src/test/regress/expected/rowsecurity.out

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4554,8 +4554,108 @@ execute q;
45544554
--------------+---
45554555
(0 rows)
45564556

4557+
-- make sure RLS dependencies in CTEs are handled
4558+
reset role;
4559+
create or replace function rls_f() returns setof rls_t
4560+
stable language sql
4561+
as $$ with cte as (select * from rls_t) select * from cte $$;
4562+
prepare r as select current_user, * from rls_f();
4563+
set role regress_rls_alice;
4564+
execute r;
4565+
current_user | c
4566+
-------------------+------------------
4567+
regress_rls_alice | invisible to bob
4568+
(1 row)
4569+
4570+
set role regress_rls_bob;
4571+
execute r;
4572+
current_user | c
4573+
--------------+---
4574+
(0 rows)
4575+
4576+
-- make sure RLS dependencies in subqueries are handled
4577+
reset role;
4578+
create or replace function rls_f() returns setof rls_t
4579+
stable language sql
4580+
as $$ select * from (select * from rls_t) _ $$;
4581+
prepare s as select current_user, * from rls_f();
4582+
set role regress_rls_alice;
4583+
execute s;
4584+
current_user | c
4585+
-------------------+------------------
4586+
regress_rls_alice | invisible to bob
4587+
(1 row)
4588+
4589+
set role regress_rls_bob;
4590+
execute s;
4591+
current_user | c
4592+
--------------+---
4593+
(0 rows)
4594+
4595+
-- make sure RLS dependencies in sublinks are handled
4596+
reset role;
4597+
create or replace function rls_f() returns setof rls_t
4598+
stable language sql
4599+
as $$ select exists(select * from rls_t)::text $$;
4600+
prepare t as select current_user, * from rls_f();
4601+
set role regress_rls_alice;
4602+
execute t;
4603+
current_user | c
4604+
-------------------+------
4605+
regress_rls_alice | true
4606+
(1 row)
4607+
4608+
set role regress_rls_bob;
4609+
execute t;
4610+
current_user | c
4611+
-----------------+-------
4612+
regress_rls_bob | false
4613+
(1 row)
4614+
4615+
-- make sure RLS dependencies are handled when coercion projections are inserted
4616+
reset role;
4617+
create or replace function rls_f() returns setof rls_t
4618+
stable language sql
4619+
as $$ select * from (select array_agg(c) as cs from rls_t) _ group by cs $$;
4620+
prepare u as select current_user, * from rls_f();
4621+
set role regress_rls_alice;
4622+
execute u;
4623+
current_user | c
4624+
-------------------+----------------------
4625+
regress_rls_alice | {"invisible to bob"}
4626+
(1 row)
4627+
4628+
set role regress_rls_bob;
4629+
execute u;
4630+
current_user | c
4631+
-----------------+---
4632+
regress_rls_bob |
4633+
(1 row)
4634+
4635+
-- make sure RLS dependencies in security invoker views are handled
4636+
reset role;
4637+
create view rls_v with (security_invoker) as select * from rls_t;
4638+
grant select on rls_v to regress_rls_alice, regress_rls_bob;
4639+
create or replace function rls_f() returns setof rls_t
4640+
stable language sql
4641+
as $$ select * from rls_v $$;
4642+
prepare v as select current_user, * from rls_f();
4643+
set role regress_rls_alice;
4644+
execute v;
4645+
current_user | c
4646+
-------------------+------------------
4647+
regress_rls_alice | invisible to bob
4648+
(1 row)
4649+
4650+
set role regress_rls_bob;
4651+
execute v;
4652+
current_user | c
4653+
--------------+---
4654+
(0 rows)
4655+
45574656
RESET ROLE;
45584657
DROP FUNCTION rls_f();
4658+
DROP VIEW rls_v;
45594659
DROP TABLE rls_t;
45604660
--
45614661
-- Clean up objects

‎src/test/regress/sql/rowsecurity.sql

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2217,8 +2217,66 @@ execute q;
22172217
set role regress_rls_bob;
22182218
execute q;
22192219

2220+
-- make sure RLS dependencies in CTEs are handled
2221+
reset role;
2222+
create or replace function rls_f() returns setof rls_t
2223+
stable language sql
2224+
as $$ with cte as (select * from rls_t) select * from cte $$;
2225+
prepare r as select current_user, * from rls_f();
2226+
set role regress_rls_alice;
2227+
execute r;
2228+
set role regress_rls_bob;
2229+
execute r;
2230+
2231+
-- make sure RLS dependencies in subqueries are handled
2232+
reset role;
2233+
create or replace function rls_f() returns setof rls_t
2234+
stable language sql
2235+
as $$ select * from (select * from rls_t) _ $$;
2236+
prepare s as select current_user, * from rls_f();
2237+
set role regress_rls_alice;
2238+
execute s;
2239+
set role regress_rls_bob;
2240+
execute s;
2241+
2242+
-- make sure RLS dependencies in sublinks are handled
2243+
reset role;
2244+
create or replace function rls_f() returns setof rls_t
2245+
stable language sql
2246+
as $$ select exists(select * from rls_t)::text $$;
2247+
prepare t as select current_user, * from rls_f();
2248+
set role regress_rls_alice;
2249+
execute t;
2250+
set role regress_rls_bob;
2251+
execute t;
2252+
2253+
-- make sure RLS dependencies are handled when coercion projections are inserted
2254+
reset role;
2255+
create or replace function rls_f() returns setof rls_t
2256+
stable language sql
2257+
as $$ select * from (select array_agg(c) as cs from rls_t) _ group by cs $$;
2258+
prepare u as select current_user, * from rls_f();
2259+
set role regress_rls_alice;
2260+
execute u;
2261+
set role regress_rls_bob;
2262+
execute u;
2263+
2264+
-- make sure RLS dependencies in security invoker views are handled
2265+
reset role;
2266+
create view rls_v with (security_invoker) as select * from rls_t;
2267+
grant select on rls_v to regress_rls_alice, regress_rls_bob;
2268+
create or replace function rls_f() returns setof rls_t
2269+
stable language sql
2270+
as $$ select * from rls_v $$;
2271+
prepare v as select current_user, * from rls_f();
2272+
set role regress_rls_alice;
2273+
execute v;
2274+
set role regress_rls_bob;
2275+
execute v;
2276+
22202277
RESET ROLE;
22212278
DROP FUNCTION rls_f();
2279+
DROP VIEW rls_v;
22222280
DROP TABLE rls_t;
22232281

22242282
--

‎src/tools/pgindent/typedefs.list

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3451,6 +3451,7 @@ fill_string_relopt
34513451
finalize_primnode_context
34523452
find_dependent_phvs_context
34533453
find_expr_references_context
3454+
fireRIRonSubLink_context
34543455
fix_join_expr_context
34553456
fix_scan_expr_context
34563457
fix_upper_expr_context

0 commit comments

Comments
 (0)