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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Amit Langote <amitlangote09(at)gmail(dot)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