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.

New changeset d174d24 by Serhiy Storchaka in branch 'master':
bpo-30730: Prevent environment variables injection in subprocess on Windows. (bpo-2325)
d174d24

New changeset e713575 by Serhiy Storchaka in branch '3.6':
[3.6] bpo-30730: Prevent environment variables injection in subprocess on Windows. (GH-2325) (bpo-2360)
e713575

New changeset a7c0264 by Serhiy Storchaka in branch '3.5':
[3.5] bpo-30730: Prevent environment variables injection in subprocess on Windows. (GH-2325) (bpo-2361)
a7c0264

3.3 and 3.4 starves from this issue

New changeset 9dda2ca by Serhiy Storchaka in branch '2.7':
[2.7] bpo-30730: Prevent environment variables injection in subprocess on Windows. (GH-2325) (bpo-2372)
9dda2ca

New changeset 0ee32c1 by Serhiy Storchaka in branch 'master':
bpo-30745: Fix compiler warnings introduced in bpo-30730. (bpo-2376)
0ee32c1

New changeset 0e1f9e8 by Serhiy Storchaka in branch '3.6':
[3.6] bpo-30745: Fix compiler warnings introduced in bpo-30730. (GH-2376) (bpo-2378)
0e1f9e8

New changeset e0d446e by Serhiy Storchaka in branch '3.5':
[3.5] bpo-30745: Fix compiler warnings introduced in bpo-30730. (GH-2376) (bpo-2379)
e0d446e

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':
[3.6] bpo-30730: Prevent environment variables injection in subprocess on Windows. (GH-2325) (bpo-2360)
a9b16cf

New changeset d1d6501 by Ned Deily (Serhiy Storchaka) in branch '3.6':
[3.6] bpo-30745: Fix compiler warnings introduced in bpo-30730. (GH-2376) (bpo-2378)
d1d6501

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)

New changeset fe82c46 by larryhastings (Serhiy Storchaka) in branch '3.4':
[security][3.4] bpo-30730: Prevent environment variables injection in subprocess on Windows. (GH-2325) (bpo-2362)
fe82c46

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
https://travis-ci.org/python/cpython/jobs/252804589

======================================================================

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__

restore_signals, start_new_session)

File "/home/travis/build/python/cpython/Lib/subprocess.py", line 1402, in _execute_child

restore_signals, start_new_session, preexec_fn)

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.

New changeset e46f1c1 by Ned Deily (Serhiy Storchaka) in branch '3.3':
[security][3.3] bpo-30730: Prevent environment variables injection in subprocess on Windows. (GH-2325) (bpo-2363)
e46f1c1

New changeset b154917 by larryhastings (Victor Stinner) in branch '3.4':
[3.4] Backport CI config from master (bpo-2475)
b154917