Обсуждение: RLS bug in expanding security quals

Haribabu,

* Haribabu Kommi (kommi.haribabu@gmail.com) wrote:
> During the testing of multi-tenancy feature from system catalog views, that
> is described in [1], found a problem in executing "user_privileges" view
> from information_schema. The following is the minimal test sql that
> reproduces the problem.

Interesting, thanks.

> >From further analysis, I found that the same issue can happen with user
> tables also. Attached
> rls_failure.sql file has test steps to reproduce the issue.

Just to make sure we're on the same page, this results in this assertion
being tripped:

TRAP: FailedAssertion("!(var->varattno <= rel->max_attr)", File:
"/home/sfrost/git/pg/dev/postgresql/src/backend/optimizer/path/costsize.c",
Line: 4152)

Due to var->varattno being 1 and rel->max_attr being 0.

> The problem is, while expanding security quals in
> function expand_security_quals, the relation
> u_grantor and test_tbl are to be expanded as they are the relations which
> have security quals.
>
> Following is the debug information of parse->rtable that shows the details
> of each RangeTblEntry.
>
> $69 = {type = T_Alias, aliasname = 0x19bd870 "u_grantor", colnames =
> 0x19bd890}
> (gdb) p *((RangeTblEntry
> *)parse->rtable->head->next->next->next->data.ptr_value)->eref
> $70 = {type = T_Alias, aliasname = 0x19bffc8 "grantee", colnames =
> 0x19bffe0}
> (gdb) p *((RangeTblEntry
> *)parse->rtable->head->next->next->next->next->data.ptr_value)->eref
> $71 = {type = T_Alias, aliasname = 0x19c3a60 "*SELECT* 1", colnames =
> 0x19c3a80}
> (gdb) p *((RangeTblEntry
> *)parse->rtable->head->next->next->next->next->next->data.ptr_value)->eref
> $72 = {type = T_Alias, aliasname = 0x19c40d8 "*SELECT* 2", colnames =
> 0x19c40f8}
> (gdb) p *((RangeTblEntry
> *)parse->rtable->head->next->next->next->next->next->next->data.ptr_value)->eref
> $73 = {type = T_Alias, aliasname = 0x19c4648 "test_tbl", colnames =
> 0x19c4668}
>
>
> In expand_security_qual function, the security_barrier_replace_vars
> function is called to prepare the context.targetlist. But this function
> doesn't generate targetlist for test_tbl RangeTblEntry. Because of this
> reason, while accessing the targetlist, it fails and throws an error.
>
> In case if the policy is changed to below other than specified in the
> rls_failure.sql
>
> create policy test_tbl_policy on test_tbl for select using(true);
>
> the query execution passes, because in expand_security_quals function,
> the rangeTableEntry_used function returns false for test_tbl entry, thus it
> avoids expanding the security qual.

Interesting.

> Any ideas how to handle this problem?

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.

Thanks!

Stephen

On Thu, Oct 8, 2015 at 2:54 PM, Stephen Frost <sfrost@snowman.net> wrote:
> Haribabu,
>
> * Haribabu Kommi (kommi.haribabu@gmail.com) wrote:
>> During the testing of multi-tenancy feature from system catalog views, that
>> is described in [1], found a problem in executing "user_privileges" view
>> from information_schema. The following is the minimal test sql that
>> reproduces the problem.
>
> Interesting, thanks.
>
>> >From further analysis, I found that the same issue can happen with user
>> tables also. Attached
>> rls_failure.sql file has test steps to reproduce the issue.
>
> Just to make sure we're on the same page, this results in this assertion
> being tripped:
>
> TRAP: FailedAssertion("!(var->varattno <= rel->max_attr)", File:
> "/home/sfrost/git/pg/dev/postgresql/src/backend/optimizer/path/costsize.c",
> Line: 4152)
>
> Due to var->varattno being 1 and rel->max_attr being 0.

Yes, the same the assertion problem with assert build.

without assert build, query fails with the following error.

ERROR:  invalid attnum -2 for rangetable entry test_tbl


>> Any ideas how to handle this problem?
>
> 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.

And also if we change the table column datatype from name to char,
the "pull_up_subqueries" function doesn't pull the union all because of
datatype mismatch and it works fine even with row level security is enabled.

Regards,
Hari Babu
Fujitsu Australia

On 8 October 2015 at 05:45, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
> On Thu, Oct 8, 2015 at 2:54 PM, Stephen Frost <sfrost@snowman.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

On 8 October 2015 at 15:05, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> Attached is a simple patch that appears to work, but it needs more
> testing (and some regression tests).
>

Here's an updated patch with an extra regression test case that
triggers the issue.

I've also updated the function comment for expand_security_quals() to
better explain the situations where it actually has work to do --
tables with RLS and updates to auto-updatable security barrier views,
but not SELECTs from security berrier views. This explains why this
bug doesn't affect security barrier views (UNION ALL views aren't
auto-updatable), so only 9.5 and HEAD need to be patched.

Regards,
Dean

On Fri, Oct 9, 2015 at 3:50 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> On 8 October 2015 at 15:05, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>> Attached is a simple patch that appears to work, but it needs more
>> testing (and some regression tests).
>>
>
> Here's an updated patch with an extra regression test case that
> triggers the issue.
>
> I've also updated the function comment for expand_security_quals() to
> better explain the situations where it actually has work to do --
> tables with RLS and updates to auto-updatable security barrier views,
> but not SELECTs from security berrier views. This explains why this
> bug doesn't affect security barrier views (UNION ALL views aren't
> auto-updatable), so only 9.5 and HEAD need to be patched.

Thanks for the patch. I didn't find any problem in my test with the patch.

Regards,
Hari Babu
Fujitsu Australia

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 8 October 2015 at 15:05, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> > Attached is a simple patch that appears to work, but it needs more
> > testing (and some regression tests).
>
> Here's an updated patch with an extra regression test case that
> triggers the issue.

Thanks!

> I've also updated the function comment for expand_security_quals() to
> better explain the situations where it actually has work to do --
> tables with RLS and updates to auto-updatable security barrier views,
> but not SELECTs from security berrier views. This explains why this
> bug doesn't affect security barrier views (UNION ALL views aren't
> auto-updatable), so only 9.5 and HEAD need to be patched.

Excellent, I definitely like the additional comments.

I plan to do a bit more testing tomorrow morning, but barring any
issues found or concerns raised, I'll push this sometime tomorrow.

Thanks again!

Stephen

* Haribabu Kommi (kommi.haribabu@gmail.com) wrote:
> On Fri, Oct 9, 2015 at 3:50 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> > On 8 October 2015 at 15:05, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> >> Attached is a simple patch that appears to work, but it needs more
> >> testing (and some regression tests).
> >>
> >
> > Here's an updated patch with an extra regression test case that
> > triggers the issue.
> >
> > I've also updated the function comment for expand_security_quals() to
> > better explain the situations where it actually has work to do --
> > tables with RLS and updates to auto-updatable security barrier views,
> > but not SELECTs from security berrier views. This explains why this
> > bug doesn't affect security barrier views (UNION ALL views aren't
> > auto-updatable), so only 9.5 and HEAD need to be patched.
>
> Thanks for the patch. I didn't find any problem in my test with the patch.

Excellent, fix pushed.

I also updated the Open Items wiki (putting this under 'resolved after
9.5beta1), in case folks run into it while testing beta1.

Thanks!

Stephen