Skip to content

Commit 4e51030

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 d15ec27 commit 4e51030

File tree

5 files changed

+190
-6
lines changed

5 files changed

+190
-6
lines changed

‎src/backend/executor/functions.c

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1991,6 +1991,12 @@ check_sql_fn_retval_ext(List *queryTreeLists,
19911991
rtr->rtindex = 1;
19921992
newquery->jointree = makeFromExpr(list_make1(rtr), NULL);
19931993

1994+
/*
1995+
* Make sure the new query is marked as having row security if the
1996+
* original one does.
1997+
*/
1998+
newquery->hasRowSecurity = parse->hasRowSecurity;
1999+
19942000
/* Replace original query in the correct element of the query list */
19952001
lfirst(parse_cell) = newquery;
19962002
}

‎src/backend/rewrite/rewriteHandler.c

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

62+
typedef struct fireRIRonSubLink_context
63+
{
64+
List *activeRIRs;
65+
bool hasRowSecurity;
66+
} fireRIRonSubLink_context;
67+
6268
static bool acquireLocksOnSubLinks(Node *node,
6369
acquireLocksOnSubLinks_context *context);
6470
static Query *rewriteRuleAction(Query *parsetree,
@@ -1856,6 +1862,12 @@ ApplyRetrieveRule(Query *parsetree,
18561862
*/
18571863
rule_action = fireRIRrules(rule_action, activeRIRs);
18581864

1865+
/*
1866+
* Make sure the query is marked as having row security if the view query
1867+
* does.
1868+
*/
1869+
parsetree->hasRowSecurity |= rule_action->hasRowSecurity;
1870+
18591871
/*
18601872
* Now, plug the view query in as a subselect, converting the relation's
18611873
* original RTE to a subquery RTE.
@@ -1981,7 +1993,7 @@ markQueryForLocking(Query *qry, Node *jtnode,
19811993
* the SubLink's subselect link with the possibly-rewritten subquery.
19821994
*/
19831995
static bool
1984-
fireRIRonSubLink(Node *node, List *activeRIRs)
1996+
fireRIRonSubLink(Node *node, fireRIRonSubLink_context *context)
19851997
{
19861998
if (node == NULL)
19871999
return false;
@@ -1991,7 +2003,13 @@ fireRIRonSubLink(Node *node, List *activeRIRs)
19912003

19922004
/* Do what we came for */
19932005
sub->subselect = (Node *) fireRIRrules((Query *) sub->subselect,
1994-
activeRIRs);
2006+
context->activeRIRs);
2007+
2008+
/*
2009+
* Remember if any of the sublinks have row security.
2010+
*/
2011+
context->hasRowSecurity |= ((Query *) sub->subselect)->hasRowSecurity;
2012+
19952013
/* Fall through to process lefthand args of SubLink */
19962014
}
19972015

@@ -2000,7 +2018,7 @@ fireRIRonSubLink(Node *node, List *activeRIRs)
20002018
* subselects of subselects for us.
20012019
*/
20022020
return expression_tree_walker(node, fireRIRonSubLink,
2003-
(void *) activeRIRs);
2021+
(void *) context);
20042022
}
20052023

20062024

@@ -2061,6 +2079,13 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
20612079
if (rte->rtekind == RTE_SUBQUERY)
20622080
{
20632081
rte->subquery = fireRIRrules(rte->subquery, activeRIRs);
2082+
2083+
/*
2084+
* While we are here, make sure the query is marked as having row
2085+
* security if any of its subqueries do.
2086+
*/
2087+
parsetree->hasRowSecurity |= rte->subquery->hasRowSecurity;
2088+
20642089
continue;
20652090
}
20662091

@@ -2174,16 +2199,35 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
21742199

21752200
cte->ctequery = (Node *)
21762201
fireRIRrules((Query *) cte->ctequery, activeRIRs);
2202+
2203+
/*
2204+
* While we are here, make sure the query is marked as having row
2205+
* security if any of its CTEs do.
2206+
*/
2207+
parsetree->hasRowSecurity |= ((Query *) cte->ctequery)->hasRowSecurity;
21772208
}
21782209

