Re: [v9.2] Fix leaky-view problem, part 1

2011/6/28 Noah Misch <noah(at)leadboat(dot)com>:
> On Tue, Jun 28, 2011 at 10:11:59PM +0200, Kohei KaiGai wrote:
>> 2011/6/28 Noah Misch <noah(at)leadboat(dot)com>:
>> > Suppose your query references two views owned by different roles. ?The quals
>> > of those views will have the same depth. ?Is there a way for information to
>> > leak from one view owner to another due to that?
>> >
>> Even if multiple subqueries were pulled-up and qualifiers got merged, user given
>> qualifiers shall have smaller depth value, so it will be always
>> launched after the
>> qualifiers originally used in the subqueries.
>>
>> Of course, it is another topic in the case when the view is originally
>> defined with
>> leaky functions.
>
> Right.  I was thinking of a pair of quals, one in each of two view definitions.
> The views are mutually-untrusting.  Something like this:
>
> CREATE VIEW a AS SELECT * FROM ta WHERE ac = 5;
> ALTER VIEW a OWNER TO alice;
> CREATE VIEW b AS SELECT * FROM tb WHERE bc = 6;
> ALTER VIEW b OWNER TO bob;
> SELECT * FROM a, b;
>
> Both the ac=5 and the bc=6 quals do run at the same depth despite enforcing
> security for different principals.  I can't think of a way that one view owner
> could use this situation to subvert the security of the other view owner, but I
> wanted to throw it out.
>
Even if view owner set a trap in his view, we have no way to reference variables
come from outside of the view. In above example, even if I added f_leak() into
the definition of VIEW A, we cannot give argument to reference VIEW B.

> I was referring to this paragraph:
>
>  On the technical side, I am pretty doubtful that the approach of adding a
>  nestlevel to FuncExpr and RelOptInfo is the right way to go.  I believe we
>  have existing code (to handle left joins) that prevents quals from being
>  pushed down too far by fudging the set of relations that are supposedly needed
>  to evaluate the qual.  I suspect a similar approach would work here.
>
It seems to me the later half of this paragraph is talking about the problem of
unexpected qualifier pushing-down over the security barrier; I'm trying to solve
the problem with the part.2 patch.
The scenario the part.1 patch tries to solve is order to launch qualifiers, not
unexpected pushing-down.

As long as we focus on the ordering problem, it is reasonable approach to
track original nestlevel to enforce to launch qualifier come from deeper level
earlier. Because it makes performance damages, if we prevent pull-up
subqueries come from security view.

For example, if we cannot pull-up this subquery, we will need to scan the
relation twice.
SELECT * FROM (SELECT * FROM tbl WHERE f_policy(x)) WHERE f_leak(y);

Normally, it should be pulled-up into the following form:
SELECT * FROM tbl WHERE f_policy(x) AND f_leak(y);

>> In addition, implementation will become complex, if both of qualifiers pulled-up
>> from security barrier view and qualifiers pulled-up from regular views are mixed
>> within a single qualifier list.
>
> I only scanned the part 2 patch, but isn't the bookkeeping already happening for
> its purposes?  How much more complexity would we get to apply the same strategy
> to the behavior of this patch?
>
If conditional, what criteria we should have to reorder the quelifier?
The current patch checks the depth at first, then it checks cost if same deptn.
It is quite simple rule. I have no idea of the criteria to order the
mixed qualifier
come from security-barrier views and regular views.

>> > On Mon, Jun 06, 2011 at 01:37:11PM +0100, Kohei Kaigai wrote:
>> >> --- a/src/backend/optimizer/plan/planner.c
>> >> +++ b/src/backend/optimizer/plan/planner.c
>> >
>> >> + ? ? else if (IsA(node, Query))
>> >> + ? ? {
>> >> + ? ? ? ? ? ? depth += 2;
>> >
>> > Why two?
>> >
>> This patch is a groundwork for the upcoming row-level security feature.
>> In the case when the backend appends security policy functions, it should
>> be launched prior to user given qualifiers. So, I intends to give odd depth
>> numbers for these functions to have higher priorities to other one.
>> Of course, 1 might work well right now.
>
> I'd say it should either be 1 until such time as that's needed, or it needs a
> comment noting why it's 2.
>
OK, I'll add comment to introduce why the depth is incremented by 2.

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