From ab49ce7c3414ac19e4afb386d7843ce2d2fb8bda Mon Sep 17 00:00:00 2001 From: Noah Misch Date: Mon, 9 May 2022 08:35:08 -0700 Subject: Make relation-enumerating operations be security-restricted operations. When a feature enumerates relations and runs functions associated with all found relations, the feature's user shall not need to trust every user having permission to create objects. BRIN-specific functionality in autovacuum neglected to account for this, as did pg_amcheck and CLUSTER. An attacker having permission to create non-temp objects in at least one schema could execute arbitrary SQL functions under the identity of the bootstrap superuser. CREATE INDEX (not a relation-enumerating operation) and REINDEX protected themselves too late. This change extends to the non-enumerating amcheck interface. Back-patch to v10 (all supported versions). Sergey Shinderuk, reviewed (in earlier versions) by Alexander Lakhin. Reported by Alexander Lakhin. Security: CVE-2022-1552 --- src/backend/access/brin/brin.c | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) (limited to 'src/backend/access') diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index ccc9fa0959a..685899fc24c 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -1007,6 +1007,9 @@ brin_summarize_range(PG_FUNCTION_ARGS) Oid heapoid; Relation indexRel; Relation heapRel; + Oid save_userid; + int save_sec_context; + int save_nestlevel; double numSummarized = 0; if (RecoveryInProgress()) @@ -1033,7 +1036,22 @@ brin_summarize_range(PG_FUNCTION_ARGS) */ heapoid = IndexGetRelation(indexoid, true); if (OidIsValid(heapoid)) + { heapRel = table_open(heapoid, ShareUpdateExclusiveLock); + + /* + * Autovacuum calls us. For its benefit, switch to the table owner's + * userid, so that any index functions are run as that user. Also + * lock down security-restricted operations and arrange to make GUC + * variable changes local to this command. This is harmless, albeit + * unnecessary, when called from SQL, because we fail shortly if the + * user does not own the index. + */ + GetUserIdAndSecContext(&save_userid, &save_sec_context); + SetUserIdAndSecContext(heapRel->rd_rel->relowner, + save_sec_context | SECURITY_RESTRICTED_OPERATION); + save_nestlevel = NewGUCNestLevel(); + } else heapRel = NULL; @@ -1048,7 +1066,7 @@ brin_summarize_range(PG_FUNCTION_ARGS) RelationGetRelationName(indexRel)))); /* User must own the index (comparable to privileges needed for VACUUM) */ - if (!pg_class_ownercheck(indexoid, GetUserId())) + if (heapRel != NULL && !pg_class_ownercheck(indexoid, save_userid)) aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_INDEX, RelationGetRelationName(indexRel)); @@ -1066,6 +1084,12 @@ brin_summarize_range(PG_FUNCTION_ARGS) /* OK, do it */ brinsummarize(indexRel, heapRel, heapBlk, true, &numSummarized, NULL); + /* Roll back any GUC changes executed by index functions */ + AtEOXact_GUC(false, save_nestlevel); + + /* Restore userid and security context */ + SetUserIdAndSecContext(save_userid, save_sec_context); + relation_close(indexRel, ShareUpdateExclusiveLock); relation_close(heapRel, ShareUpdateExclusiveLock); @@ -1107,6 +1131,9 @@ brin_desummarize_range(PG_FUNCTION_ARGS) * passed indexoid isn't an index then IndexGetRelation() will fail. * Rather than emitting a not-very-helpful error message, postpone * complaining, expecting that the is-it-an-index test below will fail. + * + * Unlike brin_summarize_range(), autovacuum never calls this. Hence, we + * don't switch userid. */ heapoid = IndexGetRelation(indexoid, true); if (OidIsValid(heapoid)) -- cgit v1.2.3