Обсуждение: pg_init_privs corruption.
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';
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)
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.
[1] yoext https://github.com/reshke/yoext/
Вложения
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
> 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
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
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
Вложения
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
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