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 issue13617, but extracted to a separate issue due to increased severity.

New changeset d174d24a5d37d1516b885dc7c82f71ecd5930700 by Serhiy Storchaka in branch 'master':
bpo-30730: Prevent environment variables injection in subprocess on Windows. (#2325)
https://github.com/python/cpython/commit/d174d24a5d37d1516b885dc7c82f71ecd5930700

New changeset e7135751b8e48af80665e40ac8fa6d0073e5affe by Serhiy Storchaka in branch '3.6':
[3.6] bpo-30730: Prevent environment variables injection in subprocess on Windows. (GH-2325) (#2360)
https://github.com/python/cpython/commit/e7135751b8e48af80665e40ac8fa6d0073e5affe

New changeset a7c0264735f46afab13771be4218d8eab0d7dc91 by Serhiy Storchaka in branch '3.5':
[3.5] bpo-30730: Prevent environment variables injection in subprocess on Windows. (GH-2325) (#2361)
https://github.com/python/cpython/commit/a7c0264735f46afab13771be4218d8eab0d7dc91

3.3 and 3.4 starves from this issue

New changeset 9dda2caca8edc7ff1285f6b0d1c5279b51854b7d by Serhiy Storchaka in branch '2.7':
[2.7] bpo-30730: Prevent environment variables injection in subprocess on Windows. (GH-2325) (#2372)
https://github.com/python/cpython/commit/9dda2caca8edc7ff1285f6b0d1c5279b51854b7d

New changeset 0ee32c148119031e19c79359f5c4789ee69fa355 by Serhiy Storchaka in branch 'master':
bpo-30745: Fix compiler warnings introduced in bpo-30730. (#2376)
https://github.com/python/cpython/commit/0ee32c148119031e19c79359f5c4789ee69fa355

New changeset 0e1f9e8d3ea82262cbb9a403b70a884da5e6a6ac by Serhiy Storchaka in branch '3.6':
[3.6] bpo-30745: Fix compiler warnings introduced in bpo-30730. (GH-2376) (#2378)
https://github.com/python/cpython/commit/0e1f9e8d3ea82262cbb9a403b70a884da5e6a6ac

New changeset e0d446e9caa38923e43818f78c94f95fe0aa995e by Serhiy Storchaka in branch '3.5':
[3.5] bpo-30745: Fix compiler warnings introduced in bpo-30730. (GH-2376) (#2379)
https://github.com/python/cpython/commit/e0d446e9caa38923e43818f78c94f95fe0aa995e

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 a9b16cff35811f88cdfeb4f50758140dfff36ebc by Ned Deily (Serhiy Storchaka) in branch '3.6':
[3.6] bpo-30730: Prevent environment variables injection in subprocess on Windows. (GH-2325) (#2360)
https://github.com/python/cpython/commit/a9b16cff35811f88cdfeb4f50758140dfff36ebc

New changeset d1d65015fca44b8d1f0b1df78694310270f03a6d by Ned Deily (Serhiy Storchaka) in branch '3.6':
[3.6] bpo-30745: Fix compiler warnings introduced in bpo-30730. (GH-2376) (#2378)
https://github.com/python/cpython/commit/d1d65015fca44b8d1f0b1df78694310270f03a6d

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 fe82c46327effc124ff166e1fa1e611579e1176b by larryhastings (Serhiy Storchaka) in branch '3.4':
[security][3.4] bpo-30730: Prevent environment variables injection in subprocess on Windows. (GH-2325) (#2362)
https://github.com/python/cpython/commit/fe82c46327effc124ff166e1fa1e611579e1176b

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 issue30746). I don't think that should be backported to 3.4.

I rebased my "[3.4] Backport CI config from master" PR #2475 on top of 3.4 to test the new security fixes, but a few test_subprocess tests failed:

https://github.com/python/cpython/pull/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 e46f1c19642ea1882f427d8246987ba49351a97d by Ned Deily (Serhiy Storchaka) in branch '3.3':
[security][3.3] bpo-30730: Prevent environment variables injection in subprocess on Windows. (GH-2325) (#2363)
https://github.com/python/cpython/commit/e46f1c19642ea1882f427d8246987ba49351a97d

New changeset b1549175ed30f2931e2bb980a7e3c360ed19e1c9 by larryhastings (Victor Stinner) in branch '3.4':
[3.4] Backport CI config from master (#2475)
https://github.com/python/cpython/commit/b1549175ed30f2931e2bb980a7e3c360ed19e1c9