From 50c6bb022475bd9ad9c73e3b68b5152db5163b22 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 14 Apr 2018 15:38:09 -0400 Subject: [PATCH] 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 --- src/backend/rewrite/rewriteHandler.c | 31 +++-- src/test/regress/expected/updatable_views.out | 124 ++++++++++++++++++ src/test/regress/sql/updatable_views.sql | 76 +++++++++++ 3 files changed, 219 insertions(+), 12 deletions(-) diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index 88140bc687..981a233d10 100644 --- 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 aedf8ce404..b34bab4b29 100644 --- 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 6a9005243b..a7786b26e9 100644 --- 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; -- 2.40.0