Improve qual pushdown for RLS and SB views

The original security barrier view implementation, on which RLS is
built, prevented all non-leakproof functions from being pushed down to
below the view, even when the function was not receiving any data from
the view.  This optimization improves on that situation by, instead of
checking strictly for non-leakproof functions, it checks for Vars being
passed to non-leakproof functions and allows functions which do not
accept arguments or whose arguments are not from the current query level
(eg: constants can be particularly useful) to be pushed down.

As discussed, this does mean that a function which is pushed down might
gain some idea that there are rows meeting a certain criteria based on
the number of times the function is called, but this isn't a
particularly new issue and the documentation in rules.sgml already
addressed similar covert-channel risks.  That documentation is updated
to reflect that non-leakproof functions may be pushed down now, if
they meet the above-described criteria.

Author: Dean Rasheed, with a bit of rework to make things clearer,
along with comment and documentation updates from me.

Author: sfrost

doc/src/sgml/rules.sgml

@@ -2136,7 +2136,7 @@ SELECT * FROM phone_number WHERE tricky(person, phone);
     When it is necessary for a view to provide row level security, the
     <literal>security_barrier</literal> attribute should be applied to
     the view.  This prevents maliciously-chosen functions and operators from
-    being invoked on rows until after the view has done its work.  For
+    being passed values from rows until after the view has done its work.  For
     example, if the view shown above had been created like this, it would
     be secure:
 <programlisting>
@@ -2157,9 +2157,12 @@ CREATE VIEW phone_number WITH (security_barrier) AS
     operators.  The query planner can safely allow such functions to be evaluated
     at any point in the query execution process, since invoking them on rows
     invisible to the user will not leak any information about the unseen rows.
-    In contrast, a function that might throw an error depending on the values
-    received as arguments (such as one that throws an error in the event of
-    overflow or division by zero) are not leak-proof, and could provide
+    Further, functions which do not take arguments or which are not passed any
+    arguments from the security barrier view do not have to be marked as
+    <literal>LEAKPROOF</literal> to be pushed down, as they never receive data
+    from the view.  In contrast, a function that might throw an error depending
+    on the values received as arguments (such as one that throws an error in the
+    event of overflow or division by zero) are not leak-proof, and could provide
     significant information about the unseen rows if applied before the security
     view's row filters.
 </para>

src/backend/optimizer/path/allpaths.c

@@ -1982,7 +1982,9 @@ targetIsInAllPartitionLists(TargetEntry *tle, Query *query)
  * 2. If unsafeVolatile is set, the qual must not contain any volatile
  * functions.
  *
- * 3. If unsafeLeaky is set, the qual must not contain any leaky functions.
+ * 3. If unsafeLeaky is set, the qual must not contain any leaky functions
+ * that are passed Var nodes, and therefore might reveal values from the
+ * subquery as side effects.
  *
  * 4. The qual must not refer to the whole-row output of the subquery
  * (since there is no easy way to name that within the subquery itself).
@@ -2009,7 +2011,7 @@ qual_is_pushdown_safe(Query *subquery, Index rti, Node *qual,
 
 	/* Refuse leaky quals if told to (point 3) */
 	if (safetyInfo->unsafeLeaky &&
-		contain_leaky_functions(qual))
+		contain_leaked_vars(qual))
 		return false;
 
 	/*

src/backend/optimizer/util/clauses.c

@@ -97,7 +97,7 @@ static bool contain_mutable_functions_walker(Node *node, void *context);
 static bool contain_volatile_functions_walker(Node *node, void *context);
 static bool contain_volatile_functions_not_nextval_walker(Node *node, void *context);
 static bool contain_nonstrict_functions_walker(Node *node, void *context);
-static bool contain_leaky_functions_walker(Node *node, void *context);
+static bool contain_leaked_vars_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);
@@ -1318,26 +1318,30 @@ contain_nonstrict_functions_walker(Node *node, void *context)
 }
 
 /*****************************************************************************
- *		  Check clauses for non-leakproof functions
+ *		  Check clauses for Vars passed to non-leakproof functions
  *****************************************************************************/
 
 /*
- * contain_leaky_functions
- *		Recursively search for leaky functions within a clause.
+ * contain_leaked_vars
+ *		Recursively scan a clause to discover whether it contains any Var
+ *		nodes (of the current query level) that are passed as arguments to
+ *		leaky functions.
  *
- * Returns true if any function call with side-effect may be present in the
- * clause.  Qualifiers from outside the a security_barrier view should not
- * be pushed down into the view, lest the contents of tuples intended to be
- * filtered out be revealed via side effects.
+ * Returns true if the clause contains any non-leakproof functions that are
+ * passed Var nodes of the current query level, and which might therefore leak
+ * data.  Qualifiers from outside a security_barrier view that might leak data
+ * in this way should not be pushed down into the view in case the contents of
+ * tuples intended to be filtered out by the view are revealed by the leaky
+ * functions.
  */
 bool
-contain_leaky_functions(Node *clause)
+contain_leaked_vars(Node *clause)
 {
-	return contain_leaky_functions_walker(clause, NULL);
+	return contain_leaked_vars_walker(clause, NULL);
 }
 
 static bool
-contain_leaky_functions_walker(Node *node, void *context)
+contain_leaked_vars_walker(Node *node, void *context)
 {
 	if (node == NULL)
 		return false;
@@ -1369,7 +1373,8 @@ contain_leaky_functions_walker(Node *node, void *context)
 			{
 				FuncExpr   *expr = (FuncExpr *) node;
 
-				if (!get_func_leakproof(expr->funcid))
+				if (!get_func_leakproof(expr->funcid) &&
+					contain_var_clause((Node *) expr->args))
 					return true;
 			}
 			break;
@@ -1381,7 +1386,8 @@ contain_leaky_functions_walker(Node *node, void *context)
 				OpExpr	   *expr = (OpExpr *) node;
 
 				set_opfuncid(expr);
-				if (!get_func_leakproof(expr->opfuncid))
+				if (!get_func_leakproof(expr->opfuncid) &&
+					contain_var_clause((Node *) expr->args))
 					return true;
 			}
 			break;
@@ -1391,7 +1397,8 @@ contain_leaky_functions_walker(Node *node, void *context)
 				ScalarArrayOpExpr *expr = (ScalarArrayOpExpr *) node;
 
 				set_sa_opfuncid(expr);
-				if (!get_func_leakproof(expr->opfuncid))
+				if (!get_func_leakproof(expr->opfuncid) &&
+					contain_var_clause((Node *) expr->args))
 					return true;
 			}
 			break;
@@ -1401,15 +1408,29 @@ contain_leaky_functions_walker(Node *node, void *context)
 				CoerceViaIO *expr = (CoerceViaIO *) node;
 				Oid			funcid;
 				Oid			ioparam;
+				bool		leakproof;
 				bool		varlena;
 
+				/*
+				 * Data may be leaked if either the input or the output
+				 * function is leaky.
+				 */
 				getTypeInputInfo(exprType((Node *) expr->arg),
 								 &funcid, &ioparam);
-				if (!get_func_leakproof(funcid))
-					return true;
+				leakproof = get_func_leakproof(funcid);
+
+				/*
+				 * If the input function is leakproof, then check the output
+				 * function.
+				 */
+				if (leakproof)
+				{
+					getTypeOutputInfo(expr->resulttype, &funcid, &varlena);
+					leakproof = get_func_leakproof(funcid);
+				}
 
-				getTypeOutputInfo(expr->resulttype, &funcid, &varlena);
-				if (!get_func_leakproof(funcid))
+				if (!leakproof &&
+					contain_var_clause((Node *) expr->arg))
 					return true;
 			}
 			break;
@@ -1419,14 +1440,29 @@ contain_leaky_functions_walker(Node *node, void *context)
 				ArrayCoerceExpr *expr = (ArrayCoerceExpr *) node;
 				Oid			funcid;
 				Oid			ioparam;
+				bool		leakproof;
 				bool		varlena;
 
+				/*
+				 * Data may be leaked if either the input or the output
+				 * function is leaky.
+				 */
 				getTypeInputInfo(exprType((Node *) expr->arg),
 								 &funcid, &ioparam);
-				if (!get_func_leakproof(funcid))
-					return true;
-				getTypeOutputInfo(expr->resulttype, &funcid, &varlena);
-				if (!get_func_leakproof(funcid))
+				leakproof = get_func_leakproof(funcid);
+
+				/*
+				 * If the input function is leakproof, then check the output
+				 * function.
+				 */
+				if (leakproof)
+				{
+					getTypeOutputInfo(expr->resulttype, &funcid, &varlena);
+					leakproof = get_func_leakproof(funcid);
+				}
+
+				if (!leakproof &&
+					contain_var_clause((Node *) expr->arg))
 					return true;
 			}
 			break;
@@ -1435,12 +1471,22 @@ contain_leaky_functions_walker(Node *node, void *context)
 			{
 				RowCompareExpr *rcexpr = (RowCompareExpr *) node;
 				ListCell   *opid;
+				ListCell   *larg;
+				ListCell   *rarg;
 
-				foreach(opid, rcexpr->opnos)
+				/*
+				 * Check the comparison function and arguments passed to it for
+				 * each pair of row elements.
+				 */
+				forthree(opid, rcexpr->opnos,
+						 larg, rcexpr->largs,
+						 rarg, rcexpr->rargs)
 				{
 					Oid			funcid = get_opcode(lfirst_oid(opid));
 
-					if (!get_func_leakproof(funcid))
+					if (!get_func_leakproof(funcid) &&
+						(contain_var_clause((Node *) lfirst(larg)) ||
+						 contain_var_clause((Node *) lfirst(rarg))))
 						return true;
 				}
 			}
@@ -1455,7 +1501,7 @@ contain_leaky_functions_walker(Node *node, void *context)
 			 */
 			return true;
 	}
-	return expression_tree_walker(node, contain_leaky_functions_walker,
+	return expression_tree_walker(node, contain_leaked_vars_walker,
 								  context);
 }
 

src/include/optimizer/clauses.h

@@ -63,7 +63,7 @@ extern bool contain_mutable_functions(Node *clause);
 extern bool contain_volatile_functions(Node *clause);
 extern bool contain_volatile_functions_not_nextval(Node *clause);
 extern bool contain_nonstrict_functions(Node *clause);
-extern bool contain_leaky_functions(Node *clause);
+extern bool contain_leaked_vars(Node *clause);
 
 extern Relids find_nonnullable_rels(Node *clause);
 extern List *find_nonnullable_vars(Node *clause);

src/test/modules/test_rls_hooks/expected/test_rls_hooks.out

@@ -85,13 +85,11 @@ SET ROLE s1;
 -- restrictive hook's policy is current_user = superuser
 -- combined with AND, results in nothing being allowed
 EXPLAIN (costs off) SELECT * FROM rls_test_both;
-                      QUERY PLAN                       
--------------------------------------------------------
- Subquery Scan on rls_test_both
-   Filter: ("current_user"() = rls_test_both.username)
-   ->  Seq Scan on rls_test_both rls_test_both_1
-         Filter: ("current_user"() = supervisor)
-(4 rows)
+                                  QUERY PLAN                                   
+-------------------------------------------------------------------------------
+ Seq Scan on rls_test_both
+   Filter: ((supervisor = "current_user"()) AND (username = "current_user"()))
+(2 rows)
 
 SELECT * FROM rls_test_both;
  username | supervisor | data 

src/test/regress/expected/rowsecurity.out

@@ -1913,6 +1913,112 @@ EXPLAIN (COSTS OFF) SELECT * FROM y2 WHERE f_leak(b);
          Filter: (((a % 4) = 0) OR ((a % 3) = 0) OR ((a % 2) = 0))
 (4 rows)
 
