CVE-2023-6507: Insufficient Information (NVD-CWE-noinfo)
value: An issue was found in CPython 3.12.0 `subprocess` module on POSIX platforms. The issue was fixed in CPython 3.12.1 and does not affect other stable releases.

When using the `extra_groups=` parameter with an empty list as a value (ie `extra_groups=[]`) the logic regressed to not call `setgroups(0, NULL)` before calling `exec()`, thus not dropping the original processes' groups before starting the new process. There is no issue when the parameter isn't used or when any value is used besides an empty list.

This issue only impacts CPython processes run with sufficient privilege to make the `setgroups` system call (typically `root`).


CWE description: There is insufficient information about the issue to classify it; details are unkown or unspecified.
CVSS v3 base score: 4.9
Published: 2023-12-08T19:15Z
Repository: https://github.com/python/cpython
Language: Other
Files changed: Lib/test/test_subprocess.py, Misc/NEWS.d/next/Library/2023-12-01-21-05-46.gh-issue-112334.DmNXKh.rst, Modules/_posixsubprocess.c
Commit date: 2023-12-04 15:08:19 -0800
Commit message: gh-112334: Restore subprocess's use of `vfork()` & fix `extra_groups=[]` behavior (#112617)

Restore `subprocess`'s intended use of `vfork()` by default for performance on Linux;
also fixes the behavior of `extra_groups=[]` which was unintentionally broken in 3.12.0:

Fixed a performance regression in 3.12's :mod:`subprocess` on Linux where it
would no longer use the fast-path ``vfork()`` system call when it could have
due to a logic bug, instead falling back to the safe but slower ``fork()``.

Also fixed a security bug introduced in 3.12.0.  If a value of ``extra_groups=[]``
was passed to :mod:`subprocess.Popen` or related APIs, the underlying
``setgroups(0, NULL)`` system call to clear the groups list would not be made
in the child process prior to ``exec()``.

The security issue was identified via code inspection in the process of
fixing the first bug.  Thanks to @vain for the detailed report and
analysis in the initial bug on Github.

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Diff: diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py
index fe1a3675fced..319bc0d26385 100644
--- a/Lib/test/test_subprocess.py
+++ b/Lib/test/test_subprocess.py
@@ -2064,60 +2064,56 @@ def test_group_error(self):
 
     @unittest.skipUnless(hasattr(os, 'setgroups'), 'no setgroups() on platform')
     def test_extra_groups(self):
         gid = os.getegid()
         group_list = [65534 if gid != 65534 else 65533]
+        self._test_extra_groups_impl(gid=gid, group_list=group_list)
+
+    @unittest.skipUnless(hasattr(os, 'setgroups'), 'no setgroups() on platform')
+    def test_extra_groups_empty_list(self):
+        self._test_extra_groups_impl(gid=os.getegid(), group_list=[])
+
+    def _test_extra_groups_impl(self, *, gid, group_list):
         name_group = _get_test_grp_name()
-        perm_error = False
 
         if grp is not None:
             group_list.append(name_group)
 
         try:
             output = subprocess.check_output(
                     [sys.executable, "-c",
                      "import os, sys, json; json.dump(os.getgroups(), sys.stdout)"],
                     extra_groups=group_list)
-        except OSError as ex:
-            if ex.errno != errno.EPERM:
-                raise
-            perm_error = True
-
+        except PermissionError:
+            self.skipTest("setgroup() EPERM; this test may require root.")
         else:
             parent_groups = os.getgroups()
             child_groups = json.loads(output)
 
             if grp is not None:
                 desired_gids = [grp.getgrnam(g).gr_gid if isinstance(g, str) else g
                                 for g in group_list]
             else:
                 desired_gids = group_list
 
-            if perm_error:
-                self.assertEqual(set(child_groups), set(parent_groups))
-            else:
-                self.assertEqual(set(desired_gids), set(child_groups))
+            self.assertEqual(set(desired_gids), set(child_groups))
 
-        # make sure we bomb on negative values
+        if grp is None:
+            with self.assertRaises(ValueError):
+                subprocess.check_call(ZERO_RETURN_CMD,
+                                      extra_groups=[name_group])
+
+    # No skip necessary, this test won't make it to a setgroup() call.
+    def test_extra_groups_invalid_gid_t_values(self):
         with self.assertRaises(ValueError):
             subprocess.check_call(ZERO_RETURN_CMD, extra_groups=[-1])
 
         with self.assertRaises(ValueError):
             subprocess.check_call(ZERO_RETURN_CMD,
                                   cwd=os.curdir, env=os.environ,
                                   extra_groups=[2**64])
 
-        if grp is None:
-            with self.assertRaises(ValueError):
-                subprocess.check_call(ZERO_RETURN_CMD,
-                                      extra_groups=[name_group])
-
-    @unittest.skipIf(hasattr(os, 'setgroups'), 'setgroups() available on platform')
-    def test_extra_groups_error(self):
-        with self.assertRaises(ValueError):
-            subprocess.check_call(ZERO_RETURN_CMD, extra_groups=[])
-
     @unittest.skipIf(mswindows or not hasattr(os, 'umask'),
                      'POSIX umask() is not available.')
     def test_umask(self):
         tmpdir = None
         try:
