Re: Overcoming SELECT ... FOR UPDATE permission restrictions

[Tom Lane]

Michael Paquier  writes:
> I also don't quite understand the argument of application relying on
> this behavior.  If they do, that's wrong anyway, so the risk of
> operation disruptions for shared environments would matter more in my
> opinion.

I'm not totally sure about that.  If you suppose that the only purpose
of doing SELECT FOR UPDATE is to clear the way for a subsequent UPDATE,
then people who are using it would certainly have had to grant the
necessary UPDATE permission to let the second command go through.
But I'm not 100% sure that that's the only use-case.  S-F-U could be
useful strictly for mutual-exclusion perhaps.  Or maybe your application
does S-F-U to get row locks, then does DELETE rather than UPDATE.

Still, it seems unlikely that somebody would be doing those sorts of
things through two levels of view.  So maybe the set of applications
that would get broken is vanishingly small.

regards, tom lane

Re: Overcoming SELECT ... FOR UPDATE permission restrictions

[Michael Paquier]

On Mon, Apr 16, 2018 at 08:12:45PM +0300, Alexander Lakhin wrote:
> The worst scenario (with the current system views) I could think of is:
> user=> CREATE VIEW pgg AS SELECT * FROM pg_group;
> BEGIN TRANSACTION; SELECT * FROM pgg FOR UPDATE; SELECT pg_sleep(60);
> ROLLBACK;
> and the parallel operation:
> admin=> DROP ROLE testrole;
> hangs for one minute.
> But admin can observer the locks and kill the offending backend so it's
> hardly a critical issue.

No need to use pg_sleep(), you could just as well start a full
transaction block and let the wait happen forever.

The main point is that row-level locks are not strong enough to prevent
read-only operations, so critical operations like authentication can
still be triggered.  This can be used though to disrupt the activity of
all DDL operations if you take a lock on them, which sucks.  So while
that's a nuisance, it is always possible to counter it.

I also don't quite understand the argument of application relying on
this behavior.  If they do, that's wrong anyway, so the risk of
operation disruptions for shared environments would matter more in my
opinion.
--
Michael


signature.asc
Description: PGP signature

Re: Overcoming SELECT ... FOR UPDATE permission restrictions

[Alexander Lakhin]

13.04.2018 18:55, Tom Lane wrote:

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?

The worst scenario (with the current system views) I could think of is:
user=> CREATE VIEW pgg AS SELECT * FROM pg_group;
BEGIN TRANSACTION; SELECT * FROM pgg FOR UPDATE; SELECT pg_sleep(60); 
ROLLBACK;

and the parallel operation:
admin=> DROP ROLE testrole;
hangs for one minute.
But admin can observer the locks and kill the offending backend so it's 
hardly a critical issue.

Re: Overcoming SELECT ... FOR UPDATE permission restrictions

[Tom Lane]

Alexander Lakhin  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