Skip to content

Commit 553d2ec

Browse files
tglsfdctglsfdc
committed
Allow access to child table statistics if user can read parent table.
The fix for CVE-2017-7484 disallowed use of pg_statistic data for planning purposes if the user would not be able to select the associated column and a non-leakproof function is to be applied to the statistics values. That turns out to disable use of pg_statistic data in some common cases involving inheritance/partitioning, where the user does have permission to select from the parent table that was actually named in the query, but not from a child table whose stats are needed. Since, in non-corner cases, the user *can* select the child table's data via the parent, this restriction is not actually useful from a security standpoint. Improve the logic so that we also check the permissions of the originally-named table, and allow access if select permission exists for that. When checking access to stats for a simple child column, we can map the child column number back to the parent, and perform this test exactly (including not allowing access if the child column isn't exposed by the parent). For expression indexes, the current logic just insists on whole-table select access, and this patch allows access if the user can select the whole parent table. In principle, if the child table has extra columns, this might allow access to stats on columns the user can't read. In practice, it's unlikely that the planner is going to do any stats calculations involving expressions that are not visible to the query, so we'll ignore that fine point for now. Perhaps someday we'll improve that logic to detect exactly which columns are used by an expression index ... but today is not that day. Back-patch to v11. The issue was created in 9.2 and up by the CVE-2017-7484 fix, but this patch depends on the append_rel_array[] planner data structure which only exists in v11 and up. In practice the issue is most urgent with partitioned tables, so fixing v11 and later should satisfy much of the practical need. Dilip Kumar and Amit Langote, with some kibitzing by me Discussion: https://postgr.es/m/3876.1531261875@sss.pgh.pa.us
1 parent 1219823 commit 553d2ec

File tree

3 files changed

+240
-0
lines changed

3 files changed

+240
-0
lines changed

