Thread: WINDOW RANGE patch versus leakproofness

I am wondering whether we need to worry about leakproofness in connection
with adding in_range support functions to btree opclasses.  My initial
conclusion is not, but let me lay out the reasoning so people can check
it --- I might be missing something.

The first question is whether we need to encourage in_range functions
to be leakproof.  In the submitted patch, they mostly are not, because
they will happily blurt out the value of the "offset" argument if it's
negative, eg

    if (offset < 0)
        ereport(ERROR,
                (errcode(ERRCODE_WINDOWING_ERROR),
                 errmsg("RANGE offsets cannot be negative. invalid value %d", offset)));

(I'm not concerned here about whether this message text meets our style
guidelines, only about whether it should include the value of "offset".)
I think we could drop printing the value without a great deal of loss of
user-friendliness, although it is probably worth something.  But there
are other cases that would be harder to prevent, for example an in_range
function might rely on a subtraction function that will complain on
overflow.  So on the whole it'd be nicer if there were not a reason to
be concerned about leakiness of these functions.

The second question is whether there's even any need to worry about
leakproofness of support functions for window functions.  Window function
calls are only allowed in a SELECT's target list and ORDER BY, not in
anything that could get pushed underneath a qual, so my first thought is
that we do not actually care.  If we do care, there are weaknesses in this
area already, because contain_leaked_vars doesn't consider WindowFuncs at
all, let alone dig into the other support functions (such as equality)
that a WindowFunc node will use.  But ISTM that's not an oversight, just
not bothering to write code that would be dead.

So I'm thinking that (a) we do not need to check for leaky functions used
in window support, and (b) therefore there's no need to avoid leaky
behavior in in_range support functions.  Objections?

            regards, tom lane

On 30 January 2018 at 16:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> So I'm thinking that (a) we do not need to check for leaky functions used
> in window support, and (b) therefore there's no need to avoid leaky
> behavior in in_range support functions.  Objections?
>

Yes, I concur. Since window functions can only appear in the SELECT
target list and ORDER BY clauses, they should never appear in a qual
that gets considered for push down, and thus contain_leaked_vars()
should never see a window function.

Moreover, contain_leaked_vars() is intentionally coded defensively, so
if it ever does somehow see a window function (or any other unexpected
node type) it will return true and the resulting qual/restrictinfo
will be marked leaky, and not pushed through security barriers.

Regards,
Dean

On Wed, Jan 31, 2018 at 5:52 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> On 30 January 2018 at 16:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> So I'm thinking that (a) we do not need to check for leaky functions used
>> in window support, and (b) therefore there's no need to avoid leaky
>> behavior in in_range support functions.  Objections?
>
> Yes, I concur. Since window functions can only appear in the SELECT
> target list and ORDER BY clauses, they should never appear in a qual
> that gets considered for push down, and thus contain_leaked_vars()
> should never see a window function.

What about a query that uses window functions within a subquery?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 31 January 2018 at 21:51, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Jan 31, 2018 at 5:52 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>> On 30 January 2018 at 16:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> So I'm thinking that (a) we do not need to check for leaky functions used
>>> in window support, and (b) therefore there's no need to avoid leaky
>>> behavior in in_range support functions.  Objections?
>>
>> Yes, I concur. Since window functions can only appear in the SELECT
>> target list and ORDER BY clauses, they should never appear in a qual
>> that gets considered for push down, and thus contain_leaked_vars()
>> should never see a window function.
>
> What about a query that uses window functions within a subquery?
>

A qual containing a subquery is treated as not push down safe, so it
wouldn't even be considered for pushing down into a security barrier
view. On a table with RLS, contain_leaked_vars() would see a subplan
on the restrictinfo's clause and return true, causing the restrictinfo
to be treated as leaky. So in each case, the qual with the subquery
would be executed after any security quals, regardless of what it
contained.

The contents of the subquery aren't currently examined, it just
defaults to leaky. If they were to be examined, the window function
would be found and it would still default to being leaky.

Regards,
Dean