+--
+-- Qual push-down of leaky functions, when not referring to table
+--
+SELECT * FROM y2 WHERE f_leak('abc');
+NOTICE:  f_leak => abc
+NOTICE:  f_leak => abc
+NOTICE:  f_leak => abc
+NOTICE:  f_leak => abc
+NOTICE:  f_leak => abc
+NOTICE:  f_leak => abc
+NOTICE:  f_leak => abc
+NOTICE:  f_leak => abc
+NOTICE:  f_leak => abc
+NOTICE:  f_leak => abc
+NOTICE:  f_leak => abc
+NOTICE:  f_leak => abc
+NOTICE:  f_leak => abc
+NOTICE:  f_leak => abc
+NOTICE:  f_leak => abc
+NOTICE:  f_leak => abc
+NOTICE:  f_leak => abc
+NOTICE:  f_leak => abc
+NOTICE:  f_leak => abc
+NOTICE:  f_leak => abc
+NOTICE:  f_leak => abc
+ a  |                b                 
+----+----------------------------------
+  0 | cfcd208495d565ef66e7dff9f98764da
+  2 | c81e728d9d4c2f636f067f89cc14862c
+  3 | eccbc87e4b5ce2fe28308fd9f2a7baf3
+  4 | a87ff679a2f3e71d9181a67b7542122c
+  6 | 1679091c5a880faf6fb5e6087eb1b2dc
+  8 | c9f0f895fb98ab9159f51fd0297e236d
+  9 | 45c48cce2e2d7fbdea1afc51c7c6ad26
+ 10 | d3d9446802a44259755d38e6d163e820
+ 12 | c20ad4d76fe97759aa27a0c99bff6710
+ 14 | aab3238922bcc25a6f606eb525ffdc56
+ 15 | 9bf31c7ff062936a96d3c8bd1f8f2ff3
+ 16 | c74d97b01eae257e44aa9d5bade97baf
+ 18 | 6f4922f45568161a8cdf4ad2299f6d23
+ 20 | 98f13708210194c475687be6106a3b84
+(14 rows)
+
+EXPLAIN (COSTS OFF) SELECT * FROM y2 WHERE f_leak('abc');
+                                      QUERY PLAN                                       
+---------------------------------------------------------------------------------------
+ Seq Scan on y2
+   Filter: (f_leak('abc'::text) AND (((a % 4) = 0) OR ((a % 3) = 0) OR ((a % 2) = 0)))
+(2 rows)
+
+CREATE TABLE test_qual_pushdown (
+	abc		text
+);
+INSERT INTO test_qual_pushdown VALUES ('abc'),('def');
+SELECT * FROM y2 JOIN test_qual_pushdown ON (b = abc) WHERE f_leak(abc);
+NOTICE:  f_leak => abc
+NOTICE:  f_leak => def
+ a | b | abc 
+---+---+-----
+(0 rows)
+
+EXPLAIN (COSTS OFF) SELECT * FROM y2 JOIN test_qual_pushdown ON (b = abc) WHERE f_leak(abc);
+                               QUERY PLAN                                
+-------------------------------------------------------------------------
+ Hash Join
+   Hash Cond: (test_qual_pushdown.abc = y2.b)
+   ->  Seq Scan on test_qual_pushdown
+         Filter: f_leak(abc)
+   ->  Hash
+         ->  Seq Scan on y2
+               Filter: (((a % 4) = 0) OR ((a % 3) = 0) OR ((a % 2) = 0))
+(7 rows)
+
+SELECT * FROM y2 JOIN test_qual_pushdown ON (b = abc) WHERE f_leak(b);
+NOTICE:  f_leak => cfcd208495d565ef66e7dff9f98764da
+NOTICE:  f_leak => c81e728d9d4c2f636f067f89cc14862c
+NOTICE:  f_leak => eccbc87e4b5ce2fe28308fd9f2a7baf3
+NOTICE:  f_leak => a87ff679a2f3e71d9181a67b7542122c
+NOTICE:  f_leak => 1679091c5a880faf6fb5e6087eb1b2dc
+NOTICE:  f_leak => c9f0f895fb98ab9159f51fd0297e236d
+NOTICE:  f_leak => 45c48cce2e2d7fbdea1afc51c7c6ad26
+NOTICE:  f_leak => d3d9446802a44259755d38e6d163e820
+NOTICE:  f_leak => c20ad4d76fe97759aa27a0c99bff6710
+NOTICE:  f_leak => aab3238922bcc25a6f606eb525ffdc56
+NOTICE:  f_leak => 9bf31c7ff062936a96d3c8bd1f8f2ff3
+NOTICE:  f_leak => c74d97b01eae257e44aa9d5bade97baf
+NOTICE:  f_leak => 6f4922f45568161a8cdf4ad2299f6d23
+NOTICE:  f_leak => 98f13708210194c475687be6106a3b84
+ a | b | abc 
+---+---+-----
+(0 rows)
+
+EXPLAIN (COSTS OFF) SELECT * FROM y2 JOIN test_qual_pushdown ON (b = abc) WHERE f_leak(b);
+                                  QUERY PLAN                                   
+-------------------------------------------------------------------------------
+ Hash Join
+   Hash Cond: (test_qual_pushdown.abc = y2.b)
+   ->  Seq Scan on test_qual_pushdown
+   ->  Hash
+         ->  Subquery Scan on y2
+               Filter: f_leak(y2.b)
+               ->  Seq Scan on y2 y2_1
+                     Filter: (((a % 4) = 0) OR ((a % 3) = 0) OR ((a % 2) = 0))
+(8 rows)
+
+DROP TABLE test_qual_pushdown;
 --
 -- Plancache invalidate on user change.
 --