Allow access to child table statistics if user can read parent table.
authorTom Lane <[email protected]>
Tue, 26 Nov 2019 19:41:48 +0000 (14:41 -0500)
committerTom Lane <[email protected]>
Tue, 26 Nov 2019 19:41:48 +0000 (14:41 -0500)
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-proof 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  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- to v11.  The issue was created in 9.2 and up by the
CVE-2017-7484 fix, but this  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

src/backend/utils/adt/selfuncs.c
src/test/regress/expected/inherit.out
src/test/regress/sql/inherit.sql

index 26a2e3bb8b8ecbbfecdb1790932a647a515cfc2e..35dbd728ecf3c54a7e634f82abd7580fc7c8d41a 100644 (file)
@@ -4613,6 +4613,52 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid,
                                                                        rte->securityQuals == NIL &&
                                                                        (pg_class_aclcheck(rte->relid, userid,
                                                                                                           ACL_SELECT) == ACLCHECK_OK);
+
+                                                               /*
+                                                                * If the user doesn't have permissions to
+                                                                * access an inheritance child relation, check
+                                                                * the permissions of the table actually
+                                                                * mentioned in the query, since most likely
+                                                                * the user does have that permission.  Note
+                                                                * that whole-table select privilege on the
+                                                                * parent doesn't quite guarantee that the
+                                                                * user could read all columns of the child.
+                                                                * But in practice it's unlikely that any
+                                                                * interesting security violation could result
+                                                                * from allowing access to the expression
+                                                                * index's stats, so we allow it anyway.  See
+                                                                * similar code in examine_simple_variable()
+                                                                * for additional comments.
+                                                                */
+                                                               if (!vardata->acl_ok &&
+                                                                       root->append_rel_array != NULL)
+                                                               {
+                                                                       AppendRelInfo *appinfo;
+                                                                       Index           varno = index->rel->relid;
+
+                                                                       appinfo = root->append_rel_array[varno];
+                                                                       while (appinfo &&
+                                                                                  planner_rt_fetch(appinfo->parent_relid,
+                                                                                                                       root)->rtekind == RTE_RELATION)
+                                                                       {
+                                                                               varno = appinfo->parent_relid;
+                                                                               appinfo = root->append_rel_array[varno];
+                                                                       }
+                                                                       if (varno != index->rel->relid)
+                                                                       {
+                                                                               /* Repeat access check on this rel */
+                                                                               rte = planner_rt_fetch(varno, root);
+                                                                               Assert(rte->rtekind == RTE_RELATION);
+
+                                                                               userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
+
+                                                                               vardata->acl_ok =
+                                                                                       rte->securityQuals == NIL &&
+                                                                                       (pg_class_aclcheck(rte->relid,
+                                                                                                                          userid,
+                                                                                                                          ACL_SELECT) == ACLCHECK_OK);
+                                                                       }
+                                                               }
                                                        }
                                                        else
                                                        {
@@ -4690,6 +4736,88 @@ examine_simple_variable(PlannerInfo *root, Var *var,
                                                                        ACL_SELECT) == ACLCHECK_OK) ||
                                 (pg_attribute_aclcheck(rte->relid, var->varattno, userid,
                                                                                ACL_SELECT) == ACLCHECK_OK));
+
+                       /*
+                        * If the user doesn't have permissions to access an inheritance
+                        * child relation or specifically this attribute, check the
+                        * permissions of the table/column actually mentioned in the
+                        * query, since most likely the user does have that permission
+                        * (else the query will fail at runtime), and if the user can read
+                        * the column there then he can get the values of the child table
+                        * too.  To do that, we must find out which of the root parent's
+                        * attributes the child relation's attribute corresponds to.
+                        */
+                       if (!vardata->acl_ok && var->varattno > 0 &&
+                               root->append_rel_array != NULL)
+                       {
+                               AppendRelInfo *appinfo;
+                               Index           varno = var->varno;
+                               int                     varattno = var->varattno;
+                               bool            found = false;
+
+                               appinfo = root->append_rel_array[varno];
+
+                               /*
+                                * Partitions are mapped to their immediate parent, not the
+                                * root parent, so must be ready to walk up multiple
+                                * AppendRelInfos.  But stop if we hit a parent that is not
+                                * RTE_RELATION --- that's a flattened UNION ALL subquery, not
+                                * an inheritance parent.
+                                */
+                               while (appinfo &&
+                                          planner_rt_fetch(appinfo->parent_relid,
+                                                                               root)->rtekind == RTE_RELATION)
+                               {
+                                       int                     parent_varattno;
+                                       ListCell   *l;
+
+                                       parent_varattno = 1;
+                                       found = false;
+                                       foreach(l, appinfo->translated_vars)
+                                       {
+                                               Var                *childvar = lfirst_node(Var, l);
+
+                                               /* Ignore dropped attributes of the parent. */
+                                               if (childvar != NULL &&
+                                                       varattno == childvar->varattno)
+                                               {
+                                                       found = true;
+                                                       break;
+                                               }
+                                               parent_varattno++;
+                                       }
+
+                                       if (!found)
+                                               break;
+
+                                       varno = appinfo->parent_relid;
+                                       varattno = parent_varattno;
+
+                                       /* If the parent is itself a child, continue up. */
+                                       appinfo = root->append_rel_array[varno];
+                               }
+
+                               /*
+                                * In rare cases, the Var may be local to the child table, in
+                                * which case, we've got to live with having no access to this
+                                * column's stats.
+                                */
+                               if (!found)
+                                       return;
+
+                               /* Repeat the access check on this parent rel & column */
+                               rte = planner_rt_fetch(varno, root);
+                               Assert(rte->rtekind == RTE_RELATION);
+
+                               userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
+
+                               vardata->acl_ok =
+                                       rte->securityQuals == NIL &&
+                                       ((pg_class_aclcheck(rte->relid, userid,
+                                                                               ACL_SELECT) == ACLCHECK_OK) ||
+                                        (pg_attribute_aclcheck(rte->relid, varattno, userid,
+                                                                                       ACL_SELECT) == ACLCHECK_OK));
+                       }
                }
                else
                {
index 44d51ed71102781fe786bab5b3ca4dd80551ea56..e4dbbad75c1343a2d1319fdf7b28485d1e01a123 100644 (file)
@@ -2335,3 +2335,77 @@ explain (costs off) select * from range_parted order by a desc,b desc,c desc;
 (3 rows)
 
 drop table range_parted;
+-- Check that we allow access to a child table's statistics when the user
+-- has permissions only for the parent table.
+create table permtest_parent (a int, b text, c text) partition by list (a);
+create table permtest_child (b text, c text, a int) partition by list (b);
+create table permtest_grandchild (c text, b text, a int);
+alter table permtest_child attach partition permtest_grandchild for values in ('a');
+alter table permtest_parent attach partition permtest_child for values in (1);
+create index on permtest_parent (left(c, 3));
+insert into permtest_parent
+  select 1, 'a', left(md5(i::text), 5) from generate_series(0, 100) i;
+analyze permtest_parent;
+create role regress_no_child_access;
+revoke all on permtest_grandchild from regress_no_child_access;
+grant select on permtest_parent to regress_no_child_access;
+set session authorization regress_no_child_access;
+-- without stats access, these queries would produce hash join plans:
+explain (costs off)
+  select * from permtest_parent p1 inner join permtest_parent p2
+  on p1.a = p2.a and p1.c ~ 'a1$';
+                QUERY PLAN                
+------------------------------------------
+ Nested Loop
+   Join Filter: (p1.a = p2.a)
+   ->  Seq Scan on permtest_grandchild p1
+         Filter: (c ~ 'a1$'::text)
+   ->  Seq Scan on permtest_grandchild p2
+(5 rows)
+
+explain (costs off)
+  select * from permtest_parent p1 inner join permtest_parent p2
+  on p1.a = p2.a and left(p1.c, 3) ~ 'a1$';
+                  QUERY PLAN                  
+----------------------------------------------
+ Nested Loop
+   Join Filter: (p1.a = p2.a)
+   ->  Seq Scan on permtest_grandchild p1
+         Filter: ("left"(c, 3) ~ 'a1$'::text)
+   ->  Seq Scan on permtest_grandchild p2
+(5 rows)
+
+reset session authorization;
+revoke all on permtest_parent from regress_no_child_access;
+grant select(a,c) on permtest_parent to regress_no_child_access;
+set session authorization regress_no_child_access;
+explain (costs off)
+  select p2.a, p1.c from permtest_parent p1 inner join permtest_parent p2
+  on p1.a = p2.a and p1.c ~ 'a1$';
+                QUERY PLAN                
+------------------------------------------
+ Nested Loop
+   Join Filter: (p1.a = p2.a)
+   ->  Seq Scan on permtest_grandchild p1
+         Filter: (c ~ 'a1$'::text)
+   ->  Seq Scan on permtest_grandchild p2
+(5 rows)
+
+-- we will not have access to the expression index's stats here:
+explain (costs off)
+  select p2.a, p1.c from permtest_parent p1 inner join permtest_parent p2
+  on p1.a = p2.a and left(p1.c, 3) ~ 'a1$';
+                     QUERY PLAN                     
+----------------------------------------------------
+ Hash Join
+   Hash Cond: (p2.a = p1.a)
+   ->  Seq Scan on permtest_grandchild p2
+   ->  Hash
+         ->  Seq Scan on permtest_grandchild p1
+               Filter: ("left"(c, 3) ~ 'a1$'::text)
+(6 rows)
+
+reset session authorization;
+revoke all on permtest_parent from regress_no_child_access;
+drop role regress_no_child_access;
+drop table permtest_parent;
index 3af1bf30a79bb980fad52dd6377db1a145eda3f5..317f951fa55d4957d72a736c72019f264b222adf 100644 (file)
@@ -845,3 +845,41 @@ explain (costs off) select * from range_parted order by a,b,c;
 explain (costs off) select * from range_parted order by a desc,b desc,c desc;
 
 drop table range_parted;
+
+-- Check that we allow access to a child table's statistics when the user
+-- has permissions only for the parent table.
+create table permtest_parent (a int, b text, c text) partition by list (a);
+create table permtest_child (b text, c text, a int) partition by list (b);
+create table permtest_grandchild (c text, b text, a int);
+alter table permtest_child attach partition permtest_grandchild for values in ('a');
+alter table permtest_parent attach partition permtest_child for values in (1);
+create index on permtest_parent (left(c, 3));
+insert into permtest_parent
+  select 1, 'a', left(md5(i::text), 5) from generate_series(0, 100) i;
+analyze permtest_parent;
+create role regress_no_child_access;
+revoke all on permtest_grandchild from regress_no_child_access;
+grant select on permtest_parent to regress_no_child_access;
+set session authorization regress_no_child_access;
+-- without stats access, these queries would produce hash join plans:
+explain (costs off)
+  select * from permtest_parent p1 inner join permtest_parent p2
+  on p1.a = p2.a and p1.c ~ 'a1$';
+explain (costs off)
+  select * from permtest_parent p1 inner join permtest_parent p2
+  on p1.a = p2.a and left(p1.c, 3) ~ 'a1$';
+reset session authorization;
+revoke all on permtest_parent from regress_no_child_access;
+grant select(a,c) on permtest_parent to regress_no_child_access;
+set session authorization regress_no_child_access;
+explain (costs off)
+  select p2.a, p1.c from permtest_parent p1 inner join permtest_parent p2
+  on p1.a = p2.a and p1.c ~ 'a1$';
+-- we will not have access to the expression index's stats here:
+explain (costs off)
+  select p2.a, p1.c from permtest_parent p1 inner join permtest_parent p2
+  on p1.a = p2.a and left(p1.c, 3) ~ 'a1$';
+reset session authorization;
+revoke all on permtest_parent from regress_no_child_access;
+drop role regress_no_child_access;
+drop table permtest_parent;