Skip to content

Commit 21a4edd

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 1cc3a90 commit 21a4edd

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
@@ -4631,6 +4631,52 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid,
46314631
rte->securityQuals == NIL &&
46324632
(pg_class_aclcheck(rte->relid, userid,
46334633
ACL_SELECT) == ACLCHECK_OK);
4634+
4635+
/*
4636+
* If the user doesn't have permissions to
4637+
* access an inheritance child relation, check
4638+
* the permissions of the table actually
4639+
* mentioned in the query, since most likely
4640+
* the user does have that permission. Note
4641+
* that whole-table select privilege on the
4642+
* parent doesn't quite guarantee that the
4643+
* user could read all columns of the child.
4644+
* But in practice it's unlikely that any
4645+
* interesting security violation could result
4646+
* from allowing access to the expression
4647+
* index's stats, so we allow it anyway. See
4648+
* similar code in examine_simple_variable()
4649+
* for additional comments.
4650+
*/
4651+
if (!vardata->acl_ok &&
4652+
root->append_rel_array != NULL)
4653+
{
4654+
AppendRelInfo *appinfo;
4655+
Index varno = index->rel->relid;
4656+
4657+
appinfo = root->append_rel_array[varno];
4658+
while (appinfo &&
4659+
planner_rt_fetch(appinfo->parent_relid,
4660+
root)->rtekind == RTE_RELATION)
4661+
{
4662+
varno = appinfo->parent_relid;
4663+
appinfo = root->append_rel_array[varno];
4664+
}
4665+
if (varno != index->rel->relid)
4666+
{
4667+
/* Repeat access check on this rel */
4668+
rte = planner_rt_fetch(varno, root);
4669+
Assert(rte->rtekind == RTE_RELATION);
4670+
4671+
userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
4672+
4673+
vardata->acl_ok =
4674+
rte->securityQuals == NIL &&
4675+
(pg_class_aclcheck(rte->relid,
4676+
userid,
4677+
ACL_SELECT) == ACLCHECK_OK);
4678+
}
4679+
}
46344680
}
46354681
else
46364682
{
@@ -4708,6 +4754,88 @@ examine_simple_variable(PlannerInfo *root, Var *var,
47084754
ACL_SELECT) == ACLCHECK_OK) ||
47094755
(pg_attribute_aclcheck(rte->relid, var->varattno, userid,
47104756
ACL_SELECT) == ACLCHECK_OK));
4757+
4758+
/*
4759+
* If the user doesn't have permissions to access an inheritance
4760+
* child relation or specifically this attribute, check the
4761+
* permissions of the table/column actually mentioned in the
4762+
* query, since most likely the user does have that permission
4763+
* (else the query will fail at runtime), and if the user can read
4764+
* the column there then he can get the values of the child table
4765+
* too. To do that, we must find out which of the root parent's
4766+
* attributes the child relation's attribute corresponds to.
4767+
*/
4768+
if (!vardata->acl_ok && var->varattno > 0 &&
4769+
root->append_rel_array != NULL)
4770+
{
4771+
AppendRelInfo *appinfo;
4772+
Index varno = var->varno;
4773+
int varattno = var->varattno;
4774+
bool found = false;
4775+
4776+
appinfo = root->append_rel_array[varno];
4777+
4778+
/*
4779+
* Partitions are mapped to their immediate parent, not the
4780+
* root parent, so must be ready to walk up multiple
4781+
* AppendRelInfos. But stop if we hit a parent that is not
4782+
* RTE_RELATION --- that's a flattened UNION ALL subquery, not
4783+
* an inheritance parent.
4784+
*/
4785+
while (appinfo &&
4786+
planner_rt_fetch(appinfo->parent_relid,
4787+
root)->rtekind == RTE_RELATION)
4788+
{
4789+
int parent_varattno;
4790+
ListCell *l;
4791+
4792+
parent_varattno = 1;
4793+
found = false;
4794+
foreach(l, appinfo->translated_vars)
4795+
{
4796+
Var *childvar = lfirst_node(Var, l);
4797+
4798+
/* Ignore dropped attributes of the parent. */
4799+
if (childvar != NULL &&
4800+
varattno == childvar->varattno)
4801+
{
4802+
found = true;
4803+
break;
4804+
}
4805+
parent_varattno++;
4806+
}
4807+
4808+
if (!found)
4809+
break;
4810+
4811+
varno = appinfo->parent_relid;
4812+
varattno = parent_varattno;
4813+
4814+
/* If the parent is itself a child, continue up. */
4815+
appinfo = root->append_rel_array[varno];
4816+
}
4817+
4818+
/*
4819+
* In rare cases, the Var may be local to the child table, in
4820+
* which case, we've got to live with having no access to this
4821+
* column's stats.
4822+
*/
4823+
if (!found)
4824+
return;
4825+
4826+
/* Repeat the access check on this parent rel & column */
4827+
rte = planner_rt_fetch(varno, root);
4828+
Assert(rte->rtekind == RTE_RELATION);
4829+
4830+
userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
4831+
4832+
vardata->acl_ok =
4833+
rte->securityQuals == NIL &&
4834+
((pg_class_aclcheck(rte->relid, userid,
4835+
ACL_SELECT) == ACLCHECK_OK) ||
4836+
(pg_attribute_aclcheck(rte->relid, varattno, userid,
4837+
ACL_SELECT) == ACLCHECK_OK));
4838+
}
47114839
}
47124840
else
47134841
{

‎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)