Repository navigation
Undocumented risky behaviour in subprocess module #101283
Description
Bug report - Undocumented risky behaviour in subprocess module
When using subprocess.Popen with shell=True on Windows and without a COMSPEC environment variable, a cmd.exe is launched. The problem is the cmd.exe full path is not written, Windows will search the executable in the current directory and in the PATH. If an arbitrary executable file is written to the current directory or to a directory in the PATH, it can be run instead of the real cmd.exe.
See the code here and a POC here.
- This risky behaviour can be patched by replacing
cmd.exestring byC:\WINDOWS\system32\cmd.exe. - If the behavior was chosen by python developers, it should be documented.
Linked PRs
- gh-101283: Try to load the fallback cmd.exe by an absolute path #101286
- [3.10] gh-101283: Improved fallback logic for subprocess with shell=True on Windows (GH-101286) #101708
- [3.9] gh-101283: Improved fallback logic for subprocess with shell=True on Windows (GH-101286) #101709
- [3.8] gh-101283: Improved fallback logic for subprocess with shell=True on Windows (GH-101286) #101710
- [3.11] gh-101283: Improved fallback logic for subprocess with shell=True on Windows (GH-101286) #101711
- gh-101283: Fix use of unbound variable #101712
- [3.7] gh-101283: Improved fallback logic for subprocess with shell=True on Windows (GH-101286) #101713
- gh-101283: Version was just released, so should be changed in 3.11.3 #101719
- [3.11] gh-101283: Version was just released, so should be changed in 3.11.3 (GH-101719) #101721
- gh-101283: Fix the
versionchangedof gh-101283 (3.12 only) #101728
Reacted by Oleg Iarygin and Raj Patnaik
Activity
eryksun commentedon Jan 24, 2023
FYI,
subprocess.Popenshould never find "cmd.exe" inPATH. We callCreateProcessW(), which always searches the application directory and system directories before it searchesPATH.CreateProcessW()may also check the current directory immediately after the application directory and before the system directories. This unsafe behavior can be prevented by defining the environment variableNoDefaultCurrentDirectoryInExePath. This environment variable has been supported for over 15 years now, since Windows Vista. Our implementation ofshutil.which()should honor this security feature by callingNeedCurrentDirectoryForExePathW().Note that if
NoDefaultCurrentDirectoryInExePathis defined, the current directory is still included in the search path if the name is a relative path that includes backslashes (e.g. "Scripts\eggs.exe"), but not if it only includes forward slashes (e.g. "Scripts/eggs.exe"). Note also that the application directory takes precedence over the current directory in this case (e.g. if the application directory is "C:\Program Files\Python311", then "Scripts\eggs.exe" will resolve to "C:\Program Files\Python311\Scripts\eggs.exe" if it exists).If you define
NoDefaultCurrentDirectoryInExePath, the current directory can be added explicitly as a "." entry at the end of either the system or userPATH. I add "." to the end of the userPATHbecause some apps and scripts still depend on finding binaries and scripts in the current directory, but I won't cater to a bad actor that expects the current directory to be searched before system directories.gpshead commentedon Jan 24, 2023
FYI - this originally came in as a security report to PSRT. I redirected the reporter here (thanks for reporting this!) as this didn't seem serious enough to be embargoed. I'm still tagging it as such and aiming for a full set of backports as it feels like a good idea.
17 remaining items
[3.9] gh-101283: Improved fallback logic for subprocess with shell=Tr…
[3.8] gh-101283: Improved fallback logic for subprocess with shell=Tr…
[3.7] gh-101283: Improved fallback logic for subprocess with shell=Tr…
zooba commentedon Feb 9, 2023
Fully merged, and all the fixes should be in (I hope). Thanks all!
gh-101283: Fix 'versionchanged' for the shell=True fallback on Window…
Merge branch 'main' into inlinecomp2
Merge branch 'main' into inlinecomp2
Upgrade to 3.8.17 (#26)
Further expand "not from cwd" test re: cmd.exe
Make Git.execute a bit simpler and very slightly more robust