Fix enforcement of SELECT FOR UPDATE permissions with nested views.

SELECT FOR UPDATE on a view should require UPDATE (as well as SELECT)
permissions on the view, and then the view's owner needs those same
permissions against the relations it references, and so on all the way
down to base tables.  But ApplyRetrieveRule did things in the wrong order,
resulting in failure to mark intermediate view levels as needing UPDATE
permission.  Thus for example, if user A creates a table T and an updatable
view V1 on T, then grants only SELECT permissions on V1 to user B, B could
create a second view V2 on V1 and then would be allowed to perform SELECT
FOR UPDATE via V2 (since V1 wouldn't be checked for UPDATE permissions).

To fix, just switch the order of expanding sub-views and marking referenced
objects as needing UPDATE permission.  I think additional simplifications
are now possible, but that's distinct from the bug fix proper.

This is certainly a security issue, but the consequences are pretty minor
(just the ability to lock rows that shouldn't be lockable).  Against that
we have a small risk of breaking applications that are working as-desired,
since nested views have behaved this way since such cases worked at all.
On balance I'm inclined not to back-patch.

Per report from Alexander Lakhin.

Discussion: https://postgr.es/m/24db7b8f-3de5-e25f-7ab9-d8848351d42c@gmail.com

diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 88140bc6877455826be33d13320960a760e79802..981a233d1070d5c678da8354521886517e9d1eaf 100644 (file)
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -1559,8 +1559,27 @@ ApplyRetrieveRule(Query *parsetree,
 
        AcquireRewriteLocks(rule_action, true, forUpdatePushedDown);
 
+       /*
+        * If FOR [KEY] UPDATE/SHARE of view, mark all the contained tables as
+        * implicit FOR [KEY] UPDATE/SHARE, the same as the parser would have done
+        * if the view's subquery had been written out explicitly.
+        *
+        * Note: we needn't consider forUpdatePushedDown for this; if there was an
+        * ancestor query level with a relevant FOR [KEY] UPDATE/SHARE clause,
+        * that's already been pushed down to here and is reflected in "rc".
+        */
+       if (rc != NULL)
+               markQueryForLocking(rule_action, (Node *) rule_action->jointree,
+                                                       rc->strength, rc->waitPolicy, true);
+
        /*
         * Recursively expand any view references inside the view.
+        *
+        * Note: this must happen after markQueryForLocking.  That way, any UPDATE
+        * permission bits needed for sub-views are initially applied to their
+        * RTE_RELATION RTEs by markQueryForLocking, and then transferred to their
+        * OLD rangetable entries by the action below (in a recursive call of this
+        * routine).
         */
        rule_action = fireRIRrules(rule_action, activeRIRs, forUpdatePushedDown);
 
@@ -1594,18 +1613,6 @@ ApplyRetrieveRule(Query *parsetree,
        rte->insertedCols = NULL;
        rte->updatedCols = NULL;
 
-       /*
-        * If FOR [KEY] UPDATE/SHARE of view, mark all the contained tables as
-        * implicit FOR [KEY] UPDATE/SHARE, the same as the parser would have done
-        * if the view's subquery had been written out explicitly.
-        *
-        * Note: we don't consider forUpdatePushedDown here; such marks will be
-        * made by recursing from the upper level in markQueryForLocking.
-        */
-       if (rc != NULL)
-               markQueryForLocking(rule_action, (Node *) rule_action->jointree,
-                                                       rc->strength, rc->waitPolicy, true);
-
        return parsetree;
 }
 

diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out
index aedf8ce40447a3f293e72346df7d0b940eee8726..b34bab4b2973eb7fb7d14fc55bc0093664114d1a 100644 (file)
--- a/src/test/regress/expected/updatable_views.out
+++ b/src/test/regress/expected/updatable_views.out
@@ -1053,6 +1053,130 @@ SELECT * FROM base_tbl;
  5 | Row 5 | 5
 (2 rows)
 
+RESET SESSION AUTHORIZATION;
+DROP TABLE base_tbl CASCADE;
+NOTICE:  drop cascades to 2 other objects
+DETAIL:  drop cascades to view rw_view1
+drop cascades to view rw_view2
+-- nested-view permissions
+CREATE TABLE base_tbl(a int, b text, c float);
+INSERT INTO base_tbl VALUES (1, 'Row 1', 1.0);
+SET SESSION AUTHORIZATION regress_view_user1;
+CREATE VIEW rw_view1 AS SELECT * FROM base_tbl;
+SELECT * FROM rw_view1;  -- not allowed
+ERROR:  permission denied for table base_tbl
+SELECT * FROM rw_view1 FOR UPDATE;  -- not allowed
+ERROR:  permission denied for table base_tbl
+UPDATE rw_view1 SET b = 'foo' WHERE a = 1;  -- not allowed
+ERROR:  permission denied for table base_tbl
+SET SESSION AUTHORIZATION regress_view_user2;
+CREATE VIEW rw_view2 AS SELECT * FROM rw_view1;
+SELECT * FROM rw_view2;  -- not allowed
+ERROR:  permission denied for view rw_view1
+SELECT * FROM rw_view2 FOR UPDATE;  -- not allowed
+ERROR:  permission denied for view rw_view1
+UPDATE rw_view2 SET b = 'bar' WHERE a = 1;  -- not allowed
+ERROR:  permission denied for view rw_view1
+RESET SESSION AUTHORIZATION;
+GRANT SELECT ON base_tbl TO regress_view_user1;
+SET SESSION AUTHORIZATION regress_view_user1;
+SELECT * FROM rw_view1;
+ a |   b   | c 
+---+-------+---
+ 1 | Row 1 | 1
+(1 row)
+
+SELECT * FROM rw_view1 FOR UPDATE;  -- not allowed
+ERROR:  permission denied for table base_tbl
+UPDATE rw_view1 SET b = 'foo' WHERE a = 1;  -- not allowed
+ERROR:  permission denied for table base_tbl
+SET SESSION AUTHORIZATION regress_view_user2;
+SELECT * FROM rw_view2;  -- not allowed
+ERROR:  permission denied for view rw_view1
+SELECT * FROM rw_view2 FOR UPDATE;  -- not allowed
+ERROR:  permission denied for view rw_view1
+UPDATE rw_view2 SET b = 'bar' WHERE a = 1;  -- not allowed
+ERROR:  permission denied for view rw_view1
+SET SESSION AUTHORIZATION regress_view_user1;
+GRANT SELECT ON rw_view1 TO regress_view_user2;
+SET SESSION AUTHORIZATION regress_view_user2;
+SELECT * FROM rw_view2;
+ a |   b   | c 
+---+-------+---
+ 1 | Row 1 | 1
+(1 row)
+
+SELECT * FROM rw_view2 FOR UPDATE;  -- not allowed
+ERROR:  permission denied for view rw_view1
+UPDATE rw_view2 SET b = 'bar' WHERE a = 1;  -- not allowed
+ERROR:  permission denied for view rw_view1
+RESET SESSION AUTHORIZATION;
+GRANT UPDATE ON base_tbl TO regress_view_user1;
+SET SESSION AUTHORIZATION regress_view_user1;
+SELECT * FROM rw_view1;
+ a |   b   | c 
+---+-------+---
+ 1 | Row 1 | 1
+(1 row)
+
+SELECT * FROM rw_view1 FOR UPDATE;
+ a |   b   | c 
+---+-------+---
+ 1 | Row 1 | 1
+(1 row)
+
+UPDATE rw_view1 SET b = 'foo' WHERE a = 1;
+SET SESSION AUTHORIZATION regress_view_user2;
+SELECT * FROM rw_view2;
+ a |  b  | c 
+---+-----+---
+ 1 | foo | 1
+(1 row)
+
+SELECT * FROM rw_view2 FOR UPDATE;  -- not allowed
+ERROR:  permission denied for view rw_view1
+UPDATE rw_view2 SET b = 'bar' WHERE a = 1;  -- not allowed
+ERROR:  permission denied for view rw_view1
+SET SESSION AUTHORIZATION regress_view_user1;
+GRANT UPDATE ON rw_view1 TO regress_view_user2;
+SET SESSION AUTHORIZATION regress_view_user2;
+SELECT * FROM rw_view2;
+ a |  b  | c 
+---+-----+---
+ 1 | foo | 1
+(1 row)
+
+SELECT * FROM rw_view2 FOR UPDATE;
+ a |  b  | c 
+---+-----+---
+ 1 | foo | 1
+(1 row)
+
+UPDATE rw_view2 SET b = 'bar' WHERE a = 1;
+RESET SESSION AUTHORIZATION;
+REVOKE UPDATE ON base_tbl FROM regress_view_user1;
+SET SESSION AUTHORIZATION regress_view_user1;
+SELECT * FROM rw_view1;
+ a |  b  | c 
+---+-----+---
+ 1 | bar | 1
+(1 row)
+
+SELECT * FROM rw_view1 FOR UPDATE;  -- not allowed
+ERROR:  permission denied for table base_tbl
+UPDATE rw_view1 SET b = 'foo' WHERE a = 1;  -- not allowed
+ERROR:  permission denied for table base_tbl
+SET SESSION AUTHORIZATION regress_view_user2;
+SELECT * FROM rw_view2;
+ a |  b  | c 
+---+-----+---
+ 1 | bar | 1
+(1 row)
+
+SELECT * FROM rw_view2 FOR UPDATE;  -- not allowed
+ERROR:  permission denied for table base_tbl
+UPDATE rw_view2 SET b = 'bar' WHERE a = 1;  -- not allowed
+ERROR:  permission denied for table base_tbl
 RESET SESSION AUTHORIZATION;
 DROP TABLE base_tbl CASCADE;
 NOTICE:  drop cascades to 2 other objects

diff --git a/src/test/regress/sql/updatable_views.sql b/src/test/regress/sql/updatable_views.sql
index 6a9005243bbfd0ecf690ada21fd4c75c4e79ae3e..a7786b26e974d6c824a56f137cab93d0d98db0dc 100644 (file)
--- a/src/test/regress/sql/updatable_views.sql
+++ b/src/test/regress/sql/updatable_views.sql
@@ -459,6 +459,82 @@ RESET SESSION AUTHORIZATION;
 
 DROP TABLE base_tbl CASCADE;
 
+-- nested-view permissions
+
+CREATE TABLE base_tbl(a int, b text, c float);
+INSERT INTO base_tbl VALUES (1, 'Row 1', 1.0);
+
+SET SESSION AUTHORIZATION regress_view_user1;
+CREATE VIEW rw_view1 AS SELECT * FROM base_tbl;
+SELECT * FROM rw_view1;  -- not allowed
+SELECT * FROM rw_view1 FOR UPDATE;  -- not allowed
+UPDATE rw_view1 SET b = 'foo' WHERE a = 1;  -- not allowed
+
+SET SESSION AUTHORIZATION regress_view_user2;
+CREATE VIEW rw_view2 AS SELECT * FROM rw_view1;
+SELECT * FROM rw_view2;  -- not allowed
+SELECT * FROM rw_view2 FOR UPDATE;  -- not allowed
+UPDATE rw_view2 SET b = 'bar' WHERE a = 1;  -- not allowed
+
+RESET SESSION AUTHORIZATION;
+GRANT SELECT ON base_tbl TO regress_view_user1;
+
+SET SESSION AUTHORIZATION regress_view_user1;
+SELECT * FROM rw_view1;
+SELECT * FROM rw_view1 FOR UPDATE;  -- not allowed
+UPDATE rw_view1 SET b = 'foo' WHERE a = 1;  -- not allowed
+
+SET SESSION AUTHORIZATION regress_view_user2;
+SELECT * FROM rw_view2;  -- not allowed
+SELECT * FROM rw_view2 FOR UPDATE;  -- not allowed
+UPDATE rw_view2 SET b = 'bar' WHERE a = 1;  -- not allowed
+
+SET SESSION AUTHORIZATION regress_view_user1;
+GRANT SELECT ON rw_view1 TO regress_view_user2;
+
+SET SESSION AUTHORIZATION regress_view_user2;
+SELECT * FROM rw_view2;
+SELECT * FROM rw_view2 FOR UPDATE;  -- not allowed
+UPDATE rw_view2 SET b = 'bar' WHERE a = 1;  -- not allowed
+
+RESET SESSION AUTHORIZATION;
+GRANT UPDATE ON base_tbl TO regress_view_user1;
+
+SET SESSION AUTHORIZATION regress_view_user1;
+SELECT * FROM rw_view1;
+SELECT * FROM rw_view1 FOR UPDATE;
+UPDATE rw_view1 SET b = 'foo' WHERE a = 1;
+
+SET SESSION AUTHORIZATION regress_view_user2;
+SELECT * FROM rw_view2;
+SELECT * FROM rw_view2 FOR UPDATE;  -- not allowed
+UPDATE rw_view2 SET b = 'bar' WHERE a = 1;  -- not allowed
+
+SET SESSION AUTHORIZATION regress_view_user1;
+GRANT UPDATE ON rw_view1 TO regress_view_user2;
+
+SET SESSION AUTHORIZATION regress_view_user2;
+SELECT * FROM rw_view2;
+SELECT * FROM rw_view2 FOR UPDATE;
+UPDATE rw_view2 SET b = 'bar' WHERE a = 1;
+
+RESET SESSION AUTHORIZATION;
+REVOKE UPDATE ON base_tbl FROM regress_view_user1;
+
+SET SESSION AUTHORIZATION regress_view_user1;
+SELECT * FROM rw_view1;
+SELECT * FROM rw_view1 FOR UPDATE;  -- not allowed
+UPDATE rw_view1 SET b = 'foo' WHERE a = 1;  -- not allowed
+
+SET SESSION AUTHORIZATION regress_view_user2;
+SELECT * FROM rw_view2;
+SELECT * FROM rw_view2 FOR UPDATE;  -- not allowed
+UPDATE rw_view2 SET b = 'bar' WHERE a = 1;  -- not allowed
+
+RESET SESSION AUTHORIZATION;
+
+DROP TABLE base_tbl CASCADE;
+
 DROP USER regress_view_user1;
 DROP USER regress_view_user2;