BUG #15708: RLS 'using' running as wrong user when called from a view
The following bug has been logged on the website:
Bug reference: 15708
Logged by: Daurnimator
Email address: quae@daurnimator.com
PostgreSQL version: 11.2
Operating system: linux
Description:
(from https://gist.github.com/daurnimator/b1d2c16359e346a466b3093ae2757acf
)
This fails, seemingly because the RLS on 'bar' is being checked by alice,
instead of the view owner bob:
```sql
create role alice;
create table bar(a integer);
alter table bar enable row level security;
create table qux(b integer);
create role bob;
create policy blahblah on bar to bob
using(exists(select 1 from qux));
grant select on table bar to bob;
grant select on table qux to bob;
create view foo as select * from bar;
alter view foo owner to bob;
grant select on table foo to alice;
-- grant select on table qux to alice; -- shouldn't be required
set role alice;
select * from foo;
```
```
$ psql -f rls_trouble.sql
CREATE ROLE
CREATE TABLE
ALTER TABLE
CREATE TABLE
CREATE ROLE
CREATE POLICY
GRANT
GRANT
CREATE VIEW
ALTER VIEW
GRANT
SET
psql:rls_trouble.sql:18: ERROR: permission denied for table qux
```
If we add an indirection via another view, then I get the result I
expected...
```sql
create role alice;
create table bar(a integer);
alter table bar enable row level security;
create table qux(b integer);
-- if we add a layer of indirection it works.... wat?
create view indirection as select * from bar;
create role bob;
create policy blahblah on bar to bob
using(exists(select 1 from qux));
grant select on table bar to bob;
grant select on table indirection to bob;
grant select on table qux to bob;
create view foo as select * from indirection;
alter view foo owner to bob;
grant select on table foo to alice;
set role alice;
select * from foo;
```
On Thu, 21 Mar 2019 at 00:39, PG Bug reporting form
<noreply@postgresql.org> wrote:
This fails, seemingly because the RLS on 'bar' is being checked by alice,
instead of the view owner bob:
Yes I agree, that appears to be a bug. The subquery in the RLS policy
should be checked as the view owner -- i.e., we need to propagate the
checkAsUser for the RTE with RLS to any subqueries in its RLS
policies.
It looks like the best place to fix it is in
get_policies_for_relation(), since that's where all the policies to be
applied for a given RTE are pulled together. Patch attached.
Regards,
Dean
Attachments:
rls-perm-check-fix.patchapplication/octet-stream; name=rls-perm-check-fix.patchDownload
diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c
new file mode 100644
index 835540c..8cd5ff0
--- a/src/backend/rewrite/rowsecurity.c
+++ b/src/backend/rewrite/rowsecurity.c
@@ -47,6 +47,7 @@
#include "nodes/pg_list.h"
#include "nodes/plannodes.h"
#include "parser/parsetree.h"
+#include "rewrite/rewriteDefine.h"
#include "rewrite/rewriteHandler.h"
#include "rewrite/rewriteManip.h"
#include "rewrite/rowsecurity.h"
@@ -504,6 +505,29 @@ get_policies_for_relation(Relation relat
*permissive_policies = lappend(*permissive_policies, policy);
}
}
+
+ /*
+ * Use the specified role to perform all permission checks on any
+ * relations referred to by the policies. This role is the checkAsUser
+ * for the RTE that these policies are for, which isn't necessarily the
+ * current user (it may be a view owner in the case of a view on top of a
+ * table with RLS), and we need that checkAsUser to propagate to any
+ * subqueries in the RLS policies.
+ */
+ foreach(item, *permissive_policies)
+ {
+ RowSecurityPolicy *policy = (RowSecurityPolicy *) lfirst(item);
+
+ setRuleCheckAsUser((Node *) policy->qual, user_id);
+ setRuleCheckAsUser((Node *) policy->with_check_qual, user_id);
+ }
+ foreach(item, *restrictive_policies)
+ {
+ RowSecurityPolicy *policy = (RowSecurityPolicy *) lfirst(item);
+
+ setRuleCheckAsUser((Node *) policy->qual, user_id);
+ setRuleCheckAsUser((Node *) policy->with_check_qual, user_id);
+ }
}
/*
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
new file mode 100644
index bad5199..f532dea
--- a/src/test/regress/expected/rowsecurity.out
+++ b/src/test/regress/expected/rowsecurity.out
@@ -3912,6 +3912,33 @@ DROP OWNED BY regress_rls_dob_role1;
DROP POLICY p1 ON dob_t2; -- should succeed
DROP USER regress_rls_dob_role1;
DROP USER regress_rls_dob_role2;
+-- Bug #15708: view + table with RLS should check policies as view owner
+CREATE TABLE ref_tbl (a int);
+INSERT INTO ref_tbl VALUES (1);
+CREATE TABLE rls_tbl (a int);
+INSERT INTO rls_tbl VALUES (10);
+ALTER TABLE rls_tbl ENABLE ROW LEVEL SECURITY;
+CREATE POLICY p1 ON rls_tbl USING (EXISTS (SELECT 1 FROM ref_tbl));
+GRANT SELECT ON ref_tbl TO regress_rls_bob;
+GRANT SELECT ON rls_tbl TO regress_rls_bob;
+CREATE VIEW rls_view AS SELECT * FROM rls_tbl;
+ALTER VIEW rls_view OWNER TO regress_rls_bob;
+GRANT SELECT ON rls_view TO regress_rls_alice;
+SET SESSION AUTHORIZATION regress_rls_alice;
+SELECT * FROM ref_tbl; -- Permission denied
+ERROR: permission denied for table ref_tbl
+SELECT * FROM rls_tbl; -- Permission denied
+ERROR: permission denied for table rls_tbl
+SELECT * FROM rls_view; -- OK
+ a
+----
+ 10
+(1 row)
+
+RESET SESSION AUTHORIZATION;
+DROP VIEW rls_view;
+DROP TABLE rls_tbl;
+DROP TABLE ref_tbl;
--
-- Clean up objects
--
diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql
new file mode 100644
index 52da276..d7ed996
--- a/src/test/regress/sql/rowsecurity.sql
+++ b/src/test/regress/sql/rowsecurity.sql
@@ -1768,6 +1768,32 @@ DROP POLICY p1 ON dob_t2; -- should succ
DROP USER regress_rls_dob_role1;
DROP USER regress_rls_dob_role2;
+-- Bug #15708: view + table with RLS should check policies as view owner
+CREATE TABLE ref_tbl (a int);
+INSERT INTO ref_tbl VALUES (1);
+
+CREATE TABLE rls_tbl (a int);
+INSERT INTO rls_tbl VALUES (10);
+ALTER TABLE rls_tbl ENABLE ROW LEVEL SECURITY;
+CREATE POLICY p1 ON rls_tbl USING (EXISTS (SELECT 1 FROM ref_tbl));
+
+GRANT SELECT ON ref_tbl TO regress_rls_bob;
+GRANT SELECT ON rls_tbl TO regress_rls_bob;
+
+CREATE VIEW rls_view AS SELECT * FROM rls_tbl;
+ALTER VIEW rls_view OWNER TO regress_rls_bob;
+GRANT SELECT ON rls_view TO regress_rls_alice;
+
+SET SESSION AUTHORIZATION regress_rls_alice;
+SELECT * FROM ref_tbl; -- Permission denied
+SELECT * FROM rls_tbl; -- Permission denied
+SELECT * FROM rls_view; -- OK
+RESET SESSION AUTHORIZATION;
+
+DROP VIEW rls_view;
+DROP TABLE rls_tbl;
+DROP TABLE ref_tbl;
+
--
-- Clean up objects
--
Greetings,
* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
On Thu, 21 Mar 2019 at 00:39, PG Bug reporting form
<noreply@postgresql.org> wrote:This fails, seemingly because the RLS on 'bar' is being checked by alice,
instead of the view owner bob:Yes I agree, that appears to be a bug. The subquery in the RLS policy
should be checked as the view owner -- i.e., we need to propagate the
checkAsUser for the RTE with RLS to any subqueries in its RLS
policies.
Agreed.
It looks like the best place to fix it is in
get_policies_for_relation(), since that's where all the policies to be
applied for a given RTE are pulled together. Patch attached.
Yes, on a quick review, that looks like a good solution to me as well.
Thanks!
Stephen
On Mon, 25 Mar 2019 at 20:27, Stephen Frost <sfrost@snowman.net> wrote:
* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
It looks like the best place to fix it is in
get_policies_for_relation(), since that's where all the policies to be
applied for a given RTE are pulled together. Patch attached.Yes, on a quick review, that looks like a good solution to me as well.
On second thoughts, it actually needs to be in
get_row_security_policies(), after making copies of the quals from the
policies, otherwise it would be scribbling on the copies from the
relcache. Actually that makes the code change a bit simpler too.
Regards,
Dean
Attachments:
rls-perm-check-fix-v2.patchtext/x-patch; charset=US-ASCII; name=rls-perm-check-fix-v2.patchDownload
diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c
new file mode 100644
index 835540c..e9f532c
--- a/src/backend/rewrite/rowsecurity.c
+++ b/src/backend/rewrite/rowsecurity.c
@@ -47,6 +47,7 @@
#include "nodes/pg_list.h"
#include "nodes/plannodes.h"
#include "parser/parsetree.h"
+#include "rewrite/rewriteDefine.h"
#include "rewrite/rewriteHandler.h"
#include "rewrite/rewriteManip.h"
#include "rewrite/rowsecurity.h"
@@ -382,6 +383,13 @@ get_row_security_policies(Query *root, R
table_close(rel, NoLock);
/*
+ * Copy checkAsUser to the row security quals and WithCheckOption checks,
+ * in case they contain any subqueries referring to other relations.
+ */
+ setRuleCheckAsUser((Node *) *securityQuals, rte->checkAsUser);
+ setRuleCheckAsUser((Node *) *withCheckOptions, rte->checkAsUser);
+
+ /*
* Mark this query as having row security, so plancache can invalidate it
* when necessary (eg: role changes)
*/
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
new file mode 100644
index 4b53401..d764991
--- a/src/test/regress/expected/rowsecurity.out
+++ b/src/test/regress/expected/rowsecurity.out
@@ -3910,6 +3910,33 @@ DROP OWNED BY regress_rls_dob_role1;
DROP POLICY p1 ON dob_t2; -- should succeed
DROP USER regress_rls_dob_role1;
DROP USER regress_rls_dob_role2;
+-- Bug #15708: view + table with RLS should check policies as view owner
+CREATE TABLE ref_tbl (a int);
+INSERT INTO ref_tbl VALUES (1);
+CREATE TABLE rls_tbl (a int);
+INSERT INTO rls_tbl VALUES (10);
+ALTER TABLE rls_tbl ENABLE ROW LEVEL SECURITY;
+CREATE POLICY p1 ON rls_tbl USING (EXISTS (SELECT 1 FROM ref_tbl));
+GRANT SELECT ON ref_tbl TO regress_rls_bob;
+GRANT SELECT ON rls_tbl TO regress_rls_bob;
+CREATE VIEW rls_view AS SELECT * FROM rls_tbl;
+ALTER VIEW rls_view OWNER TO regress_rls_bob;
+GRANT SELECT ON rls_view TO regress_rls_alice;
+SET SESSION AUTHORIZATION regress_rls_alice;
+SELECT * FROM ref_tbl; -- Permission denied
+ERROR: permission denied for table ref_tbl
+SELECT * FROM rls_tbl; -- Permission denied
+ERROR: permission denied for table rls_tbl
+SELECT * FROM rls_view; -- OK
+ a
+----
+ 10
+(1 row)
+
+RESET SESSION AUTHORIZATION;
+DROP VIEW rls_view;
+DROP TABLE rls_tbl;
+DROP TABLE ref_tbl;
--
-- Clean up objects
--
diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql
new file mode 100644
index ea83153..df8fe11
--- a/src/test/regress/sql/rowsecurity.sql
+++ b/src/test/regress/sql/rowsecurity.sql
@@ -1764,6 +1764,32 @@ DROP POLICY p1 ON dob_t2; -- should succ
DROP USER regress_rls_dob_role1;
DROP USER regress_rls_dob_role2;
+-- Bug #15708: view + table with RLS should check policies as view owner
+CREATE TABLE ref_tbl (a int);
+INSERT INTO ref_tbl VALUES (1);
+
+CREATE TABLE rls_tbl (a int);
+INSERT INTO rls_tbl VALUES (10);
+ALTER TABLE rls_tbl ENABLE ROW LEVEL SECURITY;
+CREATE POLICY p1 ON rls_tbl USING (EXISTS (SELECT 1 FROM ref_tbl));
+
+GRANT SELECT ON ref_tbl TO regress_rls_bob;
+GRANT SELECT ON rls_tbl TO regress_rls_bob;
+
+CREATE VIEW rls_view AS SELECT * FROM rls_tbl;
+ALTER VIEW rls_view OWNER TO regress_rls_bob;
+GRANT SELECT ON rls_view TO regress_rls_alice;
+
+SET SESSION AUTHORIZATION regress_rls_alice;
+SELECT * FROM ref_tbl; -- Permission denied
+SELECT * FROM rls_tbl; -- Permission denied
+SELECT * FROM rls_view; -- OK
+RESET SESSION AUTHORIZATION;
+
+DROP VIEW rls_view;
+DROP TABLE rls_tbl;
+DROP TABLE ref_tbl;
+
--
-- Clean up objects
--
On Wed, 27 Mar 2019 at 23:46, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On second thoughts, it actually needs to be in
get_row_security_policies(), after making copies of the quals from the
policies, otherwise it would be scribbling on the copies from the
relcache. Actually that makes the code change a bit simpler too.
Thanks for writing the patch!
I'm sad this missed the last commit fest; I think this bug might be
causing security issues in a few deployments.
Could you submit the patch for the next commit fest?
On Mon, 29 Apr 2019 at 04:56, Daurnimator <quae@daurnimator.com> wrote:
On Wed, 27 Mar 2019 at 23:46, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On second thoughts, it actually needs to be in
get_row_security_policies(), after making copies of the quals from the
policies, otherwise it would be scribbling on the copies from the
relcache. Actually that makes the code change a bit simpler too.Thanks for writing the patch!
I'm sad this missed the last commit fest; I think this bug might be
causing security issues in a few deployments.
Could you submit the patch for the next commit fest?
Actually I pushed the fix for this a while ago [1]https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=e2d28c0f404713f564dc2250646551c75172f17b (sorry I forgot to
reply back to this thread), so it will be available in the next set of
minor version updates later this week.
Regards,
Dean