The attached patch is a proof of concept.
It tries to fix the problem of leaky VIEWs for RLS.
* The scenario of leaky VIEWs for RLS
-------------------------------------
When a view contains any table joins and user gives a function that
takes arguments depending on only one-side table of the joins, the
planner tries to distribute the qualifier into least unit of scans.
It enables to minimize the scale of execution cost, but it means
user given function may be invoked earlier than other qualifiers
within a view, although row-level security is intended using views.
If the user given function has a side-effect (E.g, it may try to
insert given arguments into other tables), this behavior allows us
to leak contents of invisible tuples.
postgres=# SELECT * FROM v2 WHERE f_malicious(y);
NOTICE: f_malicious: xxx
NOTICE: f_malicious: yyy <-- leaky contents
NOTICE: f_malicious: zzz
a | b | x | y
---+-----+---+-----
1 | aaa | 1 | xxx
3 | ccc | 3 | zzz
(2 rows)
* Discussions in -hackers
-------------------------
Although it is a security problem, we will face unignorable performance
regression, if we disallow to distribute all user defined functions to
inside of joins come from views originally.
One idea is to distinguish a view into two types. The one is security
oriented view, and the other is regular view. Because only creator of
the view knows purpose of the view, an idea was suggested that takes
a hint on CREATE VIEW, like CREATE SECURITY VIEW.
(But I don't implement this feature in this patch yet.)
In addition, I was pointed out it may be harmless as long as
a person executes the query (not view's owner) has enough privileges
on underlaying tables, even if a view is marked as security view.
However, it was also pointed out the idea needs to check privileges
on planner stage, not execution stage. Is it really preferable?
It is not concluded yet. But inline_set_returning_function() checks
ACL_EXECUTE permission on planner stage, then it makes a FuncExpr
flatten prior to optimization. At least, it seems to me something
violation to the dogma.
Please point out any issues which I missed.
* About this patch
------------------
This patch mainly consists of two parts.
The first part is at pull_up_simple_subquery().
It checks privileges of underlaying tables in the subquery to be pulled
up, then it marks FromExpr->security_view as TRUE.
Note that it does not distinct a subquery come from a view and a pure
subquery in the original query. But it eventually causes access violation
error, if current user does not have privileges on a pure subquery.
So, no matter.
And, also note that it does not have statement support right now.
So, it assumes all the subqueries are possibly security views.
The second part is at distribute_qual_to_rels().
It computes what relations does the given clause depend on at first.
If the given clause depends on a part of relations to be joined
inside of the security view, it expands the dependency of the clause
into whole of the security view.
In the result, the clause is not distributed into inside of the join
loop.
(*) This patch uses check_relation_privileges() to check permissions
on DML execution, so please apply another my patch also on testing.
* Execution result
------------------
postgres=# CREATE TABLE t1 (a int primary key, b text);
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "t1_pkey" for
table "t1"
CREATE TABLE
postgres=# CREATE TABLE t2 (x int primary key, y text);
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "t2_pkey" for
table "t2"
CREATE TABLE
postgres=# CREATE TABLE t3 (s int primary key, t text);
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "t3_pkey" for
table "t3"
CREATE TABLE
postgres=# CREATE VIEW v1 AS SELECT * FROM t1 JOIN t2 ON t1.a = t2.x;
CREATE VIEW
postgres=# CREATE VIEW v2 AS SELECT * FROM v1 JOIN t3 ON v1.a = t3.s;
CREATE VIEW
postgres=# CREATE USER tak;
CREATE ROLE
postgres=# GRANT SELECT ON v1, v2, t3 TO tak;
GRANT
The user: tak is allowed to select v1, v2 and t3.
postgres=# CREATE OR REPLACE FUNCTION f_malicious(text) RETURNS bool
AS 'BEGIN RAISE NOTICE ''f_malicious: %'', $1; RETURN true; END;'
LANGUAGE plpgsql;
CREATE FUNCTION
Because superuser can access t1 and t2 directly, f_malicious(y) shall be
distributed to Seq-scan on t2.
postgres=# EXPLAIN SELECT * FROM v1 WHERE f_malicious(y);
QUERY PLAN
-------------------------------------------------------------------
Hash Join (cost=334.93..365.94 rows=410 width=72)
Hash Cond: (t1.a = t2.x)
-> Seq Scan on t1 (cost=0.00..22.30 rows=1230 width=36)
-> Hash (cost=329.80..329.80 rows=410 width=36)
-> Seq Scan on t2 (cost=0.00..329.80 rows=410 width=36)
Filter: f_malicious(y)
(6 rows)
But tak cannot access t1 and t2 directly, f_malicious(y) shall be
distributed to outside of the join.
postgres=> EXPLAIN SELECT * FROM v1 WHERE f_malicious(y);
QUERY PLAN
-------------------------------------------------------------------
Hash Join (cost=37.67..384.39 rows=410 width=72)
Hash Cond: (t1.a = t2.x)
Join Filter: f_malicious(t2.y)
-> Seq Scan on t1 (cost=0.00..22.30 rows=1230 width=36)
-> Hash (cost=22.30..22.30 rows=1230 width=36)
-> Seq Scan on t2 (cost=0.00..22.30 rows=1230 width=36)
(6 rows)
In this path, it assumes operators are not malicious (is it a correct
assumption?), this restriction does not prevent indexing of scans.
postgres=> EXPLAIN SELECT * FROM v1 WHERE f_malicious(y) AND x = 100;
QUERY PLAN
-------------------------------------------------------------------------
Nested Loop (cost=0.00..16.80 rows=1 width=72)
Join Filter: f_malicious(t2.y)
-> Index Scan using t1_pkey on t1 (cost=0.00..8.27 rows=1 width=36)
Index Cond: (a = 100)
-> Index Scan using t2_pkey on t2 (cost=0.00..8.27 rows=1 width=36)
Index Cond: (t2.x = 100)
(6 rows)
* To do items
- Need to discuss whether we are on the right direction, or not.
- Statement support to provide a hint.
Such as CREATE SECURITY VIEW AS ... ?
- Need to tackle the matter of evaluation order of scan filter.
This patch does not care anything.
Thanks,
--
KaiGai Kohei <kai...@ak.jp.nec.com>
*** a/src/backend/nodes/copyfuncs.c
--- b/src/backend/nodes/copyfuncs.c
***************
*** 1651,1656 **** _copyFromExpr(FromExpr *from)
--- 1651,1657 ----
COPY_NODE_FIELD(fromlist);
COPY_NODE_FIELD(quals);
+ COPY_SCALAR_FIELD(security_view);
return newnode;
}
*** a/src/backend/nodes/equalfuncs.c
--- b/src/backend/nodes/equalfuncs.c
***************
*** 730,735 **** _equalFromExpr(FromExpr *a, FromExpr *b)
--- 730,736 ----
{
COMPARE_NODE_FIELD(fromlist);
COMPARE_NODE_FIELD(quals);
+ COMPARE_SCALAR_FIELD(security_view);
return true;
}
*** a/src/backend/nodes/makefuncs.c
--- b/src/backend/nodes/makefuncs.c
***************
*** 148,153 **** makeFromExpr(List *fromlist, Node *quals)
--- 148,154 ----
f->fromlist = fromlist;
f->quals = quals;
+ f->security_view = false;
return f;
}
*** a/src/backend/nodes/outfuncs.c
--- b/src/backend/nodes/outfuncs.c
***************
*** 1329,1334 **** _outFromExpr(StringInfo str, FromExpr *node)
--- 1329,1335 ----
WRITE_NODE_FIELD(fromlist);
WRITE_NODE_FIELD(quals);
+ WRITE_BOOL_FIELD(security_view);
}
/*****************************************************************************
*** a/src/backend/nodes/readfuncs.c
--- b/src/backend/nodes/readfuncs.c
***************
*** 1109,1114 **** _readFromExpr(void)
--- 1109,1115 ----
READ_NODE_FIELD(fromlist);
READ_NODE_FIELD(quals);
+ READ_BOOL_FIELD(security_view);
READ_DONE();
}
*** a/src/backend/optimizer/plan/initsplan.c
--- b/src/backend/optimizer/plan/initsplan.c
***************
*** 718,723 **** make_outerjoininfo(PlannerInfo *root,
--- 718,779 ----
*****************************************************************************/
/*
+ * expand_relids_on_view
+ *
+ * It expands the given relids when it tries to reference a part of relations
+ * within security views. If FromExpr->security_view is true, it means current
+ * user didn't have privileges to access underlaying tables without views.
+ * If user given function takes arguments which references only one-side of
+ * the join loop, optimization may distribute the function node into inside
+ * of the join. It has a possibility to leak contents of invisible tuples to
+ * be filtered using views.
+ */
+ static Relids
+ expand_relids_on_view(Node *jtnode, Relids relids)
+ {
+ if (IsA(jtnode, FromExpr))
+ {
+ FromExpr *f = (FromExpr *) jtnode;
+ ListCell *l;
+
+ foreach (l, f->fromlist)
+ {
+ if (f->security_view)
+ {
+ /*
+ * All the node under the FromExpr with security_view = true
+ * are originally come from a view with underlaying tables
+ * which are not accessable from a person executing the query.
+ * So, if the given clause depends on a part of the relations,
+ * we expand its dependency into whole of the security view.
+ */
+ Relids temp = get_relids_in_jointree((Node *)lfirst(l), false);
+
+ if (bms_overlap(relids, temp))
+ relids = bms_add_members(relids, temp);
+ }
+ else
+ {
+ relids = expand_relids_on_view((Node *)lfirst(l), relids);
+ }
+ }
+ }
+ else if (IsA(jtnode, JoinExpr))
+ {
+ JoinExpr *j = (JoinExpr *) jtnode;
+
+ relids = expand_relids_on_view(j->larg, relids);
+ relids = expand_relids_on_view(j->rarg, relids);
+ }
+ else if (!IsA(jtnode, RangeTblRef))
+ {
+ elog(ERROR, "unrecognized node type: %d", (int) nodeTag(jtnode));
+ }
+
+ return relids;
+ }
+
+ /*
* distribute_qual_to_rels
* Add clause information to either the baserestrictinfo or joininfo list
* (depending on whether the clause is a join) of each base relation
***************
*** 780,785 **** distribute_qual_to_rels(PlannerInfo *root, Node *clause,
--- 836,847 ----
elog(ERROR, "JOIN qualification cannot refer to other relations");
/*
+ * Ensure user given quals not to come into join loop
+ */
+ if (contain_functions_expr(clause))
+ relids = expand_relids_on_view((Node *)root->parse->jointree, relids);
+
+ /*
* If the clause is variable-free, our normal heuristic for pushing it
* down to just the mentioned rels doesn't work, because there are none.
*
*** a/src/backend/optimizer/prep/prepjointree.c
--- b/src/backend/optimizer/prep/prepjointree.c
***************
*** 34,39 ****
--- 34,40 ----
#include "parser/parse_relation.h"
#include "parser/parsetree.h"
#include "rewrite/rewriteManip.h"
+ #include "utils/acl.h"
typedef struct pullup_replace_vars_context
***************
*** 601,606 **** pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte,
--- 602,608 ----
PlannerInfo *subroot;
int rtoffset;
pullup_replace_vars_context rvcontext;
+ List *tempList;
ListCell *lc;
/*
***************
*** 829,834 **** pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte,
--- 831,853 ----
Assert(subroot->placeholder_list == NIL);
/*
+ * In the case when the subquery came from a view, we have to disable
+ * a part of optimization for the security reason.
+ * If user provides a function with side-effects in WHERE clause and
+ * it depends on a part of relations within a view, contents of the
+ * invisible tuples might leak somewhere via user given functions.
+ * When FromExpr->security_view is true, it prevents these functions
+ * are distributed into inside of the subquery.
+ * We assume it is harmless, if current user has enough privileges
+ * on the underlaying tables.
+ */
+ tempList = copyObject(subquery->rtable);
+ foreach (lc, tempList)
+ ((RangeTblEntry *) lfirst(lc))->checkAsUser = InvalidOid;
+ if (check_relation_privileges(tempList, false) != ACLCHECK_OK)
+ subquery->jointree->security_view = true;
+
+ /*
* Miscellaneous housekeeping.
*
* Although replace_rte_variables() faithfully updated parse->hasSubLinks
*** a/src/backend/optimizer/util/clauses.c
--- b/src/backend/optimizer/util/clauses.c
***************
*** 86,91 **** static bool contain_subplans_walker(Node *node, void *context);
--- 86,92 ----
static bool contain_mutable_functions_walker(Node *node, void *context);
static bool contain_volatile_functions_walker(Node *node, void *context);
static bool contain_nonstrict_functions_walker(Node *node, void *context);
+ static bool contain_functions_expr_walker(Node *node, void *context);
static Relids find_nonnullable_rels_walker(Node *node, bool top_level);
static List *find_nonnullable_vars_walker(Node *node, bool top_level);
static bool is_strict_saop(ScalarArrayOpExpr *expr, bool falseOK);
***************
*** 1116,1121 **** contain_nonstrict_functions_walker(Node *node, void *context)
--- 1117,1149 ----
context);
}
+ /*****************************************************************************
+ * Check clauses for function expr
+ *****************************************************************************/
+
+ /*
+ * contain_functions_expr
+ * Recursively search for functions expr within a clause.
+ *
+ * Returns true if FuncExpr is found.
+ */
+ bool
+ contain_functions_expr(Node *clause)
+ {
+ return contain_functions_expr_walker(clause, NULL);
+ }
+
+ static bool
+ contain_functions_expr_walker(Node *node, void *context)
+ {
+ if (node == NULL)
+ return false;
+
+ if (IsA(node, FuncExpr))
+ return true;
+
+ return expression_tree_walker(node, contain_functions_expr_walker, context);
+ }
/*
* find_nonnullable_rels
*** a/src/include/nodes/primnodes.h
--- b/src/include/nodes/primnodes.h
***************
*** 1207,1212 **** typedef struct FromExpr
--- 1207,1213 ----
NodeTag type;
List *fromlist; /* List of join subtrees */
Node *quals; /* qualifiers on join, if any */
+ bool security_view; /* true, if leading FromExpr of security view */
} FromExpr;
#endif /* PRIMNODES_H */
*** a/src/include/optimizer/clauses.h
--- b/src/include/optimizer/clauses.h
***************
*** 67,72 **** extern bool contain_subplans(Node *clause);
--- 67,73 ----
extern bool contain_mutable_functions(Node *clause);
extern bool contain_volatile_functions(Node *clause);
extern bool contain_nonstrict_functions(Node *clause);
+ extern bool contain_functions_expr(Node *clause);
extern Relids find_nonnullable_rels(Node *clause);
extern List *find_nonnullable_vars(Node *clause);
extern List *find_forced_null_vars(Node *clause);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