diff --git a/Misc/NEWS.d/next/Library/2023-12-01-21-05-46.gh-issue-112334.DmNXKh.rst b/Misc/NEWS.d/next/Library/2023-12-01-21-05-46.gh-issue-112334.DmNXKh.rst
new file mode 100644
index 000000000000..3a53a8bf8423
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2023-12-01-21-05-46.gh-issue-112334.DmNXKh.rst
@@ -0,0 +1,11 @@
+Fixed a performance regression in 3.12's :mod:`subprocess` on Linux where it
+would no longer use the fast-path ``vfork()`` system call when it could have
+due to a logic bug, instead falling back to the safe but slower ``fork()``.
+
+Also fixed a second 3.12.0 potential security bug.  If a value of
+``extra_groups=[]`` was passed to :mod:`subprocess.Popen` or related APIs,
+the underlying ``setgroups(0, NULL)`` system call to clear the groups list
+would not be made in the child process prior to ``exec()``.
+
+This was identified via code inspection in the process of fixing the first
+bug.
diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c
index 2898eedc3e3a..d0dd8f064e03 100644
--- a/Modules/_posixsubprocess.c
+++ b/Modules/_posixsubprocess.c
@@ -765,12 +765,14 @@ child_exec(char *const exec_array[],
         POSIX_CALL(setpgid(0, pgid_to_set));
     }
 #endif
 
 #ifdef HAVE_SETGROUPS
-    if (extra_group_size > 0)
+    if (extra_group_size >= 0) {
+        assert((extra_group_size == 0) == (extra_groups == NULL));
         POSIX_CALL(setgroups(extra_group_size, extra_groups));
+    }
 #endif /* HAVE_SETGROUPS */
 
 #ifdef HAVE_SETREGID
     if (gid != (gid_t)-1)
         POSIX_CALL(setregid(gid, gid));
@@ -1020,11 +1022,10 @@ subprocess_fork_exec_impl(PyObject *module, PyObject *process_args,
     PyObject *cwd_obj2 = NULL;
     const char *cwd = NULL;
     pid_t pid = -1;
     int need_to_reenable_gc = 0;
     char *const *argv = NULL, *const *envp = NULL;
-    Py_ssize_t extra_group_size = 0;
     int need_after_fork = 0;
     int saved_errno = 0;
     int *c_fds_to_keep = NULL;
     Py_ssize_t fds_to_keep_len = PyTuple_GET_SIZE(py_fds_to_keep);
 
@@ -1101,10 +1102,17 @@ subprocess_fork_exec_impl(PyObject *module, PyObject *process_args,
         if (PyUnicode_FSConverter(cwd_obj, &cwd_obj2) == 0)
             goto cleanup;
         cwd = PyBytes_AsString(cwd_obj2);
     }
 
+    // Special initial value meaning that subprocess API was called with
+    // extra_groups=None leading to _posixsubprocess.fork_exec(gids=None).
+    // We use this to differentiate between code desiring a setgroups(0, NULL)
+    // call vs no call at all.  The fast vfork() code path could be used when
+    // there is no setgroups call.
+    Py_ssize_t extra_group_size = -2;
+
     if (extra_groups_packed != Py_None) {
 #ifdef HAVE_SETGROUPS
         if (!PyList_Check(extra_groups_packed)) {
             PyErr_SetString(PyExc_TypeError,
                     "setgroups argument must be a list");

Vulnerable code:         perm_error = False
        except OSError as ex:
            if ex.errno != errno.EPERM:
                raise
            perm_error = True

            if perm_error:
                self.assertEqual(set(child_groups), set(parent_groups))
            else:
                self.assertEqual(set(desired_gids), set(child_groups))
        # make sure we bomb on negative values
        if grp is None:
            with self.assertRaises(ValueError):
                subprocess.check_call(ZERO_RETURN_CMD,
                                      extra_groups=[name_group])

    @unittest.skipIf(hasattr(os, 'setgroups'), 'setgroups() available on platform')
    def test_extra_groups_error(self):
        with self.assertRaises(ValueError):
            subprocess.check_call(ZERO_RETURN_CMD, extra_groups=[])

    if (extra_group_size > 0)
    Py_ssize_t extra_group_size = 0;
Fixed code:         self._test_extra_groups_impl(gid=gid, group_list=group_list)

    @unittest.skipUnless(hasattr(os, 'setgroups'), 'no setgroups() on platform')
    def test_extra_groups_empty_list(self):
        self._test_extra_groups_impl(gid=os.getegid(), group_list=[])

    def _test_extra_groups_impl(self, *, gid, group_list):
        except PermissionError:
            self.skipTest("setgroup() EPERM; this test may require root.")
            self.assertEqual(set(desired_gids), set(child_groups))
        if grp is None:
            with self.assertRaises(ValueError):
                subprocess.check_call(ZERO_RETURN_CMD,
                                      extra_groups=[name_group])

    # No skip necessary, this test won't make it to a setgroup() call.
    def test_extra_groups_invalid_gid_t_values(self):
Fixed a performance regression in 3.12's :mod:`subprocess` on Linux where it
would no longer use the fast-path ``vfork()`` system call when it could have
due to a logic bug, instead falling back to the safe but slower ``fork()``.

Also fixed a second 3.12.0 potential security bug.  If a value of
``extra_groups=[]`` was passed to :mod:`subprocess.Popen` or related APIs,
the underlying ``setgroups(0, NULL)`` system call to clear the groups list
would not be made in the child process prior to ``exec()``.

This was identified via code inspection in the process of fixing the first
bug.
    if (extra_group_size >= 0) {
        assert((extra_group_size == 0) == (extra_groups == NULL));
    }
    // Special initial value meaning that subprocess API was called with
    // extra_groups=None leading to _posixsubprocess.fork_exec(gids=None).
    // We use this to differentiate between code desiring a setgroups(0, NULL)
    // call vs no call at all.  The fast vfork() code path could be used when
    // there is no setgroups call.
    Py_ssize_t extra_group_size = -2;