From 7a896fc464a6bceb9b95268b0141667645b2a8da Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Fri, 1 Dec 2023 21:05:59 -0800 Subject: [PATCH] gh-112334: Restore subprocess's use of `vfork()` & fix `extra_groups=[]`. 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()``. 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. * [ ] A regression test is desirable. I'm pondering a test that runs when `strace` is available and permitted which and confirms use of `vfork()` vs `clone()`... --- Lib/test/test_subprocess.py | 40 +++++++++---------- ...-12-01-21-05-46.gh-issue-112334.DmNXKh.rst | 11 +++++ Modules/_posixsubprocess.c | 12 +++++- 3 files changed, 40 insertions(+), 23 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2023-12-01-21-05-46.gh-issue-112334.DmNXKh.rst diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index fe1a3675fced65..d61ceb70f69d8e 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -2064,10 +2064,18 @@ 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): gid = os.getegid() group_list = [65534 if gid != 65534 else 65533] name_group = _get_test_grp_name() - perm_error = False if grp is not None: group_list.append(name_group) @@ -2077,11 +2085,8 @@ def test_extra_groups(self): [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) @@ -2092,12 +2097,15 @@ def test_extra_groups(self): 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]) @@ -2106,16 +2114,6 @@ def test_extra_groups(self): 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): 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 00000000000000..3a53a8bf84230f --- /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 2898eedc3e3a8f..fc48b836fd5188 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -767,8 +767,10 @@ child_exec(char *const exec_array[], #endif #ifdef HAVE_SETGROUPS - if (extra_group_size > 0) + if (extra_group_size >= 0) { + assert(extra_group_size == 0 && extra_groups == NULL || extra_group_size); POSIX_CALL(setgroups(extra_group_size, extra_groups)); + } #endif /* HAVE_SETGROUPS */ #ifdef HAVE_SETREGID @@ -1022,7 +1024,6 @@ subprocess_fork_exec_impl(PyObject *module, PyObject *process_args, 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; @@ -1103,6 +1104,13 @@ subprocess_fork_exec_impl(PyObject *module, PyObject *process_args, 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)) {