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

The following bug has been logged on the website:

Bug reference:      18387
Logged by:          Maxim Boguk
Email address:      maxim.boguk@gmail.com
PostgreSQL version: 16.2
Operating system:   Linux
Description:

Hi,

There are weird behavior with permission checks for refresh materialized
view for superuser.
Reproducer script (run from postgres):

create role test_role;
create database test with owner postgres;
\c test
create table test as select 1 val;
create materialized view test_mv as select * from test;
REFRESH MATERIALIZED VIEW test_mv;
create unique index test_mv_pk on test_mv(val);
REFRESH MATERIALIZED VIEW CONCURRENTLY test_mv;
alter materialized view test_mv owner to test_role;
REFRESH MATERIALIZED VIEW CONCURRENTLY test_mv;
select * from test;
revoke temporary on database test from public;
\c test
REFRESH MATERIALIZED VIEW CONCURRENTLY test_mv;
REFRESH MATERIALIZED VIEW test_mv;

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)

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

test=# REFRESH MATERIALIZED VIEW test_mv;
ERROR:  permission denied for table test
--without CONCURRENTLY wrong behavior too

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

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
wouldnot be a burden to adjust that ROLE's settings at time of creation.  The doco is completely clear about MV owner,
wecan 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.