-
-
Notifications
You must be signed in to change notification settings - Fork 31k
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
Comments
Starting proposal:
|
Is the situation really as vague as 'may be' (unless forced by |
@zooba Thanks for opening the issue! Regarding the wording, using subprocess.run([".\\test.bat", "&calc.exe"], shell=True) The above snippet will spawn the following process, which still executes
@terryjreedy It's always launched using |
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
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). |
I guess I can add that the main issue is that a |
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. |
In defence of this case, by passing the path to 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... 🤔 |
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. |
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:
|
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. |
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 |
…ythonGH-117996) (cherry picked from commit a4b44d3) Co-authored-by: Steve Dower <steve.dower@python.org>
…ythonGH-117996) (cherry picked from commit a4b44d3) Co-authored-by: Steve Dower <steve.dower@python.org>
…ythonGH-117996) (cherry picked from commit a4b44d3) Co-authored-by: Steve Dower <steve.dower@python.org>
…ythonGH-117996) (cherry picked from commit a4b44d3) Co-authored-by: Steve Dower <steve.dower@python.org>
…ythonGH-117996) (cherry picked from commit a4b44d3) Co-authored-by: Steve Dower <steve.dower@python.org>
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. |
I think #117996 made the doc https://docs.python.org/3/library/subprocess.html self-contradictory. First, regarding whether At https://docs.python.org/3/library/subprocess.html#subprocess.Popen, it says
But in https://docs.python.org/3/library/subprocess.html#security-considerations, it says
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:
But in the second paragraph, it says Python will do it:
In the source code, whether Lines 1459 to 1471 in 81c739e
Line 1538 in 81c739e
|
@Ry0taK's comment reminds me of PowerShell/PowerShell#1995 (comment). For example, I have a echo %* Regardless of whether import subprocess
subprocess.run([r'D:\test.cmd', 'test&calc.exe'])
subprocess.run([r'D:\test.cmd', 'test&calc.exe'], shell=True) When calling Line 1547 in 81c739e
|
Another interesting discovery: even if subprocess.run('echo "a"', shell=True) Line 1538 in 81c739e
args to
This didn't corrupt the command, and it still prints
|
Sidenotes for a more ridiculous case (a byproduct when composing Since 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 |
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 providedargv[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
The text was updated successfully, but these errors were encountered: