Re: Improving RLS qual pushdown

Thanks for looking at this.

On 28 February 2015 at 04:25, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Dean,
>
> * Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)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.
>

OK fair enough.

>> --- 1318,1347 ----
>> }
>>
>> /*****************************************************************************
>> ! * Check clauses for non-leakproof functions that might leak Vars
>> *****************************************************************************/
>
> Might leak data in Vars (or somethhing along those lines...)
>

Perhaps just "Check clauses for Vars passed to non-leakproof
functions". The more detailed explanation below is probably then
sufficient to cover why that's significant.

>> /*
>> ! * 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).
>

That sounds OK (except s/sensetive/sensitive).

>> --- 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;
>

I'm not sure that's really much clearer, because of the 2
double-negatives in the final if-clause, and it's less efficient in
the case where the input function is leaky, when it's not nessecary to
check the output function (which involves 2 cache lookups).

> ...
>
>> --- 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;
>

Shrug. Not sure it makes much difference either way.

> The rest looked good to me.
>

Thanks. Do you want me to post an update, or are you going to hack on it?

Regards,
Dean