gh-112334: Restore subprocess's use of `vfork()` & fix `extra_groups=[]` behavior #112617

gpshead · 2023-12-02T05:15:53Z

Fixed a performance regression in 3.12's 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 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 regarding vfork usage is desirable. I'm pondering a test that runs when strace is available and permitted which and confirms use of vfork() vs clone()...
A test that will catch setgroup() not being called is included in this PR. It must be run as root on Linux. I believe one of our buildbots is configured to run that way.
Discuss with Python Security Response team if this is also a noteworthy security fix. It could manifest when a root uid=0 process wants to drop other group memberships while executing a subprocess. Probably security relevant if the user= and group= parameters are also being used to drop privs...

Fixes #112334.

The security issue has been assigned CVE-2023-6507.

Issue: subprocess.Popen: Performance regression on Linux since 124af17b6e [CVE-2023-6507] #112334

@vain

…roups=[]`. 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()`...

bedevere-bot · 2023-12-02T05:41:01Z

🤖 New build scheduled with the buildbot fleet by @gpshead for commit 84d060f 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

vain · 2023-12-04T07:33:37Z

@gpshead Thank you! FWIW, this fixes my test case. :)

Modules/_posixsubprocess.c

Lib/test/test_subprocess.py

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

Based on Serhiy's code review. (thanks!) Confirmed it still passes as root and non-root on Linux.

bedevere-bot · 2023-12-04T21:08:48Z

🤖 New build scheduled with the buildbot fleet by @serhiy-storchaka for commit ce31462 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

miss-islington-app · 2023-12-04T23:08:23Z

Thanks @gpshead for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

bedevere-app · 2023-12-04T23:08:31Z

GH-112731 is a backport of this pull request to the 3.12 branch.

@vain

…roups=[]` behavior (pythonGH-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. (cherry picked from commit 9fe7655) Co-authored-by: Gregory P. Smith <greg@krypto.org> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

gpshead · 2023-12-04T23:08:57Z

I will add the desired vfork regression test in a followup PR. Merging now to unblock releasing the fix.

@vain

…groups=[]` behavior (GH-112617) (#112731) 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. (cherry picked from commit 9fe7655) + Reword NEWS for the bugfix/security release. (mentions the assigned CVE number) Co-authored-by: Gregory P. Smith [Google LLC] <greg@krypto.org> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

@vain

…roups=[]` behavior (python#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>

gpshead added type-bug An unexpected behavior, bug, or error 3.12 bugs and security fixes needs backport to 3.12 bug and security fixes labels Dec 2, 2023

gpshead requested review from arhadthedev and sethmlarson December 2, 2023 05:15

gpshead self-assigned this Dec 2, 2023

bedevere-app bot added the awaiting core review label Dec 2, 2023

bedevere-app bot mentioned this pull request Dec 2, 2023

subprocess.Popen: Performance regression on Linux since 124af17b6e [CVE-2023-6507] #112334

Closed

Code Health (remove a compiler warning).

84d060f

gpshead added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 2, 2023

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 2, 2023

gpshead requested a review from Yhg1s December 2, 2023 19:08

gpshead added the release-blocker label Dec 2, 2023

serhiy-storchaka reviewed Dec 4, 2023

View reviewed changes

Modules/_posixsubprocess.c Outdated Show resolved Hide resolved

Lib/test/test_subprocess.py Outdated Show resolved Hide resolved

gpshead and others added 2 commits December 4, 2023 12:36

Improve the assertion.

52a475c

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

Fix the refactored test to not override parameters.

ce31462

Based on Serhiy's code review. (thanks!) Confirmed it still passes as root and non-root on Linux.

serhiy-storchaka approved these changes Dec 4, 2023

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Dec 4, 2023

serhiy-storchaka added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 4, 2023

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 4, 2023

gpshead merged commit 9fe7655 into python:main Dec 4, 2023
78 of 87 checks passed

bedevere-app bot removed the awaiting merge label Dec 4, 2023

gpshead deleted the bug/subprocess/112334 branch December 4, 2023 23:08

bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Dec 4, 2023

gpshead mentioned this pull request Dec 4, 2023

gh-112334: Regression test that vfork is used when expected. #112734

Merged

gpshead added the type-security A security issue label Dec 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-112334: Restore subprocess's use of `vfork()` & fix `extra_groups=[]` behavior #112617

gh-112334: Restore subprocess's use of `vfork()` & fix `extra_groups=[]` behavior #112617

gpshead commented Dec 2, 2023 •

edited

bedevere-bot commented Dec 2, 2023

vain commented Dec 4, 2023

bedevere-bot commented Dec 4, 2023

miss-islington-app bot commented Dec 4, 2023

bedevere-app bot commented Dec 4, 2023

gpshead commented Dec 4, 2023

gh-112334: Restore subprocess's use of vfork() & fix extra_groups=[] behavior #112617

gh-112334: Restore subprocess's use of vfork() & fix extra_groups=[] behavior #112617

Conversation

gpshead commented Dec 2, 2023 • edited

bedevere-bot commented Dec 2, 2023

vain commented Dec 4, 2023

bedevere-bot commented Dec 4, 2023

miss-islington-app bot commented Dec 4, 2023

bedevere-app bot commented Dec 4, 2023

gpshead commented Dec 4, 2023

gh-112334: Restore subprocess's use of `vfork()` & fix `extra_groups=[]` behavior #112617

gh-112334: Restore subprocess's use of `vfork()` & fix `extra_groups=[]` behavior #112617

gpshead commented Dec 2, 2023 •

edited