agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Noah Misch <noah@leadboat.com>
Subject: [PATCH] Make relation-enumerating operations be security-restricted operations.
Date: Mon, 9 May 2022 08:35:08 -0700
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/catalog/index.c | 41 +++++++++++++++++++--------
src/backend/commands/cluster.c | 35 ++++++++++++++++++-----
src/backend/commands/indexcmds.c | 47 +++++++++++++++++++++++++++++--
src/backend/utils/init/miscinit.c | 24 +++++++++------
src/test/regress/expected/privileges.out | 35 +++++++++++++++++++++++
src/test/regress/sql/privileges.sql | 34 ++++++++++++++++++++++
6 files changed, 187 insertions(+), 29 deletions(-)
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -2743,7 +2743,17 @@
/* Open and lock the parent heap relation */
heapRelation = heap_open(heapId, ShareUpdateExclusiveLock);
- /* And the target index relation */
+
+ /*
+ * 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.
+ */
+ GetUserIdAndSecContext(&save_userid, &save_sec_context);
+ SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
+ save_sec_context | SECURITY_RESTRICTED_OPERATION);
+ save_nestlevel = NewGUCNestLevel();
+
indexRelation = index_open(indexId, RowExclusiveLock);
/*
@@ -2757,16 +2767,6 @@
indexInfo->ii_Concurrent = true;
/*
- * 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.
- */
- GetUserIdAndSecContext(&save_userid, &save_sec_context);
- SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
- save_sec_context | SECURITY_RESTRICTED_OPERATION);
- save_nestlevel = NewGUCNestLevel();
-
- /*
* Scan the index and gather up all the TIDs into a tuplesort object.
*/
ivinfo.index = indexRelation;
@@ -3178,6 +3178,9 @@
Relation iRel,
heapRelation;
Oid heapId;
+ Oid save_userid;
+ int save_sec_context;
+ int save_nestlevel;
IndexInfo *indexInfo;
volatile bool skipped_constraint = false;
@@ -3189,6 +3192,16 @@
heapRelation = heap_open(heapId, ShareLock);
/*
+ * 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.
+ */
+ GetUserIdAndSecContext(&save_userid, &save_sec_context);
+ SetUserIdAndSecContext(heapRelation->rd_rel->relowner,
+ save_sec_context | SECURITY_RESTRICTED_OPERATION);
+ save_nestlevel = NewGUCNestLevel();
+
+ /*
* Open the target index relation and get an exclusive lock on it, to
* ensure that no one else is touching this particular index.
*/
@@ -3324,6 +3337,12 @@
heap_close(pg_index, RowExclusiveLock);
}
+ /* 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);
+
/* Close rels, but keep locks */
index_close(iRel, NoLock);
heap_close(heapRelation, NoLock);
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -41,6 +41,7 @@
#include "storage/smgr.h"
#include "utils/acl.h"
#include "utils/fmgroids.h"
+#include "utils/guc.h"
#include "utils/inval.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
@@ -259,6 +260,9 @@
cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose)
{
Relation OldHeap;
+ Oid save_userid;
+ int save_sec_context;
+ int save_nestlevel;
/* Check for user-requested abort. */
CHECK_FOR_INTERRUPTS();
@@ -276,6 +280,16 @@
return;
/*
+ * 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.
+ */
+ GetUserIdAndSecContext(&save_userid, &save_sec_context);
+ SetUserIdAndSecContext(OldHeap->rd_rel->relowner,
+ save_sec_context | SECURITY_RESTRICTED_OPERATION);
+ save_nestlevel = NewGUCNestLevel();
+
+ /*
* Since we may open a new transaction for each relation, we have to check
* that the relation still is what we think it is.
*
@@ -289,10 +303,10 @@
Form_pg_index indexForm;
/* Check that the user still owns the relation */
- if (!pg_class_ownercheck(tableOid, GetUserId()))
+ if (!pg_class_ownercheck(tableOid, save_userid))
{
relation_close(OldHeap, AccessExclusiveLock);
- return;
+ goto out;
}
/*
@@ -306,7 +320,7 @@
if (RELATION_IS_OTHER_TEMP(OldHeap))
{
relation_close(OldHeap, AccessExclusiveLock);
- return;
+ goto out;
}
if (OidIsValid(indexOid))
@@ -317,7 +331,7 @@
if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(indexOid)))
{
relation_close(OldHeap, AccessExclusiveLock);
- return;
+ goto out;
}
/*
@@ -327,14 +341,14 @@
if (!HeapTupleIsValid(tuple)) /* probably can't happen */
{
relation_close(OldHeap, AccessExclusiveLock);
- return;
+ goto out;
}
indexForm = (Form_pg_index) GETSTRUCT(tuple);
if (!indexForm->indisclustered)
{
ReleaseSysCache(tuple);
relation_close(OldHeap, AccessExclusiveLock);
- return;
+ goto out;
}
ReleaseSysCache(tuple);
}
@@ -388,7 +402,7 @@
!RelationIsPopulated(OldHeap))
{
relation_close(OldHeap, AccessExclusiveLock);
- return;
+ goto out;
}
/*
@@ -403,6 +417,13 @@
rebuild_relation(OldHeap, indexOid, verbose);
/* NB: rebuild_relation does heap_close() on OldHeap */
+
+out:
+ /* 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);
}
/*
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -44,6 +44,7 @@
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/fmgroids.h"
+#include "utils/guc.h"
#include "utils/inval.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
@@ -329,8 +330,13 @@
LOCKTAG heaplocktag;
LOCKMODE lockmode;
Snapshot snapshot;
+ Oid root_save_userid;
+ int root_save_sec_context;
+ int root_save_nestlevel;
int i;
+ root_save_nestlevel = NewGUCNestLevel();
+
/*
* Force non-concurrent build on temporary relations, even if CONCURRENTLY
* was requested. Other backends can't access a temporary relation, so
@@ -371,6 +377,15 @@
lockmode = concurrent ? ShareUpdateExclusiveLock : ShareLock;
rel = heap_open(relationId, lockmode);
+ /*
+ * Switch to the table owner's userid, so that any index functions are run
+ * as that user. Also lock down security-restricted operations. We
+ * already arranged to make GUC variable changes local to this command.
+ */
+ GetUserIdAndSecContext(&root_save_userid, &root_save_sec_context);
+ SetUserIdAndSecContext(rel->rd_rel->relowner,
+ root_save_sec_context | SECURITY_RESTRICTED_OPERATION);
+
relationId = RelationGetRelid(rel);
namespaceId = RelationGetNamespace(rel);
@@ -412,7 +427,7 @@
{
AclResult aclresult;
- aclresult = pg_namespace_aclcheck(namespaceId, GetUserId(),
+ aclresult = pg_namespace_aclcheck(namespaceId, root_save_userid,
ACL_CREATE);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, ACL_KIND_NAMESPACE,
@@ -439,7 +454,7 @@
{
AclResult aclresult;
- aclresult = pg_tablespace_aclcheck(tablespaceId, GetUserId(),
+ aclresult = pg_tablespace_aclcheck(tablespaceId, root_save_userid,
ACL_CREATE);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, ACL_KIND_TABLESPACE,
@@ -622,11 +637,23 @@
skip_build || concurrent,
concurrent, !check_rights);
+ /*
+ * Roll back any GUC changes executed by index functions, and keep
+ * subsequent changes local to this command. It's barely possible that
+ * some index function changed a behavior-affecting GUC, e.g. xmloption,
+ * that affects subsequent steps.
+ */
+ AtEOXact_GUC(false, root_save_nestlevel);
+ root_save_nestlevel = NewGUCNestLevel();
+
/* Add any requested comment */
if (stmt->idxcomment != NULL)
CreateComments(indexRelationId, RelationRelationId, 0,
stmt->idxcomment);
+ AtEOXact_GUC(false, root_save_nestlevel);
+ SetUserIdAndSecContext(root_save_userid, root_save_sec_context);
+
if (!concurrent)
{
/* Close the heap and we're done, in the non-concurrent case */
@@ -705,6 +732,16 @@
/* Open and lock the parent heap relation */
rel = heap_openrv(stmt->relation, ShareUpdateExclusiveLock);
+ /*
+ * 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.
+ */
+ GetUserIdAndSecContext(&root_save_userid, &root_save_sec_context);
+ SetUserIdAndSecContext(rel->rd_rel->relowner,
+ root_save_sec_context | SECURITY_RESTRICTED_OPERATION);
+ root_save_nestlevel = NewGUCNestLevel();
+
/* And the target index relation */
indexRelation = index_open(indexRelationId, RowExclusiveLock);
@@ -720,6 +757,12 @@
/* Now build the index */
index_build(rel, indexRelation, indexInfo, stmt->primary, false);
+ /* Roll back any GUC changes executed by index functions */
+ AtEOXact_GUC(false, root_save_nestlevel);
+
+ /* Restore userid and security context */
+ SetUserIdAndSecContext(root_save_userid, root_save_sec_context);
+
/* Close both the relations, but keep the locks */
heap_close(rel, NoLock);
index_close(indexRelation, NoLock);
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -235,15 +235,21 @@
* with guc.c's internal state, so SET ROLE has to be disallowed.
*
* SECURITY_RESTRICTED_OPERATION indicates that we are inside an operation
- * that does not wish to trust called user-defined functions at all. This
- * bit prevents not only SET ROLE, but various other changes of session state
- * that normally is unprotected but might possibly be used to subvert the
- * calling session later. An example is replacing an existing prepared
- * statement with new code, which will then be executed with the outer
- * session's permissions when the prepared statement is next used. Since
- * these restrictions are fairly draconian, we apply them only in contexts
- * where the called functions are really supposed to be side-effect-free
- * anyway, such as VACUUM/ANALYZE/REINDEX.
+ * that does not wish to trust called user-defined functions at all. The
+ * policy is to use this before operations, e.g. autovacuum and REINDEX, that
+ * enumerate relations of a database or schema and run functions associated
+ * with each found relation. The relation owner is the new user ID. Set this
+ * as soon as possible after locking the relation. Restore the old user ID as
+ * late as possible before closing the relation; restoring it shortly after
+ * close is also tolerable. If a command has both relation-enumerating and
+ * non-enumerating modes, e.g. ANALYZE, both modes set this bit. This bit
+ * prevents not only SET ROLE, but various other changes of session state that
+ * normally is unprotected but might possibly be used to subvert the calling
+ * session later. An example is replacing an existing prepared statement with
+ * new code, which will then be executed with the outer session's permissions
+ * when the prepared statement is next used. These restrictions are fairly
+ * draconian, but the functions called in relation-enumerating operations are
+ * really supposed to be side-effect-free anyway.
*
* Unlike GetUserId, GetUserIdAndSecContext does *not* Assert that the current
* value of CurrentUserId is valid; nor does SetUserIdAndSecContext require
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -1194,6 +1194,41 @@
-- security-restricted operations
\c -
CREATE ROLE regress_sro_user;
+-- Check that index expressions and predicates are run as the table's owner
+-- A dummy index function checking current_user
+CREATE FUNCTION sro_ifun(int) RETURNS int AS $$
+BEGIN
+ -- Below we set the table's owner to regress_sro_user
+ IF current_user <> 'regress_sro_user' THEN
+ RAISE 'called by %', current_user;
+ END IF;
+ RETURN $1;
+END;
+$$ LANGUAGE plpgsql IMMUTABLE;
+-- Create a table owned by regress_sro_user
+CREATE TABLE sro_tab (a int);
+ALTER TABLE sro_tab OWNER TO regress_sro_user;
+INSERT INTO sro_tab VALUES (1), (2), (3);
+-- Create an expression index with a predicate
+CREATE INDEX sro_idx ON sro_tab ((sro_ifun(a) + sro_ifun(0)))
+ WHERE sro_ifun(a + 10) > sro_ifun(10);
+DROP INDEX sro_idx;
+-- Do the same concurrently
+CREATE INDEX CONCURRENTLY sro_idx ON sro_tab ((sro_ifun(a) + sro_ifun(0)))
+ WHERE sro_ifun(a + 10) > sro_ifun(10);
+-- REINDEX
+REINDEX TABLE sro_tab;
+REINDEX INDEX sro_idx;
+REINDEX TABLE CONCURRENTLY sro_tab; -- v12+ feature
+ERROR: syntax error at or near "CONCURRENTLY"
+LINE 1: REINDEX TABLE CONCURRENTLY sro_tab;
+ ^
+DROP INDEX sro_idx;
+-- CLUSTER
+CREATE INDEX sro_cluster_idx ON sro_tab ((sro_ifun(a) + sro_ifun(0)));
+CLUSTER sro_tab USING sro_cluster_idx;
+DROP INDEX sro_cluster_idx;
+DROP TABLE sro_tab;
SET SESSION AUTHORIZATION regress_sro_user;
CREATE FUNCTION unwanted_grant() RETURNS void LANGUAGE sql AS
'GRANT regressgroup2 TO regress_sro_user';
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -724,6 +724,40 @@
\c -
CREATE ROLE regress_sro_user;
+-- Check that index expressions and predicates are run as the table's owner
+
+-- A dummy index function checking current_user
+CREATE FUNCTION sro_ifun(int) RETURNS int AS $$
+BEGIN
+ -- Below we set the table's owner to regress_sro_user
+ IF current_user <> 'regress_sro_user' THEN
+ RAISE 'called by %', current_user;
+ END IF;
+ RETURN $1;
+END;
+$$ LANGUAGE plpgsql IMMUTABLE;
+-- Create a table owned by regress_sro_user
+CREATE TABLE sro_tab (a int);
+ALTER TABLE sro_tab OWNER TO regress_sro_user;
+INSERT INTO sro_tab VALUES (1), (2), (3);
+-- Create an expression index with a predicate
+CREATE INDEX sro_idx ON sro_tab ((sro_ifun(a) + sro_ifun(0)))
+ WHERE sro_ifun(a + 10) > sro_ifun(10);
+DROP INDEX sro_idx;
+-- Do the same concurrently
+CREATE INDEX CONCURRENTLY sro_idx ON sro_tab ((sro_ifun(a) + sro_ifun(0)))
+ WHERE sro_ifun(a + 10) > sro_ifun(10);
+-- REINDEX
+REINDEX TABLE sro_tab;
+REINDEX INDEX sro_idx;
+REINDEX TABLE CONCURRENTLY sro_tab; -- v12+ feature
+DROP INDEX sro_idx;
+-- CLUSTER
+CREATE INDEX sro_cluster_idx ON sro_tab ((sro_ifun(a) + sro_ifun(0)));
+CLUSTER sro_tab USING sro_cluster_idx;
+DROP INDEX sro_cluster_idx;
+DROP TABLE sro_tab;
+
SET SESSION AUTHORIZATION regress_sro_user;
CREATE FUNCTION unwanted_grant() RETURNS void LANGUAGE sql AS
'GRANT regressgroup2 TO regress_sro_user';
--Gfegn71+neI+QTr7
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="CVE-2022-1552_f26d570285_adapted_for_9.4.patch"
view thread (5+ messages) latest in thread
Message-ID: <no-message-id-643355@localhost>
Permalink: ../../no-message-id-643355@localhost/
Also on: postgresql.org/message-id/no-message-id-643355@localhost
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: noah@leadboat.com
Subject: Re: [PATCH] Make relation-enumerating operations be security-restricted operations.
In-Reply-To: <no-message-id-643355@localhost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox