Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Undocumented risky behaviour in subprocess module #101283

Closed
mauricelambert opened this issue Jan 24, 2023 · 6 comments
Closed

Undocumented risky behaviour in subprocess module #101283

mauricelambert opened this issue Jan 24, 2023 · 6 comments
Labels
3.7 3.8 3.9 3.10 3.11 OS-windows release-blocker type-bug An unexpected behavior, bug, or error type-security A security issue

Comments

@mauricelambert
Copy link

mauricelambert commented Jan 24, 2023 •
edited by bedevere-bot

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.exe string by C:\WINDOWS\system32\cmd.exe.
  • If the behavior was chosen by python developers, it should be documented.

Linked PRs
@mauricelambert mauricelambert added the type-bug An unexpected behavior, bug, or error label Jan 24, 2023
@bedevere-bot bedevere-bot mentioned this issue Jan 24, 2023
Merged
@eryksun
Copy link
Contributor

eryksun commented Jan 24, 2023 •
edited by gpshead

FYI, subprocess.Popen should never find "cmd.exe" in PATH. We call CreateProcessW(), which always searches the application directory and system directories before it searches PATH.

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 variable NoDefaultCurrentDirectoryInExePath. This environment variable has been supported for over 15 years now, since Windows Vista. Our implementation of shutil.which() should honor this security feature by calling NeedCurrentDirectoryForExePathW().

Note that if NoDefaultCurrentDirectoryInExePath is 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 user PATH. I add "." to the end of the user PATH because 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 gpshead added the type-security A security issue label Jan 24, 2023
@gpshead
Copy link
Member

gpshead commented 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.

@eryksun eryksun removed their assignment Jan 25, 2023
zooba pushed a commit that referenced this issue Feb 8, 2023
@arhadthedev
gh-101283: Improved fallback logic for subprocess with shell=True on …
23751ed 
…Windows (GH-101286)
@bedevere-bot bedevere-bot mentioned this issue Feb 8, 2023
Merged
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Feb 8, 2023
@arhadthedev @miss-islington
pythongh-101283: Improved fallback logic for subprocess with shell=Tr…
675d428 
…ue on Windows (pythonGH-101286)

(cherry picked from commit 23751ed)

Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>
This was referenced Feb 8, 2023
Merged
Merged
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Feb 8, 2023
@arhadthedev @miss-islington
pythongh-101283: Improved fallback logic for subprocess with shell=Tr…
fccaea3 
…ue on Windows (pythonGH-101286)

(cherry picked from commit 23751ed)

Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>
@bedevere-bot bedevere-bot mentioned this issue Feb 8, 2023
Merged
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Feb 8, 2023
@arhadthedev @miss-islington
pythongh-101283: Improved fallback logic for subprocess with shell=Tr…
644134d 
…ue on Windows (pythonGH-101286)

(cherry picked from commit 23751ed)

Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>
@bedevere-bot bedevere-bot mentioned this issue Feb 8, 2023
Merged
zooba added a commit to zooba/cpython that referenced this issue Feb 8, 2023
@zooba
pythongh-101283: Fix use of unbound variable
eddde21
@bedevere-bot bedevere-bot mentioned this issue Feb 8, 2023
Merged
zooba pushed a commit to zooba/cpython that referenced this issue Feb 8, 2023
@arhadthedev @zooba
pythongh-101283: Improved fallback logic for subprocess with shell=Tr…
e06a55e 
…ue on Windows (pythonGH-101286)
@gpshead
Copy link
Member

gpshead commented Feb 8, 2023

The versionchanged markers in several of the PRs are inaccurate.

@gpshead gpshead mentioned this issue Feb 8, 2023
Open
@zooba
Copy link
Member

zooba commented Feb 8, 2023

The backports are not merged yet - do you want to just fix it there?

zooba added a commit that referenced this issue Feb 8, 2023
@zooba
gh-101283: Fix use of unbound variable (GH-101712)
20cf32e
@zooba
Copy link
Member

zooba commented Feb 8, 2023

Ah I see, it's only the versions in main and 3.11 branches.

@bedevere-bot bedevere-bot mentioned this issue Feb 8, 2023
Merged
zooba added a commit to zooba/cpython that referenced this issue Feb 8, 2023
@zooba
pythongh-101283: Version was just released, so should be changed in 3…
783a9a5 
….11.3
zooba added a commit that referenced this issue Feb 8, 2023
@zooba
gh-101283: Version was just released, so should be changed in 3.11.3 (G…
0e0c5d8 
…H-101719)
@bedevere-bot bedevere-bot mentioned this issue Feb 9, 2023
Merged
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Feb 9, 2023
@zooba @miss-islington
pythongh-101283: Version was just released, so should be changed in 3…
2926e81 
….11.3 (pythonGH-101719)

(cherry picked from commit 0e0c5d8)

Co-authored-by: Steve Dower <steve.dower@python.org>
miss-islington added a commit that referenced this issue Feb 9, 2023
@miss-islington @zooba
gh-101283: Version was just released, so should be changed in 3.11.3 (G…
4a9dff0 
…H-101719)

(cherry picked from commit 0e0c5d8)

Co-authored-by: Steve Dower <steve.dower@python.org>
arhadthedev added a commit to arhadthedev/cpython that referenced this issue Feb 9, 2023
@arhadthedev
Fix the versionchanged of pythongh-101283 (3.12 only)
59e7be4
@bedevere-bot bedevere-bot mentioned this issue Feb 9, 2023
Merged
ambv pushed a commit that referenced this issue Feb 9, 2023
@miss-islington @arhadthedev @zooba
[3.9] gh-101283: Improved fallback logic for subprocess with shell=Tr…
04cc427 
…ue on Windows (GH-101286) (#101709)

Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>
Co-authored-by: Steve Dower <steve.dower@microsoft.com>
ambv pushed a commit that referenced this issue Feb 9, 2023
@miss-islington @arhadthedev @zooba
[3.8] gh-101283: Improved fallback logic for subprocess with shell=Tr…
32a1a61 
…ue on Windows (GH-101286) (#101710)

Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>
Co-authored-by: Steve Dower <steve.dower@microsoft.com>
ambv added a commit that referenced this issue Feb 9, 2023
@zooba @arhadthedev @ambv
[3.7] gh-101283: Improved fallback logic for subprocess with shell=Tr…
c7fdc9c 
…ue on Windows (GH-101286) (#101713)

Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
Co-authored-by: Oleg Iarygin <dralife@yandex.ru>
@zooba
Copy link
Member

zooba commented Feb 9, 2023

Fully merged, and all the fixes should be in (I hope). Thanks all!

@zooba zooba closed this as completed Feb 9, 2023
zooba pushed a commit that referenced this issue Feb 9, 2023
@arhadthedev
gh-101283: Fix 'versionchanged' for the shell=True fallback on Window…
6d92373 
…s in 3.12 (GH-101728)
carljm added a commit to carljm/cpython that referenced this issue Feb 9, 2023
@carljm
Merge branch 'main' into inlinecomp2
be3becc 
* main: (82 commits)
  pythongh-101670: typo fix in PyImport_ExtendInittab() (python#101723)
  pythonGH-99293: Document that `Py_TPFLAGS_VALID_VERSION_TAG` shouldn't be used. (#pythonGH-101736)
  no-issue: Add Dong-hee Na as the cjkcodecs codeowner (pythongh-101731)
  pythongh-101678: Merge math_1_to_whatever() and math_1() (python#101730)
  pythongh-101678: refactor the math module to use special functions from c11 (pythonGH-101679)
  pythongh-85984: Remove legacy Lib/pty.py code. (python#92365)
  pythongh-98831: Use opcode metadata for stack_effect() (python#101704)
  pythongh-101283: Version was just released, so should be changed in 3.11.3 (pythonGH-101719)
  pythongh-101283: Fix use of unbound variable (pythonGH-101712)
  pythongh-101283: Improved fallback logic for subprocess with shell=True on Windows (pythonGH-101286)
  pythongh-101277: Port more itertools static types to heap types (python#101304)
  pythongh-98831: Modernize CALL and family (python#101508)
  pythonGH-101696: invalidate type version tag in `_PyStaticType_Dealloc` (python#101697)
  pythongh-100221: Fix creating dirs in `make sharedinstall` (pythonGH-100329)
  pythongh-101670: typo fix in PyImport_AppendInittab() (pythonGH-101672)
  pythongh-101196: Make isdir/isfile/exists faster on Windows (pythonGH-101324)
  pythongh-101614: Don't treat python3_d.dll as a Python DLL when checking extension modules for incompatibility (pythonGH-101615)
  pythongh-100933: Improve `check_element` helper in `test_xml_etree` (python#100934)
  pythonGH-101578: Normalize the current exception (pythonGH-101607)
  pythongh-47937: Note that Popen attributes are read-only (python#93070)
  ...
carljm added a commit to carljm/cpython that referenced this issue Feb 10, 2023
@carljm
Merge branch 'main' into inlinecomp2
add772e 
* main:
  Fix some typos in asdl_c.py (pythonGH-101757)
  pythongh-101747: Fix refleak in new `OrderedDict` repr (pythonGH-101748)
  pythongh-101430: Update tracemalloc to handle presize properly. (pythongh-101745)
  pythonGH-101228: Fix typo in docstring for read method of `_io.TextIOWrapper` class (python#101227)
  Fix typo in `test_fstring.py` (python#101600)
  pythongh-101726: Update the OpenSSL version to 1.1.1t (pythonGH-101727)
  pythongh-101283: Fix 'versionchanged' for the shell=True fallback on Windows in 3.12 (pythonGH-101728)
  LibFFI build requires x64 Cygwin, and skip the ARM build (pythonGH-101743)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
3.7 3.8 3.9 3.10 3.11 OS-windows release-blocker type-bug An unexpected behavior, bug, or error type-security A security issue
Projects
Release and Deferred blockers
Status: Done
Milestone
No milestone
Development

No branches or pull requests
5 participants
@gpshead @eryksun @zooba @sobolevn @mauricelambert