Horton

Thread

Commits

  1. Simplify view-expansion code in rewriteHandler.c.

  2. Fix enforcement of SELECT FOR UPDATE permissions with nested views.

  1. Overcoming SELECT ... FOR UPDATE permission restrictions

    Alexander Law <exclusion@gmail.com> — 2018-04-13T04:10:01Z

    Hello hackers,

Can you please explain, is this a bug or intended behaviour?

Running as non-privileged user:

postgres=> SELECT datid, datname FROM pg_stat_database FOR UPDATE;
ERROR: permission denied for view pg_stat_database (SQLState: 42501)

But:

postgres=> CREATE VIEW pgsd AS SELECT * FROM pg_stat_database; SELECT 
datid, datname FROM pgsd FOR UPDATE;
CREATE VIEW
  datid |  datname
-------+-----------
  13021 | postgres
      1 | template1
  13020 | template0
(3 rows)


(And lock is really held by the second SELECT.)

Best regards,

------
Alexander Lakhin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

  2. Re: Overcoming SELECT ... FOR UPDATE permission restrictions

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-04-13T15:55:22Z

    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

  3. Re: Overcoming SELECT ... FOR UPDATE permission restrictions

    Alexander Law <exclusion@gmail.com> — 2018-04-16T17:12:45Z

    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.

  4. Re: Overcoming SELECT ... FOR UPDATE permission restrictions

    Michael Paquier <michael@paquier.xyz> — 2018-04-17T02:18:36Z

    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

  5. Re: Overcoming SELECT ... FOR UPDATE permission restrictions

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-04-17T02:49:13Z

    Michael Paquier <michael@paquier.xyz> 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