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

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