8000 Reset plan->row_security_env and planUserId · prmdeveloper/postgres@db69e58 · GitHub
[go: up one dir, main page]
Skip to content

Commit db69e58

Browse files
sfrostsfrost
committed
Reset plan->row_security_env and planUserId
In the plancache, we check if the environment we planned the query under has changed in a way which requires us to re-plan, such as when the user for whom the plan was prepared changes and RLS is being used (and, therefore, there may be different policies to apply). Unfortunately, while those values were set and checked, they were not being reset when the query was re-planned and therefore, in cases where we change role, re-plan, and then change role again, we weren't re-planning again. This leads to potentially incorrect policies being applied in cases where role-specific policies are used and a given query is planned under one role and then executed under other roles, which could happen under security definer functions or when a common user and query is planned initially and then re-used across multiple SET ROLEs. Further, extensions which made use of CopyCachedPlan() may suffer from similar issues as the RLS-related fields were not properly copied as part of the plan and therefore RevalidateCachedQuery() would copy in the current settings without invalidating the query. Fix by using the same approach used for 'search_path', where we set the correct values in CompleteCachedPlan(), check them early on in RevalidateCachedQuery() and then properly reset them if re-planning. Also, copy through the values during CopyCachedPlan(). Pointed out by Ashutosh Bapat. Reviewed by Michael Paquier. Back-patch to 9.5 where RLS was introduced. Security: CVE-2016-2193
1 parent d6e7401 commit db69e58

File tree

3 files changed

+49
-14
lines changed

3 files changed

+49
-14
lines changed

‎src/backend/utils/cache/plancache.c

Lines changed: 26 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,8 @@
1616
* if it has one. When (and if) the next demand for a cached plan occurs,
1717
* parse analysis and rewrite is repeated to build a new valid query tree,
1818
* and then planning is performed as normal. We also force re-analysis and
19-
* re-planning if the active search_path is different from the previous time.
19+
* re-planning if the active search_path is different from the previous time
20+
* or, if RLS is involved, if the user changes or the RLS environment changes.
2021
*
10000
2122
* Note that if the sinval was a result of user DDL actions, parse analysis
2223
* could throw an error, for example if a column referenced by the query is
@@ -204,8 +205,8 @@ CreateCachedPlan(Node *raw_parse_tree,
204205
plansource->total_custom_cost = 0;
205206
plansource->num_custom_plans = 0;
206207
plansource->hasRowSecurity = false;
207-
plansource->row_security_env = row_security;
208208
plansource->planUserId = InvalidOid;
209+
plansource->row_security_env = false;
209210

210211
MemoryContextSwitchTo(oldcxt);
211212

@@ -271,6 +272,8 @@ CreateOneShotCachedPlan(Node *raw_parse_tree,
271272
plansource->generic_cost = -1;
272273
plansource->total_custom_cost = 0;
273274
plansource->num_custom_plans = 0;
275+
plansource->planUserId = InvalidOid;
276+
plansource->row_security_env = false;
274277

275278
return plansource;
276279
}
@@ -409,6 +412,8 @@ CompleteCachedPlan(CachedPlanSource *plansource,
409412
plansource->cursor_options = cursor_options;
410413
plansource->fixed_result = fixed_result;
411414
plansource->resultDesc = PlanCacheComputeResultDesc(querytree_list);
415+
plansource->planUserId = GetUserId();
416+
plansource->row_security_env = row_security;
412417

413418
MemoryContextSwitchTo(oldcxt);
414419

@@ -571,24 +576,15 @@ RevalidateCachedQuery(CachedPlanSource *plansource)
571576
return NIL;
572577
}
573578

574-
/*
575-
* If this is a new cached plan, then set the user id it was planned by
576-
* and under what row security settings; these are needed to determine
577-
* plan invalidation when RLS is involved.
578-
*/
579-
if (!OidIsValid(plansource->planUserId))
580-
{
581-
plansource->planUserId = GetUserId();
582-
plansource->row_security_env = row_security;
583-
}
584-
585579
/*
586580
* If the query is currently valid, we should have a saved search_path ---
587581
* check to see if that matches the current environment. If not, we want
588-
* to force replan.
582+
* to force replan. We should also have a valid planUserId.
589583
*/
590584
if (plansource->is_valid)
591585
{
586+
Assert(OidIsValid(plansource->planUserId));
587+
592588
Assert(plansource->search_path != NULL);
593589
if (!OverrideSearchPathMatchesCurrent(plansource->search_path))
594590
{
@@ -643,6 +639,14 @@ RevalidateCachedQuery(CachedPlanSource *plansource)
643639
plansource->invalItems = NIL;
644640
plansource->search_path = NULL;
645641

642+
/*
643+
* The plan is invalid, possibly due to row security, so we need to reset
644+
* row_security_env and planUserId as we're about to re-plan with the
645+
* current settings.
646+
*/
647+
plansource->row_security_env = row_security;
648+
plansource->planUserId = GetUserId();
649+
646650
/*
647651
* Free the query_context. We don't really expect MemoryContextDelete to
648652
* fail, but just in case, make sure the CachedPlanSource is left in a
@@ -1380,6 +1384,14 @@ CopyCachedPlan(CachedPlanSource *plansource)
13801384
newsource->total_custom_cost = plansource->total_custom_cost;
13811385
newsource->num_custom_plans = plansource->num_custom_plans;
13821386

1387+
/*
1388+
* Copy over the user the query was planned as, and under what RLS
1389+
* environment. We will check during RevalidateCachedQuery() if the user
1390+
* or environment has changed and, if so, will force a re-plan.
1391+
*/
1392+
newsource->planUserId = plansource->planUserId;
1393+
newsource->row_security_env = plansource->row_security_env;
1394+
13831395
MemoryContextSwitchTo(oldcxt);
13841396

13851397
return newsource;

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2334,23 +2334,37 @@ GRANT SELECT ON t1 TO rls_regress_user1, rls_regress_user2;
23342334
CREATE POLICY p1 ON t1 TO rls_regress_user1 USING ((a % 2) = 0);
23352335
CREATE POLICY p2 ON t1 TO rls_regress_user2 USING ((a % 4) = 0);
23362336
ALTER TABLE t1 ENABLE ROW LEVEL SECURITY;
2337+
-- Prepare as rls_regress_user1
23372338
SET ROLE rls_regress_user1;
23382339
PREPARE role_inval AS SELECT * FROM t1;
2340+
-- Check plan
23392341
EXPLAIN (COSTS OFF) EXECUTE role_inval;
23402342
QUERY PLAN
23412343
-------------------------
23422344
Seq Scan on t1
23432345
Filter: ((a % 2) = 0)
23442346
(2 rows)
23452347

2348+
-- Change to rls_regress_user2
23462349
SET ROLE rls_regress_user2;
2350+
-- Check plan- should be different
23472351
EXPLAIN (COSTS OFF) EXECUTE role_inval;
23482352
QUERY PLAN
23492353
-------------------------
23502354
Seq Scan on t1
23512355
Filter: ((a % 4) = 0)
23522356
(2 rows)
23532357

2358+
-- Change back to rls_regress_user1
2359+
SET ROLE rls_regress_user1;
2360+
-- Check plan- should be back to original
2361+
EXPLAIN (COSTS OFF) EXECUTE role_inval;
2362+
QUERY PLAN
2363+
-------------------------
2364+
Seq Scan on t1
2365+
Filter: ((a % 2) = 0)
2366+
(2 rows)
2367+
23542368
--
23552369
-- CTE and RLS
23562370
--

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -852,11 +852,20 @@ CREATE POLICY p2 ON t1 TO rls_regress_user2 USING ((a % 4) = 0);
852852

853853
ALTER TABLE t1 ENABLE ROW LEVEL SECURITY;
854854

855+
-- Prepare as rls_regress_user1
855856
SET ROLE rls_regress_user1;
856857
PREPARE role_inval AS SELECT * FROM t1;
858+
-- Check plan
857859
EXPLAIN (COSTS OFF) EXECUTE role_inval;
858860

861+
-- Change to rls_regress_user2
859862
SET ROLE rls_regress_user2;
863+
-- Check plan- should be different
864+
EXPLAIN (COSTS OFF) EXECUTE role_inval;
865+
866+
-- Change back to rls_regress_user1
867+
SET ROLE rls_regress_user1;
868+
-- Check plan- should be back to original
860869
EXPLAIN (COSTS OFF) EXECUTE role_inval;
861870

862871
--

0 commit comments

Comments
 (0)
0