Re: RLS bug in expanding security quals

From: Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>
To: Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>
Cc: Stephen Frost <sfrost(at)snowman(dot)net>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: RLS bug in expanding security quals
Date: 2015-10-08 14:05:37
Message-ID: CAEZATCXcw5_FEmkC3hfELE-6bC=2qrrd93FzxA_rsfKW28LvWw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 8 October 2015 at 05:45, Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com> wrote:
> On Thu, Oct 8, 2015 at 2:54 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> It's quite late here, but I'll take a look at this in more depth
>> tomorrow.
>>
>> Based on what the Assert's testing, I took an educated guess and tried
>> running without the UNION ALL, which appeared to work correctly.
>
> Yes, it works fine without UNION ALL.
>

I took a look at this and it appears to be a bug in the UNION ALL
handling of queries with security quals. An even simpler test case
that triggers it is:

create role test_user1;
drop table if exists foo cascade;
create table foo(a int);
grant all on foo to test_user1;

alter table foo enable row level security;
create policy foo_policy on foo for select using (a > 0);

set role test_user1;
explain (verbose, costs off)
SELECT a FROM foo
UNION ALL
SELECT 1;

What's happening is that flatten_simple_union_all() and
pull_up_union_leaf_queries() is building translated_vars lists on
appendrels, prior to the security quals being expanded, and those
security quals are adjusted to point to the parent rel. Then the code
to expand the security quals on the child RTEs no longer sees any Vars
pointing to those RTEs, so the resulting subquery RTEs end up with
empty targetlists.

This appears to be a simple oversight in expand_security_qual() -- it
needs to look at and update the Vars in the translated_vars lists of
the appendrels to work properly. I think I wasn't expecting for things
outside the Query to be pointing into its guts in this way.

Attached is a simple patch that appears to work, but it needs more
testing (and some regression tests).

Regards,
Dean

Attachment Content-Type Size
rls-bug.patch text/x-patch 1008 bytes

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Beena Emerson 2015-10-08 14:10:23 Re: Support for N synchronous standby servers - take 2
Previous Message Beena Emerson 2015-10-08 14:01:12 Re: Support for N synchronous standby servers - take 2