Thread
-
Re: [v9.2] Fix leaky-view problem, part 1
Kohei KaiGai <kaigai@kaigai.gr.jp> — 2011-07-06T20:25:12Z
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@2ndquadrant.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@kaigai.gr.jp>