21792210
/*
21802211
* Recurse into sublink subqueries, too. But we already did the ones in
21812212
* the rtable and cteList.
21822213
*/
21832214
if (parsetree->hasSubLinks)
2184-
query_tree_walker(parsetree, fireRIRonSubLink, (void *) activeRIRs,
2215+
{
2216+
fireRIRonSubLink_context context;
2217+
2218+
context.activeRIRs = activeRIRs;
2219+
context.hasRowSecurity = false;
2220+
2221+
query_tree_walker(parsetree, fireRIRonSubLink, (void *) &context,
21852222
QTW_IGNORE_RC_SUBQUERIES);
21862223

2224+
/*
2225+
* Make sure the query is marked as having row security if any of its
2226+
* sublinks do.
2227+
*/
2228+
parsetree->hasRowSecurity |= context.hasRowSecurity;
2229+
}
2230+
21872231
/*
21882232
* Apply any row-level security policies. We do this last because it
21892233
* requires special recursion detection if the new quals have sublink
@@ -2222,6 +2266,7 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
22222266
if (hasSubLinks)
22232267
{
22242268
acquireLocksOnSubLinks_context context;
2269+
fireRIRonSubLink_context fire_context;
22252270

22262271
/*
22272272
* Recursively process the new quals, checking for infinite
@@ -2252,11 +2297,21 @@ fireRIRrules(Query *parsetree, List *activeRIRs)
22522297
* Now that we have the locks on anything added by
22532298
* get_row_security_policies, fire any RIR rules for them.
22542299
*/
2300+
fire_context.activeRIRs = activeRIRs;
2301+
fire_context.hasRowSecurity = false;
2302+
22552303
expression_tree_walker((Node *) securityQuals,
2256-
fireRIRonSubLink, (void *) activeRIRs);
2304+
fireRIRonSubLink, (void *) &fire_context);
22572305

22582306
expression_tree_walker((Node *) withCheckOptions,
2259-
fireRIRonSubLink, (void *) activeRIRs);
2307+
fireRIRonSubLink, (void *) &fire_context);
2308+
2309+
/*
2310+
* We can ignore the value of fire_context.hasRowSecurity
2311+
* since we only reach this code in cases where hasRowSecurity
2312+
* is already true.
2313+
*/
2314+
Assert(hasRowSecurity);
22602315

22612316
activeRIRs = list_delete_last(activeRIRs);
22622317
}

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

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4043,6 +4043,84 @@ execute q;
40434043
--------------+---
40444044
(0 rows)
40454045

