Re: Overcoming SELECT ... FOR UPDATE permission restrictions

Alexander Lakhin <exclusion@gmail.com> writes:
> Can you please explain, is this a bug or intended behaviour?

I'd say it's a bug.  The permissions restriction should apply even with
the intermediate view.

After some rooting around, it seems like this can be blamed on wrong
order-of-operations in ApplyRetrieveRule().  It recursively expands
sub-views, then moves the permissions bits on the RELATION RTE for the
view being expanded to the view's "OLD" RTE entry, then (if the view
is selected FOR UPDATE) applies markQueryForLocking which recursively
marks relations referenced by the view as FOR UPDATE.

markQueryForLocking knows that it should avoid touching the "OLD" and
"NEW" entries in views, because they are treated specially.
Unfortunately, that means we never mark sub-views as requiring a FOR
UPDATE permission check; since we already expanded them, their RELATION
RTEs aren't in the active jointree anymore, and we skip the OLD entry
which is now the active entry for the sub-view's permissions check.

We can fix this just by switching the order of operations so that
markQueryForLocking is applied before recursive expansion.  That way,
by the time we go to expand a sub-view, its RELATION RTE has already
been marked with any needed UPDATE permission bit, and that's correctly
moved into the view's OLD entry, and you get the expected failure:

regression=> SELECT datid, datname FROM pgsd FOR UPDATE;
ERROR:  permission denied for view pg_stat_database

(Note that complaining about pg_stat_database is the correct thing; the
pgsd owner's lack of UPDATE on that view is the missing permission.)

It looks to me like we could dispense with the forUpdatePushedDown
argument to ApplyRetrieveRule altogether, because with this approach
a sub-view should always have a parse rowmark already by the time we
try to expand it.  I haven't tested that simplification though.

I haven't included a regression test case in this patch, but Alexander's
example can easily be converted into one.

Although this is arguably a security bug, I'm not sure we should
back-patch it.  The consequences seem relatively minor, and the
behavioral change carries a significant risk of breaking applications
that worked as-intended up to now.  Thoughts?

            regards, tom lane

diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 88140bc..4ce50f3 100644
*** a/src/backend/rewrite/rewriteHandler.c
--- b/src/backend/rewrite/rewriteHandler.c
*************** ApplyRetrieveRule(Query *parsetree,
*** 1560,1566 ****
--- 1560,1584 ----
      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 this routine's action below.
       */
      rule_action = fireRIRrules(rule_action, activeRIRs, forUpdatePushedDown);

*************** ApplyRetrieveRule(Query *parsetree,
*** 1594,1611 ****
      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;
  }

--- 1612,1617 ----