Postgres Pro
Downloads
Home   >   mailing lists

Thread: pg_init_privs corruption.

pg_init_privs corruption.

From
Kirill Reshke
Date:
Hi hackers!

Recently we faced a problem with one of our production clusters. Problem was with pg_upgrade,
the reason was an invalid pg_dump of cluster schema. in pg_dump sql there was strange records like

REVOKE SELECT,INSERT,DELETE,UPDATE ON TABLE *relation* FROM "144841";

but there is no role "144841"
We did dig in, and it turns out that 144841 was OID of previously-deleted role.

I have reproduced issue using simple test extension yoext(1).

SQL script:

create role user1;
ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT select ON TABLES TO user1;
create extension yoext;
drop owned by user1;
select * from pg_init_privs  where privtype = 'e';
drop role user1;
select * from pg_init_privs  where privtype = 'e';

result of execution (executed on fest master from commit 17feb6a566b77bf62ca453dec215adcc71755c20):

psql (16devel)
Type "help" for help.

postgres=#
postgres=#
postgres=# create role user1;
CREATE ROLE
postgres=# ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT select ON TABLES TO user1;
ALTER DEFAULT PRIVILEGES
postgres=# create extension yobaext ;
CREATE EXTENSION
postgres=# drop owned by user1;
DROP OWNED
postgres=# select * from pg_init_privs  where privtype = 'e';
 objoid | classoid | objsubid | privtype |                     initprivs
--------+----------+----------+----------+---------------------------------------------------
  16387 |     1259 |        0 | e        | {reshke=arwdDxtm/reshke,user1=r/reshke,=r/reshke}
(1 row)

postgres=# drop role user1;
DROP ROLE
postgres=# select * from pg_init_privs  where privtype = 'e';
 objoid | classoid | objsubid | privtype |                     initprivs
--------+----------+----------+----------+---------------------------------------------------
  16387 |     1259 |        0 | e        | {reshke=arwdDxtm/reshke,16384=r/reshke,=r/reshke}
(1 row)


As you can see, after drop role there is invalid records in pg_init_privs system relation. After this, pg_dump generate sql statements, some of which are based on content of pg_init_privs, resulting in invalid dump.

PFA fix.

The idea of fix is simply drop records from pg_init_privs while dropping role.
Records with grantor of grantee equal to oid of dropped role will erase. after that, pg_dump works ok.

Implementation comment: i failed to find proper way to alloc acl array, so defined some acl.c internal function `allocacl` in header. Need to improve this somehow.

Attachment

Re: pg_init_privs corruption.

From
Tom Lane
Date:
Kirill Reshke <reshke@double.cloud> writes:
> As you can see, after drop role there is invalid records in pg_init_privs
> system relation. After this, pg_dump generate sql statements, some of which
> are based on content of pg_init_privs, resulting in invalid dump.

Ugh.

> PFA fix.

I don't think this is anywhere near usable as-is, because it only
accounts for pg_init_privs entries in the current database.  We need
to handle these records in the DROP OWNED BY mechanism instead, and
also ensure there are shared-dependency entries for them so that the
role can't be dropped until the entries are gone in all DBs.  The real
problem may be less that DROP is doing the wrong thing, and more that
creation of the pg_init_privs entries neglects to record a dependency.

            regards, tom lane

RE: pg_init_privs corruption.

From
Floris Van Nee
Date:
> Kirill Reshke <reshke@double.cloud> writes:
> > As you can see, after drop role there is invalid records in
> > pg_init_privs system relation. After this, pg_dump generate sql
> > statements, some of which are based on content of pg_init_privs, resulting
> in invalid dump.
>

This is as far as I can see the same case as what I reported a few years ago here:
https://www.postgresql.org/message-id/flat/1574068566573.13088%40Optiver.com#488bd647ce6f5d2c92764673a7c58289
There was a discussion with some options, but no fix back then.

-Floris

Re: pg_init_privs corruption.

