From 681d9e4621aac0a9c71364b6f54f00f6d8c4337f Mon Sep 17 00:00:00 2001 From: Noah Misch Date: Mon, 8 May 2023 06:14:07 -0700 Subject: Replace last PushOverrideSearchPath() call with set_config_option(). The two methods don't cooperate, so set_config_option("search_path", ...) has been ineffective under non-empty overrideStack. This defect enabled an attacker having database-level CREATE privilege to execute arbitrary code as the bootstrap superuser. While that particular attack requires v13+ for the trusted extension attribute, other attacks are feasible in all supported versions. Standardize on the combination of NewGUCNestLevel() and set_config_option("search_path", ...). It is newer than PushOverrideSearchPath(), more-prevalent, and has no known disadvantages. The "override" mechanism remains for now, for compatibility with out-of-tree code. Users should update such code, which likely suffers from the same sort of vulnerability closed here. Back-patch to v11 (all supported versions). Alexander Lakhin. Reported by Alexander Lakhin. Security: CVE-2023-2454 --- contrib/seg/Makefile | 2 +- contrib/seg/expected/security.out | 32 +++++++++++++++++++++++ contrib/seg/sql/security.sql | 32 +++++++++++++++++++++++ src/backend/catalog/namespace.c | 4 +++ src/backend/commands/schemacmds.c | 37 +++++++++++++++++++-------- src/test/regress/expected/namespace.out | 45 +++++++++++++++++++++++++++++++++ src/test/regress/sql/namespace.sql | 24 ++++++++++++++++++ 7 files changed, 165 insertions(+), 11 deletions(-) create mode 100644 contrib/seg/expected/security.out create mode 100644 contrib/seg/sql/security.sql diff --git a/contrib/seg/Makefile b/contrib/seg/Makefile index c6c134b8f1..a1e49bf051 100644 --- a/contrib/seg/Makefile +++ b/contrib/seg/Makefile @@ -14,7 +14,7 @@ PGFILEDESC = "seg - line segment data type" HEADERS = segdata.h -REGRESS = seg +REGRESS = security seg EXTRA_CLEAN = y.tab.c y.tab.h diff --git a/contrib/seg/expected/security.out b/contrib/seg/expected/security.out new file mode 100644 index 0000000000..b47598d039 --- /dev/null +++ b/contrib/seg/expected/security.out @@ -0,0 +1,32 @@ +-- +-- Test extension script protection against search path overriding +-- +CREATE ROLE regress_seg_role; +SELECT current_database() AS datname \gset +GRANT CREATE ON DATABASE :"datname" TO regress_seg_role; +SET ROLE regress_seg_role; +CREATE SCHEMA regress_seg_schema; +CREATE FUNCTION regress_seg_schema.exfun(i int) RETURNS int AS $$ +BEGIN + CREATE EXTENSION seg VERSION '1.2'; + + CREATE FUNCTION regress_seg_schema.compare(oid, regclass) RETURNS boolean AS + 'BEGIN RAISE EXCEPTION ''overloaded compare() called by %'', current_user; END;' LANGUAGE plpgsql; + + CREATE OPERATOR = (LEFTARG = oid, RIGHTARG = regclass, PROCEDURE = regress_seg_schema.compare); + + ALTER EXTENSION seg UPDATE TO '1.3'; + + RETURN i; +END; $$ LANGUAGE plpgsql; +CREATE SCHEMA test_schema +CREATE TABLE t(i int) PARTITION BY RANGE (i) +CREATE TABLE p1 PARTITION OF t FOR VALUES FROM (1) TO (regress_seg_schema.exfun(2)); +DROP SCHEMA test_schema CASCADE; +NOTICE: drop cascades to 3 other objects +DETAIL: drop cascades to table test_schema.t +drop cascades to extension seg +drop cascades to operator test_schema.=(oid,regclass) +RESET ROLE; +DROP OWNED BY regress_seg_role; +DROP ROLE regress_seg_role; diff --git a/contrib/seg/sql/security.sql b/contrib/seg/sql/security.sql new file mode 100644 index 0000000000..7dfbbaa304 --- /dev/null +++ b/contrib/seg/sql/security.sql @@ -0,0 +1,32 @@ +-- +-- Test extension script protection against search path overriding +-- + +CREATE ROLE regress_seg_role; +SELECT current_database() AS datname \gset +GRANT CREATE ON DATABASE :"datname" TO regress_seg_role; +SET ROLE regress_seg_role; +CREATE SCHEMA regress_seg_schema; + +CREATE FUNCTION regress_seg_schema.exfun(i int) RETURNS int AS $$ +BEGIN + CREATE EXTENSION seg VERSION '1.2'; + + CREATE FUNCTION regress_seg_schema.compare(oid, regclass) RETURNS boolean AS + 'BEGIN RAISE EXCEPTION ''overloaded compare() called by %'', current_user; END;' LANGUAGE plpgsql; + + CREATE OPERATOR = (LEFTARG = oid, RIGHTARG = regclass, PROCEDURE = regress_seg_schema.compare); + + ALTER EXTENSION seg UPDATE TO '1.3'; + + RETURN i; +END; $$ LANGUAGE plpgsql; + +CREATE SCHEMA test_schema +CREATE TABLE t(i int) PARTITION BY RANGE (i) +CREATE TABLE p1 PARTITION OF t FOR VALUES FROM (1) TO (regress_seg_schema.exfun(2)); + +DROP SCHEMA test_schema CASCADE; +RESET ROLE; +DROP OWNED BY regress_seg_role; +DROP ROLE regress_seg_role; diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c index 14e57adee2..73ddb67882 100644 --- a/src/backend/catalog/namespace.c +++ b/src/backend/catalog/namespace.c @@ -3515,6 +3515,10 @@ OverrideSearchPathMatchesCurrent(OverrideSearchPath *path) /* * PushOverrideSearchPath - temporarily override the search path * + * Do not use this function; almost any usage introduces a security + * vulnerability. It exists for the benefit of legacy code running in + * non-security-sensitive environments. + * * We allow nested overrides, hence the push/pop terminology. The GUC * search_path variable is ignored while an override is active. * diff --git a/src/backend/commands/schemacmds.c b/src/backend/commands/schemacmds.c index 48590247f8..b6a71154a8 100644 --- a/src/backend/commands/schemacmds.c +++ b/src/backend/commands/schemacmds.c @@ -30,6 +30,7 @@ #include "commands/schemacmds.h" #include "miscadmin.h" #include "parser/parse_utilcmd.h" +#include "parser/scansup.h" #include "tcop/utility.h" #include "utils/acl.h" #include "utils/builtins.h" @@ -53,14 +54,16 @@ CreateSchemaCommand(CreateSchemaStmt *stmt, const char *queryString, { const char *schemaName = stmt->schemaname; Oid namespaceId; - OverrideSearchPath *overridePath; List *parsetree_list; ListCell *parsetree_item; Oid owner_uid; Oid saved_uid; int save_sec_context; + int save_nestlevel; + char *nsp = namespace_search_path; AclResult aclresult; ObjectAddress address; + StringInfoData pathbuf; GetUserIdAndSecContext(&saved_uid, &save_sec_context); @@ -153,14 +156,26 @@ CreateSchemaCommand(CreateSchemaStmt *stmt, const char *queryString, CommandCounterIncrement(); /* - * Temporarily make the new namespace be the front of the search path, as - * well as the default creation target namespace. This will be undone at - * the end of this routine, or upon error. + * Prepend the new schema to the current search path. + * + * We use the equivalent of a function SET option to allow the setting to + * persist for exactly the duration of the schema creation. guc.c also + * takes care of undoing the setting on error. */ - overridePath = GetOverrideSearchPath(CurrentMemoryContext); - overridePath->schemas = lcons_oid(namespaceId, overridePath->schemas); - /* XXX should we clear overridePath->useTemp? */ - PushOverrideSearchPath(overridePath); + save_nestlevel = NewGUCNestLevel(); + + initStringInfo(&pathbuf); + appendStringInfoString(&pathbuf, quote_identifier(schemaName)); + + while (scanner_isspace(*nsp)) + nsp++; + + if (*nsp != '\0') + appendStringInfo(&pathbuf, ", %s", nsp); + + (void) set_config_option("search_path", pathbuf.data, + PGC_USERSET, PGC_S_SESSION, + GUC_ACTION_SAVE, true, 0, false); /* * Report the new schema to possibly interested event triggers. Note we @@ -215,8 +230,10 @@ CreateSchemaCommand(CreateSchemaStmt *stmt, const char *queryString, CommandCounterIncrement(); } - /* Reset search path to normal state */ - PopOverrideSearchPath(); + /* + * Restore the GUC variable search_path we set above. + */ + AtEOXact_GUC(true, save_nestlevel); /* Reset current user and security context */ SetUserIdAndSecContext(saved_uid, save_sec_context); diff --git a/src/test/regress/expected/namespace.out b/src/test/regress/expected/namespace.out index 2564d1b080..a62fd8ded0 100644 --- a/src/test/regress/expected/namespace.out +++ b/src/test/regress/expected/namespace.out @@ -1,6 +1,14 @@ -- -- Regression tests for schemas (namespaces) -- +-- set the whitespace-only search_path to test that the +-- GUC list syntax is preserved during a schema creation +SELECT pg_catalog.set_config('search_path', ' ', false); + set_config +------------ + +(1 row) + CREATE SCHEMA test_ns_schema_1 CREATE UNIQUE INDEX abc_a_idx ON abc (a) CREATE VIEW abc_view AS @@ -9,6 +17,43 @@ CREATE SCHEMA test_ns_schema_1 a serial, b int UNIQUE ); +-- verify that the correct search_path restored on abort +SET search_path to public; +BEGIN; +SET search_path to public, test_ns_schema_1; +CREATE SCHEMA test_ns_schema_2 + CREATE VIEW abc_view AS SELECT c FROM abc; +ERROR: column "c" does not exist +LINE 2: CREATE VIEW abc_view AS SELECT c FROM abc; + ^ +COMMIT; +SHOW search_path; + search_path +------------- + public +(1 row) + +-- verify that the correct search_path preserved +-- after creating the schema and on commit +BEGIN; +SET search_path to public, test_ns_schema_1; +CREATE SCHEMA test_ns_schema_2 + CREATE VIEW abc_view AS SELECT a FROM abc; +SHOW search_path; + search_path +-------------------------- + public, test_ns_schema_1 +(1 row) + +COMMIT; +SHOW search_path; + search_path +-------------------------- + public, test_ns_schema_1 +(1 row) + +DROP SCHEMA test_ns_schema_2 CASCADE; +NOTICE: drop cascades to view test_ns_schema_2.abc_view -- verify that the objects were created SELECT COUNT(*) FROM pg_class WHERE relnamespace = (SELECT oid FROM pg_namespace WHERE nspname = 'test_ns_schema_1'); diff --git a/src/test/regress/sql/namespace.sql b/src/test/regress/sql/namespace.sql index 6b12c96193..3474f5ecf4 100644 --- a/src/test/regress/sql/namespace.sql +++ b/src/test/regress/sql/namespace.sql @@ -2,6 +2,10 @@ -- Regression tests for schemas (namespaces) -- +-- set the whitespace-only search_path to test that the +-- GUC list syntax is preserved during a schema creation +SELECT pg_catalog.set_config('search_path', ' ', false); + CREATE SCHEMA test_ns_schema_1 CREATE UNIQUE INDEX abc_a_idx ON abc (a) @@ -13,6 +17,26 @@ CREATE SCHEMA test_ns_schema_1 b int UNIQUE ); +-- verify that the correct search_path restored on abort +SET search_path to public; +BEGIN; +SET search_path to public, test_ns_schema_1; +CREATE SCHEMA test_ns_schema_2 + CREATE VIEW abc_view AS SELECT c FROM abc; +COMMIT; +SHOW search_path; + +-- verify that the correct search_path preserved +-- after creating the schema and on commit +BEGIN; +SET search_path to public, test_ns_schema_1; +CREATE SCHEMA test_ns_schema_2 + CREATE VIEW abc_view AS SELECT a FROM abc; +SHOW search_path; +COMMIT; +SHOW search_path; +DROP SCHEMA test_ns_schema_2 CASCADE; + -- verify that the objects were created SELECT COUNT(*) FROM pg_class WHERE relnamespace = (SELECT oid FROM pg_namespace WHERE nspname = 'test_ns_schema_1'); -- cgit v1.2.1