Re: Improving RLS qual pushdown

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> Attached is a patch that does that, allowing restriction clause quals
> to be pushed down into subquery RTEs if they contain leaky functions,
> provided that the arglists of those leaky functions contain no Vars
> (such Vars would necessarily refer to output columns of the subquery,
> which is the data that must not be leaked).

> --- 1982,1989 ----
>    * 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
> !  * that might reveal values from the subquery as side effects.

I'd probably extend this comment to note that the way we figure that out
is by looking for Vars being passed to non-leakproof functions.

> --- 1318,1347 ----
>   }
>
>   /*****************************************************************************
> !  *          Check clauses for non-leakproof functions that might leak Vars
>    *****************************************************************************/

Might leak data in Vars (or somethhing along those lines...)

>   /*
> !  * 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 effects may be present in the
> !  * clause and that function's arguments contain any Var nodes of the current
> !  * query level.  Qualifiers from outside a security_barrier view that leak
> !  * Var nodes in this way should not be pushed down into the view, lest the
> !  * contents of tuples intended to be filtered out be revealed via the side
> !  * effects.
>    */

We don't actually know anything about the function and if it actually
has any side effects or not, so it might better to avoid discussing
that here.  How about:

Returns true if any non-leakproof function is passed in data through a
Var node as that function might then leak see sensetive information.
Only leakproof functions are allowed to see data prior to the qualifiers
which are defined in the security_barrier view and therefore we can only
push down qualifiers if they are either leakproof or if they aren't
passed any Vars from this query level (and therefore they are not able
to see any of the data in the tuple, even if they are pushed down).

> --- 1408,1428 ----
>                   CoerceViaIO *expr = (CoerceViaIO *) node;
>                   Oid            funcid;
>                   Oid            ioparam;
> +                 bool        leaky;
>                   bool        varlena;
>
>                   getTypeInputInfo(exprType((Node *) expr->arg),
>                                    &funcid, &ioparam);
> !                 leaky = !get_func_leakproof(funcid);
>
> !                 if (!leaky)
> !                 {
> !                     getTypeOutputInfo(expr->resulttype, &funcid, &varlena);
> !                     leaky = !get_func_leakproof(funcid);
> !                 }
> !
> !                 if (leaky &&
> !                     contain_var_clause((Node *) expr->arg))
>                       return true;
>               }
>               break;

That seems slightly convoluted to me.  Why not simply do:

bool in_leakproof, out_leakproof;

getTypeInputInfo(exprType((Node *) expr->arg), &funcid, &ioparam);
in_leakproof = get_func_leakproof(funcid);

getTypeOutputInfo(expr->resulttype, &funcid, &varlena);
out_leakproof = get_func_leakproof(funcid);

if (!(in_leakproof && out_leakproof) && contain_var_clause((Node *)))return true;

...

> --- 1432,1452 ----
>                   ArrayCoerceExpr *expr = (ArrayCoerceExpr *) node;
>                   Oid            funcid;
>                   Oid            ioparam;
> +                 bool        leaky;
>                   bool        varlena;
>
>                   getTypeInputInfo(exprType((Node *) expr->arg),
>                                    &funcid, &ioparam);
> !                 leaky = !get_func_leakproof(funcid);
> !
> !                 if (!leaky)
> !                 {
> !                     getTypeOutputInfo(expr->resulttype, &funcid, &varlena);
> !                     leaky = !get_func_leakproof(funcid);
> !                 }
> !
> !                 if (leaky &&
> !                     contain_var_clause((Node *) expr->arg))
>                       return true;
>               }
>               break;

Ditto here.

> *************** contain_leaky_functions_walker(Node *nod
> *** 1435,1446 ****
>               {
>                   RowCompareExpr *rcexpr = (RowCompareExpr *) node;
>                   ListCell   *opid;
>
> !                 foreach(opid, rcexpr->opnos)
>                   {
>                       Oid            funcid = get_opcode(lfirst_oid(opid));
>
> !                     if (!get_func_leakproof(funcid))
>                           return true;
>                   }
>               }
> --- 1455,1472 ----
>               {
>                   RowCompareExpr *rcexpr = (RowCompareExpr *) node;
>                   ListCell   *opid;
> +                 ListCell   *larg;
> +                 ListCell   *rarg;
>
> !                 forthree(opid, rcexpr->opnos,
> !                          larg, rcexpr->largs,
> !                          rarg, rcexpr->rargs)
>                   {
>                       Oid            funcid = get_opcode(lfirst_oid(opid));
>
> !                     if (!get_func_leakproof(funcid) &&
> !                         (contain_var_clause((Node *) lfirst(larg)) ||
> !                          contain_var_clause((Node *) lfirst(rarg))))
>                           return true;
>                   }
>               }

Might look a bit cleaner as:

/* Leakproof functions are ok */
if (get_func_leakproof(funcid))continue;

/* Not leakproof, check if there are any Vars passed in */
if (contain_var_clause((Node *) lfirst(larg)) ||contain_var_clause((Node *) lfirst(rarg)))return true;

The rest looked good to me.
Thanks!
    Stephen