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

Thanks for your reviewing,

2011/6/28 Noah Misch <noah(at)leadboat(dot)com>:
> I took a look at this patch.  It's incredibly simple, which is great, and it
> seems to achieve its goal.
>
> 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.

> I like how you've assumed that the table owner trusts the operator class
> functions of indexes on his table to not leak information.  That handily
> catches some basic and important qual pushdowns that would otherwise be lost.
>
> 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.

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

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.

> A few minor questions/comments on the implementation:
>
> 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.

>> --- a/src/test/regress/expected/select_views.out
>> +++ b/src/test/regress/expected/select_views.out
>
>> +EXPLAIN SELECT * FROM your_credit WHERE f_leak(number,expired);
>> +                                QUERY PLAN
>> +--------------------------------------------------------------------------
>> + Seq Scan on credit_cards  (cost=0.00..181.20 rows=1 width=96)
>
> Use "EXPLAIN (COSTS OFF)" in regression tests.  We do not put much effort into
> the stability of exact cost values, and they do not matter for the purpose of
> this test.
>
OK, fixed.

>> --- a/src/test/regress/sql/select_views.sql
>> +++ b/src/test/regress/sql/select_views.sql
>> @@ -8,3 +8,46 @@ SELECT * FROM street;
>>  SELECT name, #thepath FROM iexit ORDER BY 1, 2;
>>
>>  SELECT * FROM toyemp WHERE name = 'sharon';
>> +
>> +--
>> +-- Test for leaky-view problem
>> +--
>> +
>> +-- setups
>> +SET client_min_messages TO 'warning';
>> +
>> +DROP ROLE IF EXISTS alice;
>> +DROP FUNCTION IF EXISTS f_leak(text);
>> +DROP TABLE IF EXISTS credit_cards;
>> +
>> +RESET client_min_messages;
>
> No need for this.  The regression tests always start on a clean database.
>
Fixed.

>> +
>> +CREATE USER alice;
>> +CREATE FUNCTION f_leak(text, text)
>> +    RETURNS bool LANGUAGE 'plpgsql'
>> +    AS 'begin raise notice ''% => %'', $1, $2; return true; end';
>
> I ran this test case on master, and it did not reproduce the problem.
> However, adding "COST 0.1" to this CREATE FUNCTION did yield the expected
> problem behavior.  I suggest adding that.
>
Thanks, I might miss the point of regression test.

The attached patch is revised one on regression test.

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