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

On Wed, Sep 4, 2019 at 8:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Langote <amitlangote09@gmail.com> writes:
> > [ v2-0001-Use-root-parent-s-permissions-when-read-child-tab.patch ]
>
> I took a quick look through this.  I have some cosmetic thoughts and
> also a couple of substantive concerns:

Thanks a lot for reviewing this.

> * As a rule, patches that add fields at the end of a struct are wrong.
> There is almost always some more-appropriate place to put the field
> based on its semantics.  We don't want our struct layouts to be historical
> annals; they should reflect a coherent design.  In this case I'd be
> inclined to put the new field next to the regular relid field.  It
> should have a name that's less completely unrelated to relid, too.

I've renamed the field to inh_root_relid and placed it next to relid.

> * It might make more sense to define the new field as "top parent's relid,
> or self if no parent", rather than "... or zero if no parent".  Then you
> don't need if-tests like this:
>
> +                                if (rel->inh_root_parent > 0)
> +                                    rte = planner_rt_fetch(rel->inh_root_parent,
> +                                                           root);
> +                                else
> +                                    rte = planner_rt_fetch(rel->relid, root);

Agreed, done this way.

> * The business about reverse mapping Vars seems quite inefficient, but
> what's much worse is that it only accounts for one level of parent.
> I'm pretty certain this will give the wrong answer for multiple levels
> of partitioning, if the column numbers aren't all the same.

Indeed.  We need to be checking the root parent's permissions, not the
immediate parent's which might be a child itself.

> * To fix the above, you probably need to map back one inheritance level
> at a time, which suggests that you could just use the AppendRelInfo
> parent-rel info and not need any addition to RelOptInfo at all.  This
> makes the efficiency issue even worse, though.  I don't immediately have a
> great idea about doing better.  Making it faster might require adding more
> info to AppendRelInfos, and I'm not quite sure if it's worth the tradeoff.

Hmm, yes.  If AppendRelInfos had contained a reverse translation list
that maps Vars of a given child to the root parent's, this patch would
end up being much simpler and not add too much cost to the selectivity
code.  However building such a map would not be free and the number of
places where it's useful would be significantly smaller where the
existing parent-to-child translation list is used.

Anyway, I've fixed the above-mentioned oversights in the current code for now.

> * I'd be inclined to use an actual test-and-elog not just an Assert
> for the no-mapping-found case.  For one reason, some compilers are
> going to complain about a set-but-not-used variable in non-assert
> builds.  More importantly, I'm not very convinced that it's impossible
> to hit the no-mapping case.  The original proposal was to fall back
> to current behavior (test the child-table permissions) if we couldn't
> match the var to the top parent, and I think that that is still a
> sane proposal.

OK, I've removed the Assert.  For child Vars that can't be translated
to root parent's, permissions are checked with the child relation,
like before.


> As for how to test, it doesn't seem like it should be that hard to devise
> a situation where you'll get different plan shapes depending on whether
> the planner has an accurate or default selectivity estimate.

I managed to find a test case by trial-and-error, but it may be more
convoluted than it has to be.

-- On HEAD
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);
insert into permtest_parent select 1, 'a', i || 'x' || i * 10 from
generate_series(1, 1000) 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 like '4x5%';
                                   QUERY PLAN
---------------------------------------------------------------------------------
 Nested Loop  (cost=0.00..47.00 rows=1000 width=28)
   Join Filter: (p1.a = p2.a)
   ->  Seq Scan on permtest_grandchild p1  (cost=0.00..18.50 rows=1 width=14)
         Filter: (c ~~ '4x5%'::text)
   ->  Seq Scan on permtest_grandchild p2  (cost=0.00..16.00 rows=1000 width=14)
(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 like '4x5%';
                                     QUERY PLAN
------------------------------------------------------------------------------------
 Hash Join  (cost=18.56..93.31 rows=5000 width=28)
   Hash Cond: (p2.a = p1.a)
   ->  Seq Scan on permtest_grandchild p2  (cost=0.00..16.00 rows=1000 width=14)
   ->  Hash  (cost=18.50..18.50 rows=5 width=14)
         ->  Seq Scan on permtest_grandchild p1  (cost=0.00..18.50
rows=5 width=14)
               Filter: (c ~~ '4x5%'::text)
(6 rows)

-- Patched:

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 like '4x5%';
                                   QUERY PLAN
---------------------------------------------------------------------------------
 Nested Loop  (cost=0.00..47.00 rows=1000 width=28)
   Join Filter: (p1.a = p2.a)
   ->  Seq Scan on permtest_grandchild p1  (cost=0.00..18.50 rows=1 width=14)
         Filter: (c ~~ '4x5%'::text)
   ->  Seq Scan on permtest_grandchild p2  (cost=0.00..16.00 rows=1000 width=14)
(5 rows)

Updated patch attached.

Thanks,
Amit