From
Tom Lane
Date:
Floris Van Nee <florisvannee@Optiver.com> writes:
> This is as far as I can see the same case as what I reported a few years ago here:
https://www.postgresql.org/message-id/flat/1574068566573.13088%40Optiver.com#488bd647ce6f5d2c92764673a7c58289
> There was a discussion with some options, but no fix back then.

Hmm, so Stephen was opining that the extension's objects shouldn't
have gotten these privs attached in the first place.  I'm not
quite convinced about that one way or the other, but if you buy it
then maybe this situation is unreachable once we fix that.  I'm
not sure though.  It's still clear that we are making ACL entries
that aren't reflected in pg_shdepend, and that seems bad.

            regards, tom lane

Re: pg_init_privs corruption.

From
Stephen Frost
Date:
Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Floris Van Nee <florisvannee@Optiver.com> writes:
> > This is as far as I can see the same case as what I reported a few years ago here:
https://www.postgresql.org/message-id/flat/1574068566573.13088%40Optiver.com#488bd647ce6f5d2c92764673a7c58289
> > There was a discussion with some options, but no fix back then.
>
> Hmm, so Stephen was opining that the extension's objects shouldn't
> have gotten these privs attached in the first place.  I'm not
> quite convinced about that one way or the other, but if you buy it
> then maybe this situation is unreachable once we fix that.  I'm
> not sure though.  It's still clear that we are making ACL entries
> that aren't reflected in pg_shdepend, and that seems bad.

Would be great to get some other thoughts on this then, perhaps, as it's
clearly not good as-is either.

I mentioned in that other thread that recording the dependency should be
done but that it's an independent issue and I do still generally feel
that way, so I guess we're all mostly in agreement that the dependency
should get recorded and perhaps we can just go do that.

I don't see any cases of it currently, but I do still worry, as I also
mentioned in the prior thread, that by allowing DEFAULT PRIVILEGES to
impact extension objects that we could end up with a security issue.
Specifically, if a user sets up their schema like:

ALTER DEFAULT PRIVILEGES ... GRANT EXECUTE ON FUNCTIONS TO me;

and then creates an extension which is marked as 'trusted':

CREATE EXTENSION abc;

where that extension manages function access through the GRANT system
(as many do, eg: pg_stat_statements which does:

REVOKE ALL ON FUNCTION pg_stat_statements_reset() FROM PUBLIC;
)

That the user then will have EXECUTE rights on that function which they
really shouldn't have.

Thanks,

Stephen
Attachment

Re: pg_init_privs corruption.

From
Greg Stark
Date:
On Fri, 17 Feb 2023 at 15:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Hmm, so Stephen was opining that the extension's objects shouldn't
> have gotten these privs attached in the first place.  I'm not
> quite convinced about that one way or the other, but if you buy it
> then maybe this situation is unreachable once we fix that.

Well pg_dump might still have to deal with it even if it's unreachable
in new databases (or rather schemas with extensions newly added... it
might be hard to explain what cases are affected).

Alternately it'll be a note at the top of every point release pointing
at a note explaining how to run a script to fix your schemas.

-- 
greg

Re: pg_init_privs corruption.

From
Robert Haas
Date:
On Fri, Feb 17, 2023 at 3:38 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Floris Van Nee <florisvannee@Optiver.com> writes:
> > This is as far as I can see the same case as what I reported a few years ago here:
https://www.postgresql.org/message-id/flat/1574068566573.13088%40Optiver.com#488bd647ce6f5d2c92764673a7c58289
> > There was a discussion with some options, but no fix back then.
>
> Hmm, so Stephen was opining that the extension's objects shouldn't
> have gotten these privs attached in the first place.  I'm not
> quite convinced about that one way or the other, but if you buy it
> then maybe this situation is unreachable once we fix that.  I'm
> not sure though.  It's still clear that we are making ACL entries
> that aren't reflected in pg_shdepend, and that seems bad.

Yep. I think you have the right idea how to fix this. Making extension
creation somehow not subject to the same rules about default
privileges as everything else doesn't seem like either a good idea or
a real fix for this problem.

--
Robert Haas
EDB: http://www.enterprisedb.com