Re: [v9.2] Fix Leaky View Problem

2011/10/10 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Sun, Oct 9, 2011 at 11:50 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> I tried to refactor the patches based on the interface of WITH (...)
>> and usage of
>> pg_class.reloptions, although here is no functionality changes; including the
>> behavior when a view is replaced.
>>
>> My preference is WITH (...) interface, however, it is not a strong one.
>> So, I hope either of versions being reviewed.
>
> I spent some more time looking at this, and I guess I'm pretty unsold
> on the whole approach.  In the part 2 patch, for example, we're doing
> this:
>
> +static bool
> +mark_qualifiers_depth_walker(Node *node, void *context)
> +{
> +       int             depth = *((int *)(context));
> +
... <snip> ...
> +       else if (IsA(node, RowCompareExpr))
> +       {
> +               ((RowCompareExpr *)node)->depth = depth;
> +       }
> +       return expression_tree_walker(node,
> mark_qualifiers_depth_walker, context);
> +}
>
> It seems really ugly to me to suppose that we need to add a depth
> field to every single one of these node types.  If you've missed one,
> then we have a security hole.  If someone else adds another node type
> later that requires this field and doesn't add it, we have a security
> hole.  And since all of these depth fields are going to make their way
> into stored rules, those security holes will require an initdb to fix.
>
Indeed, I have to admit this disadvantage from the perspective of code
maintenance, because it had also been a tough work for me to track
the depth field in this patch.

> If we make security views work like this, then we don't need to have
> one mechanism to sort quals by depth and another to prevent them from
> being pushed down through joins.  It all just works.  Now, there is
> one problem: if snarf() were a non-leaky function rather than a
> maliciously crafted one, it still wouldn't get pushed down:
>
Rather than my original design, I'm learning to the idea to keep
sub-queries come from security views; without flatten, because
of its straightforwardness.

> If we make security views work like this, then we don't need to have
> one mechanism to sort quals by depth and another to prevent them from
> being pushed down through joins.  It all just works.  Now, there is
> one problem: if snarf() were a non-leaky function rather than a
> maliciously crafted one, it still wouldn't get pushed down:
>
I agreed. We have been on the standpoint that tries to prevent
leakable functions to reference a portion of join-tree being already
flatten, however, it has been a tough work.
It seems to me it is much simple approach that enables to push
down only non-leaky functions into inside of sub-queries.

An idea is to add a hack on distribute_qual_to_rels() to relocate
a qualifier into inside of the sub-query, when it references only
a particular sub-query being come from a security view, and
when the sub-query satisfies is_simple_subquery(), for example.

Anyway, I'll try to tackle this long standing problem with this
approach in the next commit-fest.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>