4046+
-- make sure RLS dependencies in CTEs are handled
4047+
reset role;
4048+
create or replace function rls_f() returns setof rls_t
4049+
stable language sql
4050+
as $$ with cte as (select * from rls_t) select * from cte $$;
4051+
prepare r as select current_user, * from rls_f();
4052+
set role regress_rls_alice;
4053+
execute r;
4054+
current_user | c
4055+
-------------------+------------------
4056+
regress_rls_alice | invisible to bob
4057+
(1 row)
4058+
4059+
set role regress_rls_bob;
4060+
execute r;
4061+
current_user | c
4062+
--------------+---
4063+
(0 rows)
4064+
4065+
-- make sure RLS dependencies in subqueries are handled
4066+
reset role;
4067+
create or replace function rls_f() returns setof rls_t
4068+
stable language sql
4069+
as $$ select * from (select * from rls_t) _ $$;
4070+
prepare s as select current_user, * from rls_f();
4071+
set role regress_rls_alice;
4072+
execute s;
4073+
current_user | c
4074+
-------------------+------------------
4075+
regress_rls_alice | invisible to bob
4076+
(1 row)
4077+
4078+
set role regress_rls_bob;
4079+
execute s;
4080+
current_user | c
4081+
--------------+---
4082+
(0 rows)
4083+
4084+
-- make sure RLS dependencies in sublinks are handled
4085+
reset role;
4086+
create or replace function rls_f() returns setof rls_t
4087+
stable language sql
4088+
as $$ select exists(select * from rls_t)::text $$;
4089+
prepare t as select current_user, * from rls_f();
4090+
set role regress_rls_alice;
4091+
execute t;
4092+
current_user | c
4093+
-------------------+------
4094+
regress_rls_alice | true
4095+
(1 row)
4096+
4097+
set role regress_rls_bob;
4098+
execute t;
4099+
current_user | c
4100+
-----------------+-------
4101+
regress_rls_bob | false
4102+
(1 row)
4103+
4104+
-- make sure RLS dependencies are handled when coercion projections are inserted
4105+
reset role;
4106+
create or replace function rls_f() returns setof rls_t
4107+
stable language sql
4108+
as $$ select * from (select array_agg(c) as cs from rls_t) _ group by cs $$;
4109+
prepare u as select current_user, * from rls_f();
4110+
set role regress_rls_alice;
4111+
execute u;
4112+
current_user | c
4113+
-------------------+----------------------
4114+
regress_rls_alice | {"invisible to bob"}
4115+
(1 row)
4116+
4117+
set role regress_rls_bob;
4118+
execute u;
4119+
current_user | c
4120+
-----------------+---
4121+
regress_rls_bob |
4122+
(1 row)
4123+
40464124
RESET ROLE;
40474125
DROP FUNCTION rls_f();
40484126
DROP TABLE rls_t;

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

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1889,6 +1889,50 @@ execute q;
18891889
set role regress_rls_bob;
18901890
execute q;
18911891

1892+
-- make sure RLS dependencies in CTEs are handled
1893+
reset role;
1894+
create or replace function rls_f() returns setof rls_t
1895+
stable language sql
1896+
as $$ with cte as (select * from rls_t) select * from cte $$;
1897+
prepare r as select current_user, * from rls_f();
1898+
set role regress_rls_alice;
1899+
execute r;
1900+
set role regress_rls_bob;
1901+
execute r;
1902+
1903+
-- make sure RLS dependencies in subqueries are handled
1904+
reset role;
1905+
create or replace function rls_f() returns setof rls_t
1906+
stable language sql
1907+
as $$ select * from (select * from rls_t) _ $$;
1908+
prepare s as select current_user, * from rls_f();
1909+
set role regress_rls_alice;
1910+
execute s;
1911+
set role regress_rls_bob;
1912+
execute s;
1913+
1914+
-- make sure RLS dependencies in sublinks are handled
1915+
reset role;
1916+
create or replace function rls_f() returns setof rls_t
1917+
stable language sql
1918+
as $$ select exists(select * from rls_t)::text $$;
1919+
prepare t as select current_user, * from rls_f();
1920+
set role regress_rls_alice;
1921+
execute t;
1922+
set role regress_rls_bob;
1923+
execute t;
1924+
1925+
-- make sure RLS dependencies are handled when coercion projections are inserted
1926+
reset role;
1927+
create or replace function rls_f() returns setof rls_t
1928+
stable language sql
1929+
as $$ select * from (select array_agg(c) as cs from rls_t) _ group by cs $$;
1930+
prepare u as select current_user, * from rls_f();
1931+
set role regress_rls_alice;
1932+
execute u;
1933+
set role regress_rls_bob;
1934+
execute u;
1935+
18921936
RESET ROLE;
18931937
DROP FUNCTION rls_f();
18941938
DROP TABLE rls_t;

‎src/tools/pgindent/typedefs.list

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3118,6 +3118,7 @@ fill_string_relopt
31183118
finalize_primnode_context
31193119
find_dependent_phvs_context
31203120
find_expr_references_context
3121+
fireRIRonSubLink_context
31213122
fix_join_expr_context
31223123
fix_scan_expr_context
31233124
fix_upper_expr_context

0 commit comments

Comments
 (0)