Replace last PushOverrideSearchPath() call with set_config_option().
authorNoah Misch <[email protected]>
Mon, 8 May 2023 13:14:07 +0000 (06:14 -0700)
committerNoah Misch <[email protected]>
Mon, 8 May 2023 13:14:07 +0000 (06:14 -0700)
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
contrib/seg/expected/security.out [new file with mode: 0644]
contrib/seg/sql/security.sql [new file with mode: 0644]
src/backend/catalog/namespace.c
src/backend/commands/schemacmds.c
src/test/regress/expected/namespace.out
src/test/regress/sql/namespace.sql

index c6c134b8f15eaed0cfda8eab52322ca9117633da..a1e49bf051e3d6128d8ec1cedc04e86d8236289c 100644 (file)
@@ -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 (file)
index 0000000..b47598d
--- /dev/null
@@ -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 (file)
index 0000000..7dfbbaa
--- /dev/null
@@ -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;
index 14e57adee2b4641af8e69be78c607fd95a5918b1..73ddb67882f8c36e7c6f0a56c7db797f82ca14e8 100644 (file)
@@ -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.
  *
index 48590247f80304e365ae10e0677b30406de2d4ed..b6a71154a8784404d0988e30bd8e25189c1f6dcf 100644 (file)
@@ -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);
index 2564d1b080993efba8eef65d92375dcdb0b1c734..a62fd8ded015e6ee56784e4341e5ac1a4ed5621f 100644 (file)
@@ -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');
index 6b12c9619361ab93b3d98c3d134d84ed433893a5..3474f5ecf4215068fd89ec52f9ee8b95ddff36bb 100644 (file)
@@ -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');