Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof? - Mailing list pgsql-hackers

Amit Langote <amitlangote09@gmail.com> writes:
> On Thu, Nov 21, 2019 at 6:34 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The comment for inh_root_relid seems rather inadequate, since it
>> fails to mention the special case for UNION ALL subqueries.
>> But do we even need that special case?  It looks to me like the
>> walk-up-to-parent code is defending against such cases by checking
>> relkind, so maybe we don't need to throw away info for UNION ALL.
>> In general, if we're going to add inh_root_relid, I'd like its
>> definition to be as simple and consistent as possible, because
>> I'm sure there will be other uses for it.  If it could be something
>> like "baserel that this otherrel is a child of", full stop,
>> I think that'd be good.

> If inh_root_relid meant that, it would no longer be useful to
> examine_variable.  In examine_variable, we need to map a child table's
> relid to the relid of its root parent table.  If the root parent
> itself is under a UNION ALL subquery parent, then inh_root_relid of
> all relations in that ancestry chain would point to the UNION ALL
> subquery parent, which is not what examine_variable would want to use,
> because it's really looking for the root "table".

Hm, I see.  Still, the definition seems quite ad-hoc and of uncertain
usefulness to any other use-case.  Given that checking permissions for
access to an expression index's stats is a pretty uncommon thing to
be doing, I don't really want to let it drive the definition of a
new RelOptInfo field.

The other reason that I'm on the warpath against this field is that
it makes the patch un-back-patchable, and I'd like to be able to fix
this problem in the back branches.

Given the existence of the append_rel_array array, it's not really
difficult or expensive to use that to chain up to the root parent,
as in the attached simplified patch.  We could only use this back
to v11 where append_rel_array was added, but that's still a lot
better than no back-patched fix at all.

I've not studied the test case too closely yet, other than to verify
that it does fail without the code fix :-).  Other than that, though,
I think this patch is committable for v11 through HEAD.

            regards, tom lane

diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index 26a2e3b..35dbd72 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -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
         {
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index 44d51ed..0943ba4 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -2335,3 +2335,62 @@ explain (costs off) select * from range_parted order by a desc,b desc,c desc;
 (3 rows)

 drop table range_parted;
+-- Test for checking that lack of access permissions for a child table and
+-- hence its statistics doesn't affect plan shapes when the query is on the
+-- parent table
+create table permtest_parent (a int, b text, c text) partition by list (a);
+create table permtest_child (b text, a int, c text) 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;
+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)
+
+create role regress_no_child_access;
+revoke all on permtest_grandchild from regress_no_child_access;
+grant all on permtest_parent to regress_no_child_access;
+set session authorization regress_no_child_access;
+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;
+drop role regress_no_child_access;
+drop table permtest_parent;
diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql
index 3af1bf3..ca21bbb 100644
--- a/src/test/regress/sql/inherit.sql
+++ b/src/test/regress/sql/inherit.sql
@@ -845,3 +845,27 @@ 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;
+
+-- Test for checking that lack of access permissions for a child table and
+-- hence its statistics doesn't affect plan shapes when the query is on the
+-- parent table
+create table permtest_parent (a int, b text, c text) partition by list (a);
+create table permtest_child (b text, a int, c text) 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;
+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$';
+create role regress_no_child_access;
+revoke all on permtest_grandchild from regress_no_child_access;
+grant all on permtest_parent to regress_no_child_access;
+set session authorization regress_no_child_access;
+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;
+drop role regress_no_child_access;
+drop table permtest_parent;