Re: [HACKERS] RLS bug in expanding security quals

[Stephen Frost]

* Haribabu Kommi (kommi.harib...@gmail.com) wrote:
> On Fri, Oct 9, 2015 at 3:50 AM, Dean Rasheed  wrote:
> > On 8 October 2015 at 15:05, Dean Rasheed  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


signature.asc
Description: Digital signature

Re: [HACKERS] RLS bug in expanding security quals

[Dean Rasheed]

On 8 October 2015 at 05:45, Haribabu Kommi  wrote:
> On Thu, Oct 8, 2015 at 2:54 PM, Stephen Frost  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
diff --git a/src/backend/optimizer/prep/prepsecurity.c b/src/backend/optimizer/prep/prepsecurity.c
new file mode 100644
index c4b61df..c0d8a35
--- a/src/backend/optimizer/prep/prepsecurity.c
+++ b/src/backend/optimizer/prep/prepsecurity.c
@@ -263,7 +263,8 @@ expand_security_qual(PlannerInfo *root,
 			 * Replace any variables in the outer query that refer to the
 			 * original relation RTE with references to columns that we will
 			 * expose in the new subquery, building the subquery's targetlist
-			 * as we go.
+			 * as we go.  Also replace any references in the translated_vars
+			 * lists of any appendrels.
 			 */
 			context.rt_index = rt_index;
 			context.sublevels_up = 0;
@@ -274,6 +275,8 @@ expand_security_qual(PlannerInfo *root,
 
 			security_barrier_replace_vars((Node *) parse, );
 			security_barrier_replace_vars((Node *) tlist, );
+			security_barrier_replace_vars((Node *) root->append_rel_list,
+		  );
 
 			heap_close(context.rel, NoLock);
 

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] RLS bug in expanding security quals

[Dean Rasheed]

On 8 October 2015 at 15:05, Dean Rasheed  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
diff --git a/src/backend/optimizer/prep/prepsecurity.c b/src/backend/optimizer/prep/prepsecurity.c
new file mode 100644
index c4b61df..ee1e1e4
--- a/src/backend/optimizer/prep/prepsecurity.c
+++ b/src/backend/optimizer/prep/prepsecurity.c
@@ -56,6 +56,12 @@ static bool security_barrier_replace_var
  * the others, providing protection against malicious user-defined security
  * barriers.  The first security barrier qual in the list will be used in the
  * innermost subquery.
+ *
+ * In practice, the only RTEs that will have security barrier quals are those
+ * that refer to tables with row-level security, or which are the target
+ * relation of an update to an auto-updatable security barrier view.  RTEs
+ * that read from a security barrier view will have already been expanded by
+ * the rewriter.
  */
 void
 expand_security_quals(PlannerInfo *root, List *tlist)
@@ -263,7 +269,8 @@ expand_security_qual(PlannerInfo *root,
 			 * Replace any variables in the outer query that refer to the
 			 * original relation RTE with references to columns that we will
 			 * expose in the new subquery, building the subquery's targetlist
-			 * as we go.
+			 * as we go.  Also replace any references in the translated_vars
+			 * lists of any appendrels.
 			 */
 			context.rt_index = rt_index;
 			context.sublevels_up = 0;
@@ -274,6 +281,8 @@ expand_security_qual(PlannerInfo *root,
 
 			security_barrier_replace_vars((Node *) parse, );
 			security_barrier_replace_vars((Node *) tlist, );
+			security_barrier_replace_vars((Node *) root->append_rel_list,
+		  );
 
 			heap_close(context.rel, NoLock);
 
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
new file mode 100644
index 6becf59..c844499
--- a/src/test/regress/expected/rowsecurity.out
+++ b/src/test/regress/expected/rowsecurity.out
@@ -640,6 +640,26 @@ EXPLAIN (COSTS OFF) SELECT * FROM t1 WHE
  Filter: ((a % 2) = 0)
 (12 rows)
 
+-- union all query
+SELECT a, b, oid FROM t2 UNION ALL SELECT a, b, oid FROM t3;
+ a |  b  | oid 
+---+-+-
+ 1 | abc | 201
+ 3 | cde | 203
+ 1 | xxx | 301
+ 2 | yyy | 302
+ 3 | zzz | 303
+(5 rows)
+
+EXPLAIN (COSTS OFF) SELECT a, b, oid FROM t2 UNION ALL SELECT a, b, oid FROM t3;
+  QUERY PLAN   
+---
+ Append
+   ->  Seq Scan on t2
+ Filter: ((a % 2) = 1)
+   ->  Seq Scan on t3
+(4 rows)
+
 -- superuser is allowed to bypass RLS checks
 RESET SESSION AUTHORIZATION;
 SET row_security TO OFF;
diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql
new file mode 100644
index 662f520..b230a0f
--- a/src/test/regress/sql/rowsecurity.sql
+++ b/src/test/regress/sql/rowsecurity.sql
@@ -255,6 +255,10 @@ EXPLAIN (COSTS OFF) SELECT * FROM t1 FOR
 SELECT * FROM t1 WHERE f_leak(b) FOR SHARE;
 EXPLAIN (COSTS OFF) SELECT * FROM t1 WHERE f_leak(b) FOR SHARE;
 
+-- union all query
+SELECT a, b, oid FROM t2 UNION ALL SELECT a, b, oid FROM t3;
+EXPLAIN (COSTS OFF) SELECT a, b, oid FROM t2 UNION ALL SELECT a, b, oid FROM t3;
+
 -- superuser is allowed to bypass RLS checks
 RESET SESSION AUTHORIZATION;
 SET row_security TO OFF;

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] RLS bug in expanding security quals

[Stephen Frost]

Dean,

* Dean Rasheed (dean.a.rash...@gmail.com) wrote:
> On 8 October 2015 at 15:05, Dean Rasheed  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


signature.asc
Description: Digital signature

Re: [HACKERS] RLS bug in expanding security quals

[Haribabu Kommi]

On Fri, Oct 9, 2015 at 3:50 AM, Dean Rasheed  wrote:
> On 8 October 2015 at 15:05, Dean Rasheed  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] RLS bug in expanding security quals

[Stephen Frost]

Haribabu,

* Haribabu Kommi (kommi.harib...@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


signature.asc
Description: Digital signature

[HACKERS] RLS bug in expanding security quals

[Haribabu Kommi]

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.

SELECT (u_grantor.rolname) AS grantor,  (grantee.rolname) AS grantee
  FROM  pg_authid u_grantor,

  ( SELECT pg_authid.oid,

  pg_authid.rolname

 FROM pg_authid

  UNION ALL

  SELECT (0)::oid AS oid,

   'PUBLIC'::name) grantee(oid, rolname)
WHERE
(grantee.rolname = 'PUBLIC'::name)

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

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.

Any ideas how to handle this problem?

Regards,
Hari Babu
Fujitsu Australia

[1] -
http://www.postgresql.org/message-id/cajrrpgd2cf4hz_edpx+uqjv1ytkajs_wjdiwj7pzzuuqwou...@mail.gmail.com


rls_failure.sql
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] RLS bug in expanding security quals

[Haribabu Kommi]

On Thu, Oct 8, 2015 at 2:54 PM, Stephen Frost  wrote:
> Haribabu,
>
> * Haribabu Kommi (kommi.harib...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers