database/postgres
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2025-07-14 08:21:07 +03:00
Code Activity

Fix enforcement of SELECT FOR UPDATE permissions with nested views.

Browse Source 
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
This commit is contained in:
Tom Lane
2018-04-14 15:38:09 -04:00
parent 2a67d6440d
commit 50c6bb0224
3 changed files with 219 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files

31
src/backend/rewrite/rewriteHandler.c
View File

@ -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;
}