BUG #18387: Erroneous permission checks and/or misleading error messages with refresh materialized view

On 11/03/2024 22:10, PG Bug reporting form wrote:

Reproducer log (starting from interesting part):
test=# REFRESH MATERIALIZED VIEW CONCURRENTLY test_mv;
REFRESH MATERIALIZED VIEW
test=# alter materialized view test_mv owner to test_role;
ALTER MATERIALIZED VIEW
test=# REFRESH MATERIALIZED VIEW CONCURRENTLY test_mv;
ERROR: permission denied for table test
--what??? N1

--check that im not hallucinating
test=# select * from test;
val
-----
1
(1 row)

So far, this is working correctly. REFRESH MATERIALIZED VIEW runs with
the permissions of the materialized view's owner. In this case, the
owner is 'test_role', which doesn't have select permission on the table.

test=# revoke temporary on database test from public;
REVOKE
test=# \c test
You are now connected to database "test" as user "postgres".
test=# REFRESH MATERIALIZED VIEW CONCURRENTLY test_mv;
ERROR: permission denied to create temporary tables in database "test"
--what??? N2

That's interesting. REFRESH MATERIALIZED VIEW CONCURRENTLY uses
temporary tables internally, which fails if the user doesn't have
permissions to create temporary tables.

I guess we need to allow creating such internal temporary tables,
despite the missing permission. That'll need some careful analysis to
make sure we don't accidentally allow creating other temporary tables...

--
Heikki Linnakangas
Neon (https://neon.tech)

On Tue, Mar 12, 2024 at 01:02:16AM +0200, Heikki Linnakangas wrote:

On 11/03/2024 22:10, PG Bug reporting form wrote:

Reproducer log (starting from interesting part):
test=# REFRESH MATERIALIZED VIEW CONCURRENTLY test_mv;
REFRESH MATERIALIZED VIEW
test=# alter materialized view test_mv owner to test_role;
ALTER MATERIALIZED VIEW
test=# REFRESH MATERIALIZED VIEW CONCURRENTLY test_mv;
ERROR: permission denied for table test
--what??? N1

--check that im not hallucinating
test=# select * from test;
val
-----
1
(1 row)

So far, this is working correctly. REFRESH MATERIALIZED VIEW runs with the
permissions of the materialized view's owner. In this case, the owner is
'test_role', which doesn't have select permission on the table.

Can we do a better job of suggesting the cause of the failure?

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

Only you can decide what is important to you.

On Tue, 2024-03-12 at 01:02 +0200, Heikki Linnakangas wrote:

test=# \c test

You are now connected to database "test" as user "postgres".
test=# REFRESH MATERIALIZED VIEW CONCURRENTLY test_mv;
ERROR:  permission denied to create temporary tables in database "test"
--what??? N2

That's interesting. REFRESH MATERIALIZED VIEW CONCURRENTLY uses
temporary tables internally, which fails if the user doesn't have
permissions to create temporary tables.

I guess we need to allow creating such internal temporary tables,
despite the missing permission. That'll need some careful analysis to
make sure we don't accidentally allow creating other temporary tables...

Wouldn't it be sufficient to document that fact, perhaps add an
error hint and require the MV owner to have TEMP on the database?

That's not an outrageous requirement, and it couldn't open any
security back doors.

Yours,
Laurenz Albe

test=# REFRESH MATERIALIZED VIEW CONCURRENTLY test_mv;
ERROR: permission denied for table test
--what??? N1

--check that im not hallucinating
test=# select * from test;
val
-----
1
(1 row)

So far, this is working correctly. REFRESH MATERIALIZED VIEW runs with
the permissions of the materialized view's owner. In this case, the
owner is 'test_role', which doesn't have select permission on the table.

This decision led to a strange (and only one known to me) case when a
superuser cannot do something in the database.
(so far I have yet to see any other possible scenario when a command run by
superuser fails with permission error).

May I suggest a change to always allow superuser run REFRESH MATERIALIZED
VIEW (may be via set role or similar mechanics)?

Without that I think it's possible build a case of the database which could
be dumped but cannot be restored without errors
(restore from MV owner cannot be done because dump contains create
extension (for a sample) and restore from superuser cannot be done because
refresh MV permission check).

--
Maxim Boguk
Senior Postgresql DBA

Phone UA: +380 99 143 0000
Phone AU: +61 45 218 5678

On Tue, 2024-03-12 at 12:40 +0200, Maxim Boguk wrote:

May I suggest a change to always allow superuser run
REFRESH MATERIALIZED VIEW (may be via set role or similar mechanics)?

If the query ran with superuser permissions, that would be
a security problem:

CREATE TABLE log (t text);

CREATE FUNCTION f() RETURNS integer LANGUAGE sql
AS 'INSERT INTO log VALUES (''x''); SELECT 42';

CREATE MATERIALIZED VIEW v AS SELECT f();

Now imagine you create a malicious trigger on "log" and
get a superuser to refresh the materialized view.

I don't see why it should be a problem if a superuser gets
"permission denied" in such a case. They can also get it if
they call a SECURITY DEFINER function owned by a non-superuser.

Yours,
Laurenz Albe

I guess we need to allow creating such internal temporary tables,
despite the missing permission. That'll need some careful analysis to
make sure we don't accidentally allow creating other temporary tables...

Wouldn't it be sufficient to document that fact, perhaps add an error hint and require the MV owner to have TEMP on the database?

That's not an outrageous requirement, and it couldn't open any security back doors.

Agree. We already have to create a new user (well, that’s what I do) for MV's anyway for the REFRESH by owner only, it would not be a burden to adjust that ROLE's settings at time of creation. The doco is completely clear about MV owner, we can just add to that note to make sure CREATE permission too.

Laurenz Albe <laurenz.albe@cybertec.at> writes:

On Tue, 2024-03-12 at 01:02 +0200, Heikki Linnakangas wrote:

I guess we need to allow creating such internal temporary tables,
despite the missing permission. That'll need some careful analysis to
make sure we don't accidentally allow creating other temporary tables...

Wouldn't it be sufficient to document that fact, perhaps add an
error hint and require the MV owner to have TEMP on the database?

+1. I don't see a need for this to be a low-permissions feature.

regards, tom lane

On Tue, Mar 12, 2024 at 01:22:33PM +0100, Laurenz Albe wrote:

On Tue, 2024-03-12 at 12:40 +0200, Maxim Boguk wrote:

May I suggest a change to always allow superuser run
REFRESH MATERIALIZED VIEW (may be via set role or similar mechanics)?

If the query ran with superuser permissions, that would be
a security problem:

CREATE TABLE log (t text);

CREATE FUNCTION f() RETURNS integer LANGUAGE sql
AS 'INSERT INTO log VALUES (''x''); SELECT 42';

CREATE MATERIALIZED VIEW v AS SELECT f();

Now imagine you create a malicious trigger on "log" and
get a superuser to refresh the materialized view.

I don't see why it should be a problem if a superuser gets
"permission denied" in such a case. They can also get it if
they call a SECURITY DEFINER function owned by a non-superuser.

Can we improve the error that superusers get so they realize how to fix
it?

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

Only you can decide what is important to you.

Bruce Momjian <bruce@momjian.us> writes:

On Tue, Mar 12, 2024 at 01:22:33PM +0100, Laurenz Albe wrote:

I don't see why it should be a problem if a superuser gets
"permission denied" in such a case. They can also get it if
they call a SECURITY DEFINER function owned by a non-superuser.

Can we improve the error that superusers get so they realize how to fix
it?

I think there's been a policy of being minimalistic on
permission-denied errors to avoid giving away security information,
but I'm not sure how much sense that really makes. We already show
the specific object that didn't have permissions. I think it would
be good for these errors to also mention the specific role whose
permissions were checked. Perhaps also show the specific privileges
that were missing --- although it might be hard to do that in a
non-confusing way for complicated cases, such as queries that are
valid if you have either table- or column-level permissions.

If we just add the role I'd envision

ERROR: permission denied to role "foo" for [object]

although with any more detail that would get too long.
Another way could be

ERROR: permission denied for [object]
DETAIL: Role "foo" lacks permission [permission].

Mentioning the role that was checked should address the concern
of "I'm a superuser, why did I get this error?". However,
fixing it requires knowing which privilege to grant. I'm not
sure if that's always obvious.

regards, tom lane

On Wed, Mar 13, 2024 at 02:32:55PM -0400, Tom Lane wrote:

I think there's been a policy of being minimalistic on
permission-denied errors to avoid giving away security information,
but I'm not sure how much sense that really makes. We already show
the specific object that didn't have permissions. I think it would
be good for these errors to also mention the specific role whose
permissions were checked. Perhaps also show the specific privileges
that were missing --- although it might be hard to do that in a
non-confusing way for complicated cases, such as queries that are
valid if you have either table- or column-level permissions.

If we just add the role I'd envision

ERROR: permission denied to role "foo" for [object]

although with any more detail that would get too long.
Another way could be

ERROR: permission denied for [object]
DETAIL: Role "foo" lacks permission [permission].

Mentioning the role that was checked should address the concern
of "I'm a superuser, why did I get this error?". However,
fixing it requires knowing which privilege to grant. I'm not
sure if that's always obvious.

If we don't want to expand the error, and I can see why we might not
want to, giving the detailed error only for the superuser would be safe,
I think, since they are already the superuser.

Personal note: my son Matthew got this error when using photoview
software, and I was confused why the superuser was getting a permission
error.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

Only you can decide what is important to you.