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

Thanks for your detailed viewing and testing.

The attached patches are revised version.

Part-0)
* The patch was re-generated using context diff, instead of unified diff
* Documentation on ALTER VIEW was added
* Behavior of CREATE OR REPLACE VIEW was revised; when we replace
an existing view, reloption shall be reset, even if a particular
value was set.
* And, cosmetic changes; eliminate warnings due to lack of type cast.

Part-1)
* I removed the code to increment depth by 2, and preserve the latest bit,
because we have no module to utilize this mechanism right now.

Thanks,

2011/7/5 Noah Misch <noah(at)2ndquadrant(dot)com>:
> On Sun, Jul 03, 2011 at 11:33:38AM +0200, Kohei KaiGai wrote:
>> The attached patches are revised version.
>>
>> The part-0 provides 'security_barrier' option for view definition, and performs
>> as a common basis of part-1 and part-2 patches.
>> Syntax is extended as follows:
>>
>>   CREATE VIEW view_name [WITH (param [=value])] AS query;
>>
>> We can also turn on/off this security_barrier setting by ALTER TABLE with
>> SET/RESET options.
>>
>> The part-1 patch enforces the qualifiers originally located under the security
>> barrier view to be launched prior to ones supplied on upper level.
>> The differences from the previous version is this barrier become conditional,
>> not always. So, existing optimization will be applied without any changes
>> onto non-security-barrier views.
>
> I tested various query trees I considered interesting, and this version had
> sound semantics for all of them.  I have one suggestion for CREATE OR REPLACE
> VIEW semantics, plus various cosmetic comments.
>
> These patches are unified diffs, rather than project-standard context diffs.
>
> Part 0:
>> --- a/doc/src/sgml/ref/create_view.sgml
>> +++ b/doc/src/sgml/ref/create_view.sgml
>> @@ -22,6 +22,7 @@ PostgreSQL documentation
>>   <refsynopsisdiv>
>>  <synopsis>
>>  CREATE [ OR REPLACE ] [ TEMP | TEMPORARY ] VIEW <replaceable class="PARAMETER">name</replaceable> [ ( <replaceable class="PARAMETER">column_name</replaceable> [, ...] ) ]
>> +    [ WITH ( parameter [= value] [, ... ] ) ]
>
> This needs a bit more markup; see the corresponding case in create_table.sgml.
>
> alter_view.sgml also needs an update.  Incidentally, we should use ALTER VIEW
> SET OPTION when referring to setting this for a view.  ALTER TABLE SET OPTION
> will also support views, since that's the general pattern for tablecmds.c type
> checks, but that's largely an implementation detail.
>
>> --- a/src/backend/commands/view.c
>> +++ b/src/backend/commands/view.c
>> @@ -97,7 +97,8 @@ isViewOnTempTable_walker(Node *node, void *context)
>>   *---------------------------------------------------------------------
>>   */
>>  static Oid
>> -DefineVirtualRelation(const RangeVar *relation, List *tlist, bool replace)
>> +DefineVirtualRelation(const RangeVar *relation, List *tlist,
>> +                                       bool replace, List *options)
>>  {
>>       Oid                     viewOid,
>>                               namespaceId;
>
> This hunk and the hunk for the function's caller get rejects due to another
> recent signature change.
>
>> @@ -167,6 +168,8 @@ DefineVirtualRelation(const RangeVar *relation, List *tlist, bool replace)
>>       {
>>               Relation        rel;
>>               TupleDesc       descriptor;
>> +             List       *atcmds = NIL;
>> +             AlterTableCmd *atcmd;
>>
>>               /*
>>                * Yes.  Get exclusive lock on the existing view ...
>> @@ -211,14 +214,11 @@ DefineVirtualRelation(const RangeVar *relation, List *tlist, bool replace)
>>                */
>>               if (list_length(attrList) > rel->rd_att->natts)
>>               {
>> -                     List       *atcmds = NIL;
>>                       ListCell   *c;
>>                       int                     skip = rel->rd_att->natts;
>>
>>                       foreach(c, attrList)
>>                       {
>> -                             AlterTableCmd *atcmd;
>> -
>>                               if (skip > 0)
>>                               {
>>                                       skip--;
>> @@ -229,10 +229,24 @@ DefineVirtualRelation(const RangeVar *relation, List *tlist, bool replace)
>>                               atcmd->def = (Node *) lfirst(c);
>>                               atcmds = lappend(atcmds, atcmd);
>>                       }
>> -                     AlterTableInternal(viewOid, atcmds, true);
>>               }
>>
>>               /*
>> +              * If optional parameters are specified, we must set options
>> +              * using ALTER TABLE SET OPTION internally.
>
> I think CREATE OR REPLACE VIEW should replace the option list, while ALTER
> VIEW SET OPTION should retain its current behavior.  That is, this should
> leave the view with no options set:
>
> create or replace view v0(n) with (security_barrier) as values (1), (2), (3), (4);
> select reloptions from pg_class where oid = 'v0'::regclass;
> create or replace view v0(n) as values (4), (3), (2), (1);
> select reloptions from pg_class where oid = 'v0'::regclass;
>
>> +              */
>> +             if (list_length(options) > 0)
>> +             {
>> +                     atcmd = makeNode(AlterTableCmd);
>> +                     atcmd->subtype = AT_SetRelOptions;
>> +                     atcmd->def = options;
>
> This line produces a warning:
>
> view.c: In function `DefineVirtualRelation':
> view.c:240: warning: assignment from incompatible pointer type
>
>> +
>> +                     atcmds = lappend(atcmds, atcmd);
>> +             }
>> +             if (atcmds != NIL)
>> +                     AlterTableInternal(viewOid, atcmds, true);
>> +
>> +             /*
>>                * Seems okay, so return the OID of the pre-existing view.
>>                */
>>               relation_close(rel, NoLock);    /* keep the lock! */
>
> Part 1:
>> --- a/src/backend/optimizer/plan/planner.c
>> +++ b/src/backend/optimizer/plan/planner.c
>
>> @@ -2993,6 +3001,131 @@ get_column_info_for_window(PlannerInfo *root, WindowClause *wc, List *tlist,
>>       }
>>  }
>>
>> +/*
>> + * mark_qualifiers_depth
>> + *
>> + * It marks depth field of the each expression nodes that eventually
>> + * invokes functions, to track the original nest-level. On the evaluation
>> + * of qualifiers within WHERE or JOIN ... ON clauses during relation scans,
>> + * these items shall be reordered according to the nest-level and estimated
>> + * cost.
>> + * The optimizer may pull-up simple sub-queries or join clause, and
>> + * qualifiers to filter out tuples shall be mixed with ones in upper-
>> + * level. Thus, we need to track the original nest-level of qualifiers
>> + * to prevent reverse of order in evaluation, because some of qualifiers
>> + * can have side-effects that allows to leak supplied argument to outside.
>> + * It can be abused to break row-level security using a user defined function
>> + * with very small estimated cost, so nest level of qualifiers originated
>> + * from is used as a criteria, rather than estimated cost, to decide order
>> + * to evaluate qualifiers.
>> + */
>> +static bool
>> +mark_qualifiers_depth_walker(Node *node, void *context)
>> +{
>> +     int             depth = *((int *)(context));
>> +
>> +     if (node == NULL)
>> +             return false;
>> +     if (IsA(node, FuncExpr))
>> +     {
>> +             FuncExpr   *exp = (FuncExpr *)node;
>> +
>> +             exp->depth = depth | (exp->depth & 1);
>
> Why did these change from plain "exp->depth = depth;" of the last version?
> Since no core code sets a 1-bit in a depth value, I assume it must be related
> to your future-use design for that bit.  If so: could an external module
> realistically take advantage of this?  If yes, then a mere comment is in
> order.  If not, I think we should remove this (and the incrementing by 2) and
> add it again in the future patch that makes use thereof.
>
>> --- a/src/test/regress/sql/select_views.sql
>> +++ b/src/test/regress/sql/select_views.sql
>> @@ -8,3 +8,42 @@ SELECT * FROM street;
>>  SELECT name, #thepath FROM iexit ORDER BY 1, 2;
>>
>>  SELECT * FROM toyemp WHERE name = 'sharon';
>> +
>> +--
>> +-- Test for leaky-view
>> +--
>> +
>> +CREATE USER alice;
>> +CREATE FUNCTION f_leak(text, text)
>> +        RETURNS bool LANGUAGE 'plpgsql'
>> +        COST 0.00000001
>> +        AS 'begin raise notice ''% => %'', $1, $2; return true; end';
>> +CREATE TABLE credit_cards (
>> +    name   text,
>> +    number text,
>> +    expired    text
>> +);
>> +INSERT INTO credit_cards VALUES ('alice', '1111-2222-3333-4444', 'Aug-2012'),
>> +                                ('bob',   '5555-6666-7777-8888', 'Nov-2016'),
>> +                                ('eve',   '9801-2345-6789-0123', 'Jan-2018');
>> +CREATE VIEW your_credit_normal AS
>> +    SELECT * FROM credit_cards WHERE name = getpgusername();
>> +CREATE VIEW your_credit_secure WITH (security_barrier) AS
>> +    SELECT * FROM credit_cards WHERE name = getpgusername();
>> +
>> +GRANT SELECT ON your_credit_normal TO public;
>> +GRANT SELECT ON your_credit_secure TO public;
>> +-- run leaky view
>> +SET SESSION AUTHORIZATION alice;
>> +
>> +SELECT * FROM your_credit_normal WHERE f_leak(number,expired);
>> +EXPLAIN (COSTS OFF) SELECT * FROM your_credit_normal WHERE f_leak(number,expired);
>> +
>> +SELECT * FROM your_credit_secure WHERE f_leak(number,expired);
>> +EXPLAIN (COSTS OFF) SELECT * FROM your_credit_secure WHERE f_leak(number,expired);
>> +
>> +\c -
>> +-- cleanups
>> +DROP ROLE IF EXISTS alice;
>> +DROP FUNCTION IF EXISTS f_leak(text);
>> +DROP TABLE IF EXISTS credit_cards CASCADE;
>
> Keep the view around.  That way, pg_dump tests of the regression database will
> test the dumping of this view option.  (Your pg_dump support for this feature
> does work fine, though.)
>
> Thanks,
> nm
>

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