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
[security] Injecting environment variable in subprocess on Windows #74915
Comments
It is possible to inject an environment variable in subprocess on Windows if a user data is passed to a subprocess via environment variable. Provided PR fixes this vulnerability. It also adds other checks for invalid environment (variable names containing '=') and command arguments (containing '\0'). This was a part of bpo-13617, but extracted to a separate issue due to increased severity. |
3.3 and 3.4 starves from this issue |
Isn't "Type: security" enough? If you want you could patch Roundup for highlighting security issues. |
Steve, Paul: any comments on the severity of this issue and the pushed fixes? |
It's certainly exploitable for remote code execution if user data allows embedded nulls (can you URL encode %00?). The fixes look fine and shouldn't cause any new issues, though I thought that fsencode() already rejected embedded nulls - maybe I'm thinking of the argument converter though, which is not invoked here. |
Yes, fsencode() already rejected embedded nulls, that is why the Posix branch doesn't need additional check for null characters. The Posix branch was changed only for adding the check for the '=' character in names. |
New changeset a9b16cf by Ned Deily (Serhiy Storchaka) in branch '3.6': New changeset d1d6501 by Ned Deily (Serhiy Storchaka) in branch '3.6': |
Serhiy, I don't see where you got a full review of this change. Eryksun reviewed the code and asked for changes; you made the he asked for changes but didn't get any further review. Nor did you get a full review / "looks good to me" from anybody. As a matter of policy I do want to see reviews for security changes on Windows. I've asked Steve Dower to give it a quick review. |
It seems that os.execve() still permits this, even on Windows. Shouldn't we solve it there too? (Thanks to Steve Dower for realizing this.) -- import os
cmdline=["/usr/bin/printenv"]
env={'a=b': 'c'}
os.execve(cmdline[0], cmdline, env)
# this prints a=b=c |
(never-mind, 3.6.1 still permits this, but I see that it's been fixed in trunk) |
Sorry, actually the patch fixed two bugs. The one of them is a security issue, the other is much more severe. They look similar, are related to the same code (on Windows) and are tested with similar tests. os.execve() was not vulnerable to the first issue, it suffered only from the less severe bug. It was fixed in the separate issue (see bpo-30746). I don't think that should be backported to 3.4. |
I rebased my "[3.4] Backport CI config from master" PR bpo-2475 on top of 3.4 to test the new security fixes, but a few test_subprocess tests failed: #2475 ====================================================================== ERROR: test_invalid_cmd (test.test_subprocess.ProcessTestCase) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/travis/build/python/cpython/Lib/test/test_subprocess.py", line 613, in test_invalid_cmd subprocess.Popen([cmd, "-c", "pass"]) File "/home/travis/build/python/cpython/Lib/subprocess.py", line 856, in __init__
File "/home/travis/build/python/cpython/Lib/subprocess.py", line 1402, in _execute_child
TypeError: expected bytes with no null |
Oh, I forgot that null character/byte errors were of type TypeError before 3.5. The simplest fix is changing corresponding ValueError in self.assertRaises() to the tuple (ValueError, TypeError). I have updated the PR for 3.5. You can include the fix in your "[3.4] Backport CI config from master" PR or I can create a separate PR for 3.4. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: