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

[doc] subprocess security considerations needs a Windows-specific exception #114539

Closed
zooba opened this issue Jan 24, 2024 · 16 comments
Closed

[doc] subprocess security considerations needs a Windows-specific exception #114539

zooba opened this issue Jan 24, 2024 · 16 comments
Labels
3.8 (EOL) end of life 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes 3.12 bugs and security fixes 3.13 bugs and security fixes docs Documentation in the Doc dir type-security A security issue

Comments

@zooba
Copy link
Member

zooba commented Jan 24, 2024

The documentation at https://docs.python.org/3/library/subprocess.html#security-considerations says that "this implementation will never implicitly call a system shell".

While this is technically true, on Windows the underlying CreateProcess API may create a system shell, which then exposes arguments to shell parsing. This happens when passed a .bat or .cmd file.

PSRT review of the issue determined that we can't safely detect and handle this situation without causing new issues and making it more complex for users to work around when they want to intentionally launch a batch file without shell processing. For the two cases of untrusted input, an untrusted application/argv[0] is already vulnerable, and an untrusted argument/argv[1:] is safe provided argv[0] is controlled. However, we do need to inform developers of the inconsistency so they can check their own use.

We'll use this issue to ensure we get good wording. First proposal in the next comment.

Thanks to RyotaK for reporting responsibly to the Python Security Response Team.

Linked PRs

@zooba zooba added docs Documentation in the Doc dir type-security A security issue 3.11 only security fixes 3.10 only security fixes 3.9 only security fixes 3.8 (EOL) end of life 3.12 bugs and security fixes 3.13 bugs and security fixes labels Jan 24, 2024
@zooba
Copy link
Member Author

zooba commented Jan 24, 2024

Starting proposal:

On Windows, batch files (*.bat or *.cmd) may be launched by the operating system in a system shell. This could result in arguments being parsed according to shell rules, but without any escaping added by Python. If you are intentionally launching a batch file with arguments from untrusted sources, consider passing shell=True to allow Python to escape special characters.

@terryjreedy
Copy link
Member

Is the situation really as vague as 'may be' (unless forced by shell=True? Is it repeatable (deterministic) for the same call?

@Ry0taK
Copy link

Ry0taK commented Jan 25, 2024

@zooba Thanks for opening the issue!

Regarding the wording, using shell=True doesn't escape arguments properly.

subprocess.run([".\\test.bat", "&calc.exe"], shell=True)

The above snippet will spawn the following process, which still executes calc.exe:

C:\WINDOWS\system32\cmd.exe /c ".\test.bat &calc.exe"

@terryjreedy It's always launched using cmd.exe because batch files can't be parsed without it

@zooba
Copy link
Member Author

zooba commented Jan 25, 2024

Is the situation really as vague as 'may be' (unless forced by shell=True? Is it repeatable (deterministic) for the same call?

If we assume that the file system can change at any time, then it's not deterministic, even for the same call. But assuming nobody modifies the file you are executing, it should always use the same mechanism.

I do know more details about when/why it might vary, but we don't want to document them. There's also always the potential that Microsoft could change the behaviour and/or add an option to change the behaviour, and we don't want our docs to be invalidated by that. The main aim is to encourage users to pass shell=True if they know they're launching a batch file, and to be clear that we won't escape anything if they launch a batch file with shell=False.

Regarding the wording, using shell=True doesn't escape arguments properly.

I know we've discussed this at various points over the years... it ought to be a long-standing bug if it's not working right now. But we might need to make sure we're correctly handling command characters (and compatibly across platforms).

@zooba
Copy link
Member Author

zooba commented Jan 25, 2024

I do know more details about when/why it might vary, but we don't want to document them.

I guess I can add that the main issue is that a .bat or .cmd extension doesn't guarantee that it's actually a batch file, and that factors into how it may be run. If the extension is one of those and it's actually a batch file with no deliberate tricks, it'll always be run under cmd.exe.

@gpshead
Copy link
Member

gpshead commented Jan 25, 2024

Yep. IIRC Windows and related software has had a notorious number of security bugs related to unfortunate magic file type detection based on contents rather than what the name, extension, or mime type says... We can't prevent a fundamental platform issue on the Python side.

@zooba
Copy link
Member Author

zooba commented Jan 25, 2024

In defence of this case, by passing the path to CreateProcess you're kind of implying that you think it's an executable (akin to CPython's concrete C API). Do the same thing with other APIs and it'll use different tests (e.g. ShellExecute is more like our abstract C API).

But it's the fallback behaviour in this (and other) cases that cause problems. Which might also be why I'm instinctively against adding "abstract" behaviour to our concrete C APIs... 🤔

@gpshead
Copy link
Member

gpshead commented Apr 10, 2024

FYI - here's what Rust did implementation wise: rust-lang/rust@f66a096

For the few languages that chose to attempt to prevent users from getting into this Windows messy area, I believe they're landing those changes in public now-ish. It'll be interesting to watch how those attempts fare in practice.

@zooba
Copy link
Member Author

zooba commented Apr 11, 2024

Thanks Greg. We should probably get our doc update in.

As a reminder, this is my proposed addition, and there have been no changes yet:

On Windows, batch files (*.bat or *.cmd) may be launched by the operating system in a system shell. This could result in arguments being parsed according to shell rules, but without any escaping added by Python. If you are intentionally launching a batch file with arguments from untrusted sources, consider passing shell=True to allow Python to escape special characters.

@gpshead
Copy link
Member

gpshead commented Apr 11, 2024

I believe the coordinated disclosure and CVEs for various languages just landed. The original reporter has a public blog post now: https://flatt.tech/research/posts/batbadbut-you-cant-securely-execute-commands-on-windows/ Link to that from the doc note if deemed appropriate, or at least back to this GH issue so people who do want mitigation within Python have a singular place to chime in.

@gpshead
Copy link
Member

gpshead commented Apr 11, 2024

Rust at least happily mentions that their mitigations cannot handle all cases: https://blog.rust-lang.org/2024/04/09/cve-2024-24576.html#mitigations

miss-islington pushed a commit to miss-islington/cpython that referenced this issue Apr 17, 2024
…ythonGH-117996)

(cherry picked from commit a4b44d3)

Co-authored-by: Steve Dower <steve.dower@python.org>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Apr 17, 2024
…ythonGH-117996)

(cherry picked from commit a4b44d3)

Co-authored-by: Steve Dower <steve.dower@python.org>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Apr 17, 2024
…ythonGH-117996)

(cherry picked from commit a4b44d3)

Co-authored-by: Steve Dower <steve.dower@python.org>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Apr 17, 2024
…ythonGH-117996)

(cherry picked from commit a4b44d3)

Co-authored-by: Steve Dower <steve.dower@python.org>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Apr 17, 2024
…ythonGH-117996)

(cherry picked from commit a4b44d3)

Co-authored-by: Steve Dower <steve.dower@python.org>
zooba added a commit that referenced this issue Apr 17, 2024
…H-117996) (#118002)

gh-114539: Clarify implicit launching of shells by subprocess (GH-117996)
(cherry picked from commit a4b44d3)

Co-authored-by: Steve Dower <steve.dower@python.org>
@zooba
Copy link
Member Author

zooba commented Apr 17, 2024

Main change is committed to active branches - security fix branches will be merged by the RMs. There's no further need to keep this open.

If issues in shell quoting are discovered (they will always exist, FWIW), please open a new issue.

@zooba zooba closed this as completed Apr 17, 2024
ambv pushed a commit that referenced this issue May 7, 2024
…H-117996) (GH-118004)

(cherry picked from commit a4b44d3)

Co-authored-by: Steve Dower <steve.dower@python.org>
ambv pushed a commit that referenced this issue May 7, 2024
…H-117996) (GH-118005)

(cherry picked from commit a4b44d3)

Co-authored-by: Steve Dower <steve.dower@python.org>
ambv pushed a commit that referenced this issue May 7, 2024
…H-117996) (GH-118006)

(cherry picked from commit a4b44d3)

Co-authored-by: Steve Dower <steve.dower@python.org>
@jiasli
Copy link
Contributor

jiasli commented Aug 8, 2024

I think #117996 made the doc https://docs.python.org/3/library/subprocess.html self-contradictory.

First, regarding whether shell=True should be used to call a batch file.

At https://docs.python.org/3/library/subprocess.html#subprocess.Popen, it says shell=True should not be used:

You do not need shell=True to run a batch file or console-based executable.

But in https://docs.python.org/3/library/subprocess.html#security-considerations, it says shell=True should be used:

If you are intentionally launching a batch file with arguments from untrusted sources, consider passing shell=True to allow Python to escape special characters.

Second, regarding who should escape special characters.

In the first paragraph of https://docs.python.org/3/library/subprocess.html#security-considerations, it says the application should do it:

If the shell is invoked explicitly, via shell=True, it is the application’s responsibility to ensure that all whitespace and metacharacters are quoted appropriately to avoid shell injection vulnerabilities.

But in the second paragraph, it says Python will do it:

If you are intentionally launching a batch file with arguments from untrusted sources, consider passing shell=True to allow Python to escape special characters.

In the source code, whether list2cmdline is invoked is determined by the type of args, not shell=True:

cpython/Lib/subprocess.py

Lines 1459 to 1471 in 81c739e

if isinstance(args, str):
pass
elif isinstance(args, bytes):
if shell:
raise TypeError('bytes args is not allowed on Windows')
args = list2cmdline([args])
elif isinstance(args, os.PathLike):
if shell:
raise TypeError('path-like args is not allowed when '
'shell is true')
args = list2cmdline([args])
else:
args = list2cmdline(args)

shell=True only quotes args with ":

args = '{} /c "{}"'.format (comspec, args)

@jiasli
Copy link
Contributor

jiasli commented Aug 8, 2024

@Ry0taK's comment reminds me of PowerShell/PowerShell#1995 (comment).

For example, I have a D:\test.cmd:

echo %*

Regardless of whether shell=True is used, calc.exe is launched:

import subprocess

subprocess.run([r'D:\test.cmd', 'test&calc.exe'])
subprocess.run([r'D:\test.cmd', 'test&calc.exe'], shell=True)

When calling CreateProcess:

hp, ht, pid, tid = _winapi.CreateProcess(executable, args,

  • Without shell=True, args is D:\test.cmd test&calc.exe
  • With shell=True, args is C:\Windows\system32\cmd.exe /c "D:\test.cmd test&calc.exe"

@jiasli
Copy link
Contributor

jiasli commented Aug 8, 2024

Another interesting discovery: even if args contains double quotes ", for example:

subprocess.run('echo "a"', shell=True)

args = '{} /c "{}"'.format (comspec, args)
will set args to

C:\Windows\system32\cmd.exe /c "echo "a""

This didn't corrupt the command, and it still prints

"a"

@y5c4l3
Copy link
Contributor

y5c4l3 commented Sep 28, 2024

Sidenotes for a more ridiculous case (a byproduct when composing venv test cases):

Since & is among valid filename charset on Windows, in theory you can have executable files called &.bat and &.exe

import subprocess
out = subprocess.check_output(['&.exe', '-echoargs', '1']) # fine
print(out)
out = subprocess.check_output(['&.bat', '-echoargs', '1']) # will never succeed
print(out)
# workaround by constructing cmdline manually
subprocess.check_output('"&.bat"')

The funniest thing is that after they peek argv[0] and notice it's a Batch script, they implicitly change the cmdline mode which breaks the atomicity of argv[0] and contradicts the original peek.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.8 (EOL) end of life 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes 3.12 bugs and security fixes 3.13 bugs and security fixes docs Documentation in the Doc dir type-security A security issue
Projects
None yet
Development

No branches or pull requests

6 participants