Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit cd7ab57

Browse files
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 b7e3a52 commit cd7ab57

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
@@ -4556,8 +4556,108 @@ execute q;
45564556
--------------+---
45574557
(0 rows)
45584558

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

0 commit comments

Comments
 (0)