‎src/backend/utils/adt/selfuncs.c

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4613,6 +4613,52 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid,
46134613
rte->securityQuals == NIL &&
46144614
(pg_class_aclcheck(rte->relid, userid,
46154615
ACL_SELECT) == ACLCHECK_OK);
4616+
4617+
/*
4618+
* If the user doesn't have permissions to
4619+
* access an inheritance child relation, check
4620+
* the permissions of the table actually
4621+
* mentioned in the query, since most likely
4622+
* the user does have that permission. Note
4623+
* that whole-table select privilege on the
4624+
* parent doesn't quite guarantee that the
4625+
* user could read all columns of the child.
4626+
* But in practice it's unlikely that any
4627+
* interesting security violation could result
4628+
* from allowing access to the expression
4629+
* index's stats, so we allow it anyway. See
4630+
* similar code in examine_simple_variable()
4631+
* for additional comments.
4632+
*/
4633+
if (!vardata->acl_ok &&
4634+
root->append_rel_array != NULL)
4635+
{
4636+
AppendRelInfo *appinfo;
4637+
Index varno = index->rel->relid;
4638+
4639+
appinfo = root->append_rel_array[varno];
4640+
while (appinfo &&
4641+
planner_rt_fetch(appinfo->parent_relid,
4642+
root)->rtekind == RTE_RELATION)
4643+
{
4644+
varno = appinfo->parent_relid;
4645+
appinfo = root->append_rel_array[varno];
4646+
}
4647+
if (varno != index->rel->relid)
4648+
{
4649+
/* Repeat access check on this rel */
4650+
rte = planner_rt_fetch(varno, root);
4651+
Assert(rte->rtekind == RTE_RELATION);
4652+
4653+
userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
4654+
4655+
vardata->acl_ok =
4656+
rte->securityQuals == NIL &&
4657+
(pg_class_aclcheck(rte->relid,
4658+
userid,
4659+
ACL_SELECT) == ACLCHECK_OK);
4660+
}
4661+
}
46164662
}
46174663
else
46184664
{
@@ -4690,6 +4736,88 @@ examine_simple_variable(PlannerInfo *root, Var *var,
46904736
ACL_SELECT) == ACLCHECK_OK) ||
46914737
(pg_attribute_aclcheck(rte->relid, var->varattno, userid,
46924738
ACL_SELECT) == ACLCHECK_OK));
4739+
4740+
/*
4741+
* If the user doesn't have permissions to access an inheritance
4742+
* child relation or specifically this attribute, check the
4743+
* permissions of the table/column actually mentioned in the
4744+
* query, since most likely the user does have that permission
4745+
* (else the query will fail at runtime), and if the user can read
4746+
* the column there then he can get the values of the child table
4747+
* too. To do that, we must find out which of the root parent's
4748+
* attributes the child relation's attribute corresponds to.
4749+
*/
4750+
if (!vardata->acl_ok && var->varattno > 0 &&
4751+
root->append_rel_array != NULL)
4752+
{
4753+
AppendRelInfo *appinfo;
4754+
Index varno = var->varno;
4755+
int varattno = var->varattno;
4756+
bool found = false;
4757+
4758+
appinfo = root->append_rel_array[varno];
4759+
4760+
/*
4761+
* Partitions are mapped to their immediate parent, not the
4762+
* root parent, so must be ready to walk up multiple
4763+
* AppendRelInfos. But stop if we hit a parent that is not
4764+
* RTE_RELATION --- that's a flattened UNION ALL subquery, not
4765+
* an inheritance parent.
4766+
*/
4767+
while (appinfo &&
4768+
planner_rt_fetch(appinfo->parent_relid,
4769+
root)->rtekind == RTE_RELATION)
4770+
{
4771+
int parent_varattno;
4772+
ListCell *l;
4773+
4774+
parent_varattno = 1;
4775+
found = false;
4776+
foreach(l, appinfo->translated_vars)
4777+
{
4778+
Var *childvar = lfirst_node(Var, l);
4779+
4780+
/* Ignore dropped attributes of the parent. */
4781+
if (childvar != NULL &&
4782+
varattno == childvar->varattno)
4783+
{
4784+
found = true;
4785+
break;
4786+
}
4787+
parent_varattno++;
4788+
}
4789+
4790+
if (!found)
4791+
break;
4792+
4793+
varno = appinfo->parent_relid;
4794+
varattno = parent_varattno;
4795+
4796+
/* If the parent is itself a child, continue up. */
4797+
appinfo = root->append_rel_array[varno];
4798+
}
4799+
4800+
/*
4801+
* In rare cases, the Var may be local to the child table, in
4802+
* which case, we've got to live with having no access to this
4803+
* column's stats.
4804+
*/
4805+
if (!found)
4806+
return;
4807+
4808+
/* Repeat the access check on this parent rel & column */
4809+
rte = planner_rt_fetch(varno, root);
4810+
Assert(rte->rtekind == RTE_RELATION);
4811+
4812+
userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
4813+
4814+
vardata->acl_ok =
4815+
rte->securityQuals == NIL &&
4816+
((pg_class_aclcheck(rte->relid, userid,
4817+
ACL_SELECT) == ACLCHECK_OK) ||
4818+
(pg_attribute_aclcheck(rte->relid, varattno, userid,
4819+
ACL_SELECT) == ACLCHECK_OK));
4820+
}
46934821
}
46944822
else
46954823
{

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

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2335,3 +2335,77 @@ explain (costs off) select * from range_parted order by a desc,b desc,c desc;
23352335
(3 rows)
23362336

23372337
drop table range_parted;
2338+
-- Check that we allow access to a child table's statistics when the user
2339+
-- has permissions only for the parent table.
2340+
create table permtest_parent (a int, b text, c text) partition by list (a);
2341+
create table permtest_child (b text, c text, a int) partition by list (b);
2342+
create table permtest_grandchild (c text, b text, a int);
2343+
alter table permtest_child attach partition permtest_grandchild for values in ('a');
2344+
alter table permtest_parent attach partition permtest_child for values in (1);
2345+
create index on permtest_parent (left(c, 3));
2346+
insert into permtest_parent
2347+
select 1, 'a', left(md5(i::text), 5) from generate_series(0, 100) i;
2348+
analyze permtest_parent;
2349+
create role regress_no_child_access;
2350+
revoke all on permtest_grandchild from regress_no_child_access;
2351+
grant select on permtest_parent to regress_no_child_access;
2352+
set session authorization regress_no_child_access;
2353+
-- without stats access, these queries would produce hash join plans:
2354+
explain (costs off)
2355+
select * from permtest_parent p1 inner join permtest_parent p2
2356+
on p1.a = p2.a and p1.c ~ 'a1$';
2357+
QUERY PLAN
2358+
------------------------------------------
2359+
Nested Loop
2360+
Join Filter: (p1.a = p2.a)
2361+
-> Seq Scan on permtest_grandchild p1
2362+
Filter: (c ~ 'a1$'::text)
2363+
-> Seq Scan on permtest_grandchild p2
2364+
(5 rows)
2365+
2366+
explain (costs off)
2367+
select * from permtest_parent p1 inner join permtest_parent p2
2368+
on p1.a = p2.a and left(p1.c, 3) ~ 'a1$';
2369+
QUERY PLAN
2370+
----------------------------------------------
2371+
Nested Loop
2372+
Join Filter: (p1.a = p2.a)
2373+
-> Seq Scan on permtest_grandchild p1
2374+
Filter: ("left"(c, 3) ~ 'a1$'::text)
2375+
-> Seq Scan on permtest_grandchild p2
2376+
(5 rows)
2377+
2378+
reset session authorization;
2379+
revoke all on permtest_parent from regress_no_child_access;
2380+
grant select(a,c) on permtest_parent to regress_no_child_access;
2381+
set session authorization regress_no_child_access;
2382+
explain (costs off)
2383+
select p2.a, p1.c from permtest_parent p1 inner join permtest_parent p2
2384+
on p1.a = p2.a and p1.c ~ 'a1$';
2385+
QUERY PLAN
2386+
------------------------------------------
2387+
Nested Loop
2388+
Join Filter: (p1.a = p2.a)
2389+
-> Seq Scan on permtest_grandchild p1
2390+
Filter: (c ~ 'a1$'::text)
2391+
-> Seq Scan on permtest_grandchild p2
2392+
(5 rows)
2393+
2394+
-- we will not have access to the expression index's stats here:
2395+
explain (costs off)
2396+
select p2.a, p1.c from permtest_parent p1 inner join permtest_parent p2
2397+
on p1.a = p2.a and left(p1.c, 3) ~ 'a1$';
2398+
QUERY PLAN
2399+
----------------------------------------------------
2400+
Hash Join
2401+
Hash Cond: (p2.a = p1.a)
2402+
-> Seq Scan on permtest_grandchild p2
2403+
-> Hash
2404+
-> Seq Scan on permtest_grandchild p1
2405+
Filter: ("left"(c, 3) ~ 'a1$'::text)
2406+
(6 rows)
2407+
2408+
reset session authorization;
2409+
revoke all on permtest_parent from regress_no_child_access;
2410+
drop role regress_no_child_access;
2411+
drop table permtest_parent;

‎src/test/regress/sql/inherit.sql

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -845,3 +845,41 @@ explain (costs off) select * from range_parted order by a,b,c;
845845
explain (costs off) select * from range_parted order by a desc,b desc,c desc;
846846

847847
drop table range_parted;
848+
849+
-- Check that we allow access to a child table's statistics when the user
850+
-- has permissions only for the parent table.
851+
create table permtest_parent (a int, b text, c text) partition by list (a);
852+
create table permtest_child (b text, c text, a int) partition by list (b);
853+
create table permtest_grandchild (c text, b text, a int);
854+
alter table permtest_child attach partition permtest_grandchild for values in ('a');
855+
alter table permtest_parent attach partition permtest_child for values in (1);
856+
create index on permtest_parent (left(c, 3));
857+
insert into permtest_parent
858+
select 1, 'a', left(md5(i::text), 5) from generate_series(0, 100) i;
859+
analyze permtest_parent;
860+
create role regress_no_child_access;
861+
revoke all on permtest_grandchild from regress_no_child_access;
862+
grant select on permtest_parent to regress_no_child_access;
863+
set session authorization regress_no_child_access;
864+
-- without stats access, these queries would produce hash join plans:
865+
explain (costs off)
866+
select * from permtest_parent p1 inner join permtest_parent p2
867+
on p1.a = p2.a and p1.c ~ 'a1$';
868+
explain (costs off)
869+
select * from permtest_parent p1 inner join permtest_parent p2
870+
on p1.a = p2.a and left(p1.c, 3) ~ 'a1$';
871+
reset session authorization;
872+
revoke all on permtest_parent from regress_no_child_access;
873+
grant select(a,c) on permtest_parent to regress_no_child_access;
874+
set session authorization regress_no_child_access;
875+
explain (costs off)
876+
select p2.a, p1.c from permtest_parent p1 inner join permtest_parent p2
877+
on p1.a = p2.a and p1.c ~ 'a1$';
878+
-- we will not have access to the expression index's stats here:
879+
explain (costs off)
880+
select p2.a, p1.c from permtest_parent p1 inner join permtest_parent p2
881+
on p1.a = p2.a and left(p1.c, 3) ~ 'a1$';
882+
reset session authorization;
883+
revoke all on permtest_parent from regress_no_child_access;
884+
drop role regress_no_child_access;
885+
drop table permtest_parent;

0 commit comments

Comments
 (0)