classification
Title: [security] Injecting environment variable in subprocess on Windows
Type: security Stage: resolved
Components: Extension Modules Versions: Python 3.7, Python 3.6, Python 3.5, Python 3.4, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: benjamin.peterson, georg.brandl, haypo, larry, ned.deily, paul.moore, serhiy.storchaka, steve.dower, tim.golden, zach.ware
Priority: release blocker Keywords:

Created on 2017-06-22 08:07 by serhiy.storchaka, last changed 2017-07-22 19:22 by larry. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 2325 merged serhiy.storchaka, 2017-06-22 08:12
PR 2360 merged serhiy.storchaka, 2017-06-23 16:48
PR 2361 merged serhiy.storchaka, 2017-06-23 16:51
PR 2362 merged serhiy.storchaka, 2017-06-23 17:35
PR 2363 merged serhiy.storchaka, 2017-06-23 17:39
PR 2372 merged serhiy.storchaka, 2017-06-24 06:02
PR 2376 merged serhiy.storchaka, 2017-06-24 11:02
PR 2378 merged serhiy.storchaka, 2017-06-24 13:17
PR 2379 merged serhiy.storchaka, 2017-06-24 13:18
Messages (23)
msg296618 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-22 08:06
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.
msg296725 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-23 16:39
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
msg296728 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-23 17:17
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
msg296729 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-23 17:27
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
msg296753 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-24 05:08
3.3 and 3.4 starves from this issue
msg296760 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-24 08:49
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
msg296767 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-24 13:14
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
msg296769 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-24 13:28
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
msg296772 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-24 13:42
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
msg297175 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-28 12:15
Isn't "Type: security" enough? If you want you could patch Roundup for highlighting security issues.
msg297449 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2017-06-30 22:24
Steve, Paul: any comments on the severity of this issue and the pushed fixes?
msg297468 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2017-07-01 04:37
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.
msg297472 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-07-01 05:13
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.
msg297936 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2017-07-08 04:51
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
msg298145 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2017-07-11 10:02
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.
msg298146 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2017-07-11 10:18
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
msg298147 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2017-07-11 10:22
(never-mind, 3.6.1 still permits this, but I see that it's been fixed in trunk)
msg298148 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2017-07-11 10:24
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
msg298151 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-07-11 11:44
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.
msg298218 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-07-12 13:36
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
msg298222 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-07-12 14:41
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.
msg298628 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2017-07-19 02:40
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
msg298865 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2017-07-22 19:22
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
History
Date User Action Args
2017-07-22 19:22:34larrysetmessages: + msg298865
2017-07-19 05:17:56serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: backport needed -> resolved
2017-07-19 02:40:12ned.deilysetmessages: + msg298628
2017-07-12 14:41:57serhiy.storchakasetmessages: + msg298222
2017-07-12 13:36:34hayposetnosy: + haypo
messages: + msg298218
2017-07-11 11:44:14serhiy.storchakasetmessages: + msg298151
2017-07-11 10:24:12larrysetmessages: + msg298148
2017-07-11 10:22:45larrysetmessages: + msg298147
2017-07-11 10:18:10larrysetmessages: + msg298146
2017-07-11 10:02:57larrysetmessages: + msg298145
2017-07-08 04:51:40ned.deilysetmessages: + msg297936
2017-07-01 05:13:43serhiy.storchakasetmessages: + msg297472
2017-07-01 04:37:07steve.dowersetmessages: + msg297468
2017-06-30 22:24:57ned.deilysetpriority: normal -> release blocker
nosy: + ned.deily, larry, benjamin.peterson
messages: + msg297449

2017-06-28 12:15:18serhiy.storchakasetmessages: + msg297175
2017-06-28 09:23:53hayposettitle: Injecting environment variable in subprocess on Windows -> [security] Injecting environment variable in subprocess on Windows
2017-06-24 13:42:47serhiy.storchakasetmessages: + msg296772
2017-06-24 13:28:29serhiy.storchakasetmessages: + msg296769
2017-06-24 13:18:53serhiy.storchakasetpull_requests: + pull_request2429
2017-06-24 13:17:03serhiy.storchakasetpull_requests: + pull_request2427
2017-06-24 13:14:10serhiy.storchakasetmessages: + msg296767
2017-06-24 11:02:02serhiy.storchakasetpull_requests: + pull_request2424
2017-06-24 08:49:03serhiy.storchakasetmessages: + msg296760
2017-06-24 06:02:46serhiy.storchakasetpull_requests: + pull_request2420
2017-06-24 05:08:38serhiy.storchakasetmessages: + msg296753
versions: + Python 2.7
2017-06-23 17:42:36serhiy.storchakasetnosy: + georg.brandl
stage: patch review -> backport needed

versions: + Python 3.3, Python 3.4, - Python 2.7
2017-06-23 17:39:54serhiy.storchakasetpull_requests: + pull_request2412
2017-06-23 17:35:21serhiy.storchakasetpull_requests: + pull_request2411
2017-06-23 17:27:04serhiy.storchakasetmessages: + msg296729
2017-06-23 17:17:40serhiy.storchakasetmessages: + msg296728
2017-06-23 16:51:34serhiy.storchakasetpull_requests: + pull_request2410
2017-06-23 16:48:07serhiy.storchakasetpull_requests: + pull_request2409
2017-06-23 16:39:29serhiy.storchakasetmessages: + msg296725
2017-06-23 16:18:56serhiy.storchakasetassignee: serhiy.storchaka
2017-06-22 08:12:03serhiy.storchakasetpull_requests: + pull_request2374
2017-06-22 08:07:00serhiy.storchakacreate