From a9b16cff35811f88cdfeb4f50758140dfff36ebc Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Fri, 23 Jun 2017 20:17:38 +0300 Subject: [PATCH] [3.6] bpo-30730: Prevent environment variables injection in subprocess on Windows. (GH-2325) (#2360) Prevent passing other invalid environment variables and command arguments.. (cherry picked from commit d174d24a5d37d1516b885dc7c82f71ecd5930700) (cherry picked from commit e7135751b8e48af80665e40ac8fa6d0073e5affe) --- Lib/subprocess.py | 8 ++++++-- Lib/test/test_subprocess.py | 40 +++++++++++++++++++++++++++++++++++++ Misc/NEWS | 3 +++ Modules/_winapi.c | 26 +++++++++++++++++++----- Objects/abstract.c | 4 ++-- 5 files changed, 72 insertions(+), 9 deletions(-) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index e626a8afdbb..b790cbd65ba 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -1239,8 +1239,12 @@ class Popen(object): # and pass it to fork_exec() if env is not None: - env_list = [os.fsencode(k) + b'=' + os.fsencode(v) - for k, v in env.items()] + env_list = [] + for k, v in env.items(): + k = os.fsencode(k) + if b'=' in k: + raise ValueError("illegal environment variable name") + env_list.append(k + b'=' + os.fsencode(v)) else: env_list = None # Use execv instead of execve. executable = os.fsencode(executable) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index d8d6d82713c..9a9e3ca5791 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -644,6 +644,46 @@ class ProcessTestCase(BaseTestCase): # environment b"['__CF_USER_TEXT_ENCODING']")) + def test_invalid_cmd(self): + # null character in the command name + cmd = sys.executable + '\0' + with self.assertRaises(ValueError): + subprocess.Popen([cmd, "-c", "pass"]) + + # null character in the command argument + with self.assertRaises(ValueError): + subprocess.Popen([sys.executable, "-c", "pass#\0"]) + + def test_invalid_env(self): + # null character in the enviroment variable name + newenv = os.environ.copy() + newenv["FRUIT\0VEGETABLE"] = "cabbage" + with self.assertRaises(ValueError): + subprocess.Popen([sys.executable, "-c", "pass"], env=newenv) + + # null character in the enviroment variable value + newenv = os.environ.copy() + newenv["FRUIT"] = "orange\0VEGETABLE=cabbage" + with self.assertRaises(ValueError): + subprocess.Popen([sys.executable, "-c", "pass"], env=newenv) + + # equal character in the enviroment variable name + newenv = os.environ.copy() + newenv["FRUIT=ORANGE"] = "lemon" + with self.assertRaises(ValueError): + subprocess.Popen([sys.executable, "-c", "pass"], env=newenv) + + # equal character in the enviroment variable value + newenv = os.environ.copy() + newenv["FRUIT"] = "orange=lemon" + with subprocess.Popen([sys.executable, "-c", + 'import sys, os;' + 'sys.stdout.write(os.getenv("FRUIT"))'], + stdout=subprocess.PIPE, + env=newenv) as p: + stdout, stderr = p.communicate() + self.assertEqual(stdout, b"orange=lemon") + def test_communicate_stdin(self): p = subprocess.Popen([sys.executable, "-c", 'import sys;' diff --git a/Misc/NEWS b/Misc/NEWS index 1f6161a1f10..136c84fb9fe 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -13,6 +13,9 @@ Core and Builtins Library ------- +- [Security] bpo-30730: Prevent environment variables injection in subprocess on + Windows. Prevent passing other environment variables and command arguments. + - [Security] bpo-30694: Upgrade expat copy from 2.2.0 to 2.2.1 to get fixes of multiple security vulnerabilities including: CVE-2017-9233 (External entity infinite loop DoS), CVE-2016-9063 (Integer overflow, re-fix), diff --git a/Modules/_winapi.c b/Modules/_winapi.c index 248f4582c60..71426bc12dc 100644 --- a/Modules/_winapi.c +++ b/Modules/_winapi.c @@ -744,6 +744,20 @@ getenvironment(PyObject* environment) "environment can only contain strings"); goto error; } + if (PyUnicode_FindChar(key, '\0', 0, PyUnicode_GET_LENGTH(key), 1) != -1 || + PyUnicode_FindChar(value, '\0', 0, PyUnicode_GET_LENGTH(value), 1) != -1) + { + PyErr_SetString(PyExc_ValueError, "embedded null character"); + goto error; + } + /* Search from index 1 because on Windows starting '=' is allowed for + defining hidden environment variables. */ + if (PyUnicode_GET_LENGTH(key) == 0 || + PyUnicode_FindChar(key, '=', 1, PyUnicode_GET_LENGTH(key), 1) != -1) + { + PyErr_SetString(PyExc_ValueError, "illegal environment variable name"); + goto error; + } if (totalsize > PY_SSIZE_T_MAX - PyUnicode_GET_LENGTH(key) - 1) { PyErr_SetString(PyExc_OverflowError, "environment too long"); goto error; @@ -830,7 +844,8 @@ _winapi_CreateProcess_impl(PyObject *module, Py_UNICODE *application_name, PROCESS_INFORMATION pi; STARTUPINFOW si; PyObject* environment; - wchar_t *wenvironment; + const wchar_t *wenvironment; + Py_ssize_t wenvironment_size; ZeroMemory(&si, sizeof(si)); si.cb = sizeof(si); @@ -846,12 +861,13 @@ _winapi_CreateProcess_impl(PyObject *module, Py_UNICODE *application_name, if (env_mapping != Py_None) { environment = getenvironment(env_mapping); - if (! environment) + if (environment == NULL) { return NULL; + } + /* contains embedded null characters */ wenvironment = PyUnicode_AsUnicode(environment); - if (wenvironment == NULL) - { - Py_XDECREF(environment); + if (wenvironment == NULL) { + Py_DECREF(environment); return NULL; } } diff --git a/Objects/abstract.c b/Objects/abstract.c index 3585b34ca4d..38de774de3a 100644 --- a/Objects/abstract.c +++ b/Objects/abstract.c @@ -3193,8 +3193,8 @@ _PySequence_BytesToCharpArray(PyObject* self) array[i] = NULL; goto fail; } - data = PyBytes_AsString(item); - if (data == NULL) { + /* check for embedded null bytes */ + if (PyBytes_AsStringAndSize(item, &data, NULL) < 0) { /* NULL terminate before freeing. */ array[i] = NULL; goto fail;