Re: Improving RLS qual pushdown

Thanks for looking at this.

On 28 February 2015 at 04:25, Stephen Frost <sfrost@snowman.net> wrote:
> 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.
>

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