Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.3k
gh-112334: Restore subprocess's use ofvfork() & fixextra_groups=[] behavior#112617
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
…roups=[]`.Fixed a performance regression in 3.12's :mod:`subprocess` on Linux where itwould no longer use the fast-path ``vfork()`` system call when it could havedue 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 listwould not be made in the child process prior to ``exec()``.The security issue was identified via code inspection in the process offixing the first bug. Thanks to@vain for the detailed report andanalysis 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 commentedDec 2, 2023
vain commentedDec 4, 2023
@gpshead Thank you! FWIW, this fixes my test case. :) |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
serhiy-storchaka approved these changesDec 4, 2023
bedevere-bot commentedDec 4, 2023
🤖 New build scheduled with the buildbot fleet by@serhiy-storchaka for commitce31462 🤖 If you want to schedule another build, you need to add the🔨 test-with-buildbots label again. |
Thanks@gpshead for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12. |
GH-112731 is a backport of this pull request to the3.12 branch. |
miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestDec 4, 2023
…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 itwould no longer use the fast-path ``vfork()`` system call when it could havedue 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 madein the child process prior to ``exec()``.The security issue was identified via code inspection in the process offixing the first bug. Thanks to@vain for the detailed report andanalysis in the initial bug on Github.(cherry picked from commit9fe7655)Co-authored-by: Gregory P. Smith <greg@krypto.org>Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
I will add the desired vfork regression test in a followup PR. Merging now to unblock releasing the fix. |
gpshead added a commit that referenced this pull requestDec 4, 2023
…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 itwould no longer use the fast-path ``vfork()`` system call when it could havedue 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 madein the child process prior to ``exec()``.The security issue was identified via code inspection in the process offixing the first bug. Thanks to@vain for the detailed report andanalysis in the initial bug on Github.(cherry picked from commit9fe7655)+ 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>
aisk pushed a commit to aisk/cpython that referenced this pull requestFeb 11, 2024
…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 itwould no longer use the fast-path ``vfork()`` system call when it could havedue 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 madein the child process prior to ``exec()``.The security issue was identified via code inspection in the process offixing the first bug. Thanks to@vain for the detailed report andanalysis in the initial bug on Github.Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Glyphack pushed a commit to Glyphack/cpython that referenced this pull requestSep 2, 2024
…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 itwould no longer use the fast-path ``vfork()`` system call when it could havedue 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 madein the child process prior to ``exec()``.The security issue was identified via code inspection in the process offixing the first bug. Thanks to@vain for the detailed report andanalysis in the initial bug on Github.Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Labels
3.12only security fixes release-blockertype-bugAn unexpected behavior, bug, or error type-securityA security issue
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading.Please reload this page.
Fixed a performance regression in 3.12's
subprocesson Linux where it would no longer use the fast-pathvfork()system call when it could have due to a logic bug, instead falling back to the safe but slowerfork().Also fixed a second 3.12.0 potential security bug. If a value of
extra_groups=[]was passed tosubprocess.Popenor related APIs, the underlyingsetgroups(0, NULL)system call to clear the groups list would not be made in the child process prior toexec().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.
straceis available and permitted which and confirms use ofvfork()vsclone()...setgroup()not being called is included in this PR. It must be run asrooton Linux. I believe one of our buildbots is configured to run that way.user=andgroup=parameters are also being used to drop privs...Fixes#112334.
The security issue has been assignedCVE-2023-6507.