Re: [v9.2] Fix leaky-view problem, part 2 - Mailing list pgsql-hackers

On Sun, Jul 03, 2011 at 11:41:47AM +0200, Kohei KaiGai wrote:
> The simplified version of fix-leaky-view patch. The part of reloptions
> for views got splitted out
> into the part-0 patch, so it needs to be applied prior to this patch.
> Rest of logic to prevent unexpected pushing down across security
> barrier is not changed.
> 
> Thanks,
> 
> 2011/6/6 Kohei Kaigai <Kohei.Kaigai@emea.nec.com>:
> > This patch enables to fix up leaky-view problem using qualifiers that reference only one-side of join-loop inside
ofview definition.
 
> >
> > The point of this scenario is criteria to distribute qualifiers of scanning-plan distributed in
distribute_qual_to_rels().If and when a qualifiers that reference only one-side of join-loop, the optimizer may
distributethis qualifier into inside of the join-loop, even if it goes over the boundary of a subquery expanded from a
viewfor row-level security.
 
> > This behavior allows us to reference whole of one-side of join-loop using functions with side-effects.
> > The solution is quite simple; it prohibits to distribute qualifiers over the boundary of subquery, however,
performancecost is unignorable, because it also disables to utilize obviously indexable qualifiers such as (id=123), so
thispatch requires users a hint whether a particular view is for row-level security, or not.
 
> >
> > This patch newly adds "CREATE SECURITY VIEW" statement that marks a flag to show this view was defined for
row-levelsecurity purpose. This flag shall be stored as reloptions.
 
> > If this flag was set, the optimizer does not distribute qualifiers over the boundary of subqueries expanded from
securityviews, except for obviously safe qualifiers.
 
> > (Right now, we consider built-in indexable operators are safe, but it might be arguable.)

I took a moderately-detailed look at this patch.  This jumped out:

> --- a/src/backend/optimizer/util/clauses.c
> +++ b/src/backend/optimizer/util/clauses.c

> +static bool
> +contain_leakable_functions_walker(Node *node, void *context)
> +{
> +    if (node == NULL)
> +        return false;
> +
> +    if (IsA(node, FuncExpr))
> +    {
> +        /*
> +         * Right now, we have no way to distinguish safe functions with
> +         * leakable ones, so, we treat all the function call possibly
> +         * leakable.
> +         */
> +        return true;
> +    }
> +    else if (IsA(node, OpExpr))
> +    {
> +        OpExpr *expr = (OpExpr *) node;
> +
> +        /*
> +         * Right now, we assume operators implemented by built-in functions
> +         * are not leakable, so it does not need to prevent optimization.
> +         */
> +        set_opfuncid(expr);
> +        if (get_func_lang(expr->opfuncid) != INTERNALlanguageId)
> +            return true;
> +        /* else fall through to check args */
> +    }

Any user can do this:
CREATE OPERATOR !-! (PROCEDURE = int4in, RIGHTARG = cstring);SELECT !-! 'foo';

Making a distinction based simply on the call being an operator vs. a function
is a dead end.  I see these options:

1. The user defining a security view can be assumed to trust the operator class
members of indexes defined on the tables he references.  Keep track of which
those are and treat only them as non-leakable.  This covers many interesting
cases, but it's probably tricky to implement and/or costly at runtime.

2. Add a pg_proc flag indicating whether the function is known leak-free.
Simple, but tedious and perhaps error-prone.

3. Trust operators owned by PGUID.  This is simple and probably covers the
essential cases, but it's an ugly hack.

4. Trust nothing as leak-free.  Simple; performance will be unattractive.

There are probably others.