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

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.

> > This makes assumptions, at a distance, about the possible scan types and how
> > quals can be fitted to them. ?Specifically, this patch achieves its goals
> > because any indexable qual is trustworthy, and any non-indexable qual cannot
> > be pushed down further than the view's own quals. ?That seems to be true with
> > current plan types, but it feels fragile. ?I don't have a concrete idea for
> > improvement, though. ?Robert suggested another approach in
> > http://archives.postgresql.org/message-id/AANLkTimbN_6tYxReh5Rc7pmizT-VJB3xgp8CuHO0OAHC@mail.gmail.com
> > ; might that approach avoid this hazard?
> >
> The reason why we didn't adopt the idea to check privileges of underlying tables
> is that PostgreSQL checks privileges on executor phase, not planner phase.
>
> If we try to have a flag on pg_proc, it is a tough work to categolize trustworth
> functions and non-trustworh ones from beginning, because we have more than
> 2000 of built-in functions.
> So, it is reasonable assumption that index access methods does not leak
> contents of tuples scanned, because only superuser can define them.

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.

> > The part 2 patch in this series implements the planner restriction more likely
> > to yield performance regressions, so it introduces a mechanism for identifying
> > when to apply the restriction. ?This patch, however, applies its restriction
> > unconditionally. ?Since we will inevitably have a such mechanism before you
> > are done sealing the leaks in our view implementation, the restriction in this
> > patch should also use that mechanism. ?Therefore, I think we should review and
> > apply part 2 first, then update this patch to use its conditional behavior.
> >
> The reason why this patch always gives the depth higher priority is the matter
> is relatively minor compared to the issue the part.2 patch tries to tackle.
> That affects the selection of scan plan (IndexScan or SeqScan), so it
> is significant
> decision to be controllable. However, this issue is just on a particular scan.

True. The lost optimization opportunity is relatively minor, but perhaps not
absolutely minor. It would be one thing if we could otherwise get away without
designing any mechanism for applying these restrictions conditionally. However,
since you have implemented the conditional behavior elsewhere, it would be nice
to apply it here.

> 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?

> > 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.

Thanks,
nm