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

Commit 86ebf30

Browse files
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 d12e5bb commit 86ebf30

File tree

3 files changed

+49
-15
lines changed

3 files changed

+49
-15
lines changed

src/backend/utils/cache/plancache.c

+26-15
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
*
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
@@ -208,8 +209,8 @@ CreateCachedPlan(Node *raw_parse_tree,
208209
plansource->total_custom_cost = 0;
209210
plansource->num_custom_plans = 0;
210211
plansource->hasRowSecurity = false;
211-
plansource->row_security_env = row_security;
212212
plansource->planUserId = InvalidOid;
213+
plansource->row_security_env = false;
213214

214215
MemoryContextSwitchTo(oldcxt);
215216

@@ -275,6 +276,8 @@ CreateOneShotCachedPlan(Node *raw_parse_tree,
275276
plansource->generic_cost = -1;
276277
plansource->total_custom_cost = 0;
277278
plansource->num_custom_plans = 0;
279+
plansource->planUserId = InvalidOid;
280+
plansource->row_security_env = false;
278281

279282
return plansource;
280283
}
@@ -413,6 +416,8 @@ CompleteCachedPlan(CachedPlanSource *plansource,
413416
plansource->cursor_options = cursor_options;
414417
plansource->fixed_result = fixed_result;
415418
plansource->resultDesc = PlanCacheComputeResultDesc(querytree_list);
419+
plansource->planUserId = GetUserId();
420+
plansource->row_security_env = row_security;
416421

417422
MemoryContextSwitchTo(oldcxt);
418423

@@ -575,25 +580,15 @@ RevalidateCachedQuery(CachedPlanSource *plansource)
575580
return NIL;
576581
}
577582

578-
/*
579-
* If this is a new cached plan, then set the user id it was planned by
580-
* and under what row security settings; these are needed to determine
581-
* plan invalidation when RLS is involved or foreign joins are pushed
582-
* down.
583-
*/
584-
if (!OidIsValid(plansource->planUserId))
585-
{
586-
plansource->planUserId = GetUserId();
587-
plansource->row_security_env = row_security;
588-
}
589-
590583
/*
591584
* If the query is currently valid, we should have a saved search_path ---
592585
* check to see if that matches the current environment. If not, we want
593-
* to force replan.
586+
* to force replan. We should also have a valid planUserId.
594587
*/
595588
if (plansource->is_valid)
596589
{
590+
Assert(OidIsValid(plansource->planUserId));
591+
597592
Assert(plansource->search_path != NULL);
598593
if (!OverrideSearchPathMatchesCurrent(plansource->search_path))
599594
{
@@ -660,6 +655,14 @@ RevalidateCachedQuery(CachedPlanSource *plansource)
660655
plansource->invalItems = NIL;
661656
plansource->search_path = NULL;
662657

658+
/*
659+
* The plan is invalid, possibly due to row security, so we need to reset
660+
* row_security_env and planUserId as we're about to re-plan with the
661+
* current settings.
662+
*/
663+
plansource->row_security_env = row_security;
664+
plansource->planUserId = GetUserId();
665+
663666
/*
664667
* Free the query_context. We don't really expect MemoryContextDelete to
665668
* fail, but just in case, make sure the CachedPlanSource is left in a
@@ -1412,6 +1415,14 @@ CopyCachedPlan(CachedPlanSource *plansource)
14121415
newsource->total_custom_cost = plansource->total_custom_cost;
14131416
newsource->num_custom_plans = plansource->num_custom_plans;
14141417

1418+
/*
1419+
* Copy over the user the query was planned as, and under what RLS
1420+
* environment. We will check during RevalidateCachedQuery() if the user
1421+
* or environment has changed and, if so, will force a re-plan.
1422+
*/
1423+
newsource->planUserId = plansource->planUserId;
1424+
newsource->row_security_env = plansource->row_security_env;
1425+
14151426
MemoryContextSwitchTo(oldcxt);
14161427

14171428
return newsource;

src/test/regress/expected/rowsecurity.out

+14
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

+9
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)