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';
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.
[1]: yoext https://github.com/reshke/yoext/
Attachments:
v1-0001-Fix-pg_init_prevs-corruption.patchapplication/octet-stream; name=v1-0001-Fix-pg_init_prevs-corruption.patchDownload
From 4af27a0a9c5eacab7e580fc015caec1e19586111 Mon Sep 17 00:00:00 2001
From: reshke kirill <reshke@double.cloud>
Date: Fri, 17 Feb 2023 16:05:37 +0000
Subject: [PATCH v1] Fix pg_init_prevs corruption.
Drop some acl items from initprivs in pg_init_prevs
while DROP ROLE.
---
src/backend/catalog/dependency.c | 90 ++++++++++++++++++++++++++++++++
src/backend/commands/user.c | 5 ++
src/backend/utils/adt/acl.c | 3 +-
src/include/catalog/dependency.h | 2 +
src/include/utils/acl.h | 2 +
5 files changed, 100 insertions(+), 2 deletions(-)
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index f8a136ba0a..6bdfab5e06 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -3009,3 +3009,93 @@ DeleteInitPrivs(const ObjectAddress *object)
table_close(relation, RowExclusiveLock);
}
+
+
+
+#define ARRNELEMS(x) ArrayGetNItems( ARR_NDIM(x), ARR_DIMS(x))
+
+/*
+ * modify pg_init_prevs ACL for extension objects on role delete
+ */
+void
+DeleteInitPrivsRefs(Oid roleoid)
+{
+ Relation relation;
+ ScanKeyData key[1];
+ SysScanDesc scan;
+ HeapTuple oldtuple;
+ HeapTuple newtuple;
+ Datum initprivs;
+ AclItem * acls;
+ Acl *iniprivsacl;
+ Acl *new_acl;
+ bool initprvis_isnull;
+ bool found;
+ int dim;
+ int new_acl_sz;
+
+ Datum values[Natts_pg_init_privs] = {0};
+ bool nulls[Natts_pg_init_privs] = {0};
+ bool replaces[Natts_pg_init_privs] = {0};
+
+
+ relation = table_open(InitPrivsRelationId, RowExclusiveLock);
+
+ ScanKeyInit(&key[0],
+ Anum_pg_init_privs_privtype,
+ BTEqualStrategyNumber, F_CHAREQ,
+ INITPRIVS_EXTENSION);
+
+ scan = systable_beginscan(relation, InitPrivsObjIndexId, false,
+ NULL, 1, key);
+
+ while (HeapTupleIsValid(oldtuple = systable_getnext(scan))) {
+ initprivs = heap_getattr(oldtuple, Anum_pg_init_privs_initprivs, relation->rd_att, &initprvis_isnull);
+
+ if (initprvis_isnull) {
+ continue;
+ }
+
+ iniprivsacl = DatumGetAclP(initprivs);
+ found = false;
+
+ acls = (AclItem *) ARR_DATA_PTR(iniprivsacl);
+ dim = ARRNELEMS(iniprivsacl);
+ new_acl_sz = 0;
+
+ for (int i = 0; i < dim; ++ i) {
+ if (acls[i].ai_grantee == roleoid || acls[i].ai_grantor == roleoid) {
+ found = true;
+ continue;
+ }
+ new_acl_sz++;
+ }
+
+ if (!found) {
+ continue;
+ }
+
+ new_acl = allocacl(new_acl_sz);
+
+ new_acl_sz = 0;
+
+ for (int i = 0; i < dim; ++ i) {
+ if (acls[i].ai_grantee == roleoid || acls[i].ai_grantor == roleoid) {
+ continue;
+ }
+ ((AclItem *) ARR_DATA_PTR(new_acl))[new_acl_sz++] = acls[i];
+ }
+
+ replaces[Anum_pg_init_privs_initprivs - 1] = true;
+ values[Anum_pg_init_privs_initprivs - 1] = PointerGetDatum(new_acl);
+
+ newtuple = heap_modify_tuple(oldtuple, RelationGetDescr(relation),
+ values, nulls, replaces);
+
+ CatalogTupleUpdate(relation, &newtuple->t_self, newtuple);
+ }
+
+ systable_endscan(scan);
+
+ table_close(relation, RowExclusiveLock);
+}
\ No newline at end of file
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 3a92e930c0..49616439ef 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -1283,6 +1283,11 @@ DropRole(DropRoleStmt *stmt)
* Remove settings for this role.
*/
DropSetting(InvalidOid, roleid);
+
+ /*
+ * Remove pg_init_privs enries for that role
+ */
+ DeleteInitPrivsRefs(roleid);
}
/*
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index 8f7522d103..57485bb4dc 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -83,7 +83,6 @@ static uint32 cached_db_hash;
static const char *getid(const char *s, char *n, Node *escontext);
static void putid(char *p, const char *s);
-static Acl *allocacl(int n);
static void check_acl(const Acl *acl);
static const char *aclparse(const char *s, AclItem *aip, Node *escontext);
static bool aclitem_match(const AclItem *a1, const AclItem *a2);
@@ -399,7 +398,7 @@ aclparse(const char *s, AclItem *aip, Node *escontext)
* RETURNS:
* the new Acl
*/
-static Acl *
+Acl *
allocacl(int n)
{
Acl *new_acl;
diff --git a/src/include/catalog/dependency.h b/src/include/catalog/dependency.h
index ffd5e9dc82..d2710968ca 100644
--- a/src/include/catalog/dependency.h
+++ b/src/include/catalog/dependency.h
@@ -267,4 +267,6 @@ extern void shdepDropOwned(List *roleids, DropBehavior behavior);
extern void shdepReassignOwned(List *roleids, Oid newrole);
+extern void DeleteInitPrivsRefs(Oid roleoid);
+
#endif /* DEPENDENCY_H */
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index f8e1238fa2..dc8e3227a3 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -276,4 +276,6 @@ extern bool object_ownercheck(Oid classid, Oid objectid, Oid roleid);
extern bool has_createrole_privilege(Oid roleid);
extern bool has_bypassrls_privilege(Oid roleid);
+extern Acl * allocacl(int n);
+
#endif /* ACL_H */
--
2.25.1
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, resultingin invalid dump.
This is as far as I can see the same case as what I reported a few years ago here: /messages/by-id/1574068566573.13088@Optiver.com
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: /messages/by-id/1574068566573.13088@Optiver.com
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: /messages/by-id/1574068566573.13088@Optiver.com
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: /messages/by-id/1574068566573.13088@Optiver.com
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