From 5c9d5764b19faac9e9b5858461de3a7209f82d80 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Thu, 22 Jun 2017 11:10:07 +0300 Subject: [PATCH 1/2] bpo-30730: Prevent environment variables injection in subprocess on Windows. Prevent passing other environment variables and command arguments. --- Lib/subprocess.py | 8 ++++++-- Lib/test/test_subprocess.py | 40 +++++++++++++++++++++++++++++++++++++ Misc/NEWS | 3 +++ Modules/_winapi.c | 22 +++++++++++++++----- Objects/abstract.c | 4 ++-- 5 files changed, 68 insertions(+), 9 deletions(-) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 32417584528e0b..99bca477cecdab 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -1238,8 +1238,12 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, # 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 52b05c12b641da..4c6e1596da9a4a 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -655,6 +655,46 @@ def is_env_var_to_ignore(n): if not is_env_var_to_ignore(k)] self.assertEqual(child_env_names, []) + 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 8e25d62d89377b..558d33a1c53b02 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -371,6 +371,9 @@ Extension Modules Library ------- +- [Security] bpo-30730: Prevent environment variables injection in subprocess on + Windows. Prevent passing other environment variables and command arguments. + - bpo-29212: Fix concurrent.futures.thread.ThreadPoolExecutor threads to have a non repr() based thread name by default when no thread_name_prefix is supplied. They will now identify themselves as "ThreadPoolExecutor-y_n". diff --git a/Modules/_winapi.c b/Modules/_winapi.c index 8d01b376f2597c..3fc2780704c426 100644 --- a/Modules/_winapi.c +++ b/Modules/_winapi.c @@ -744,6 +744,16 @@ 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; + } + if (PyUnicode_FindChar(key, '=', 0, 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 +840,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 +857,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 0f1ee9dbbaf545..cb026c0df5daf6 100644 --- a/Objects/abstract.c +++ b/Objects/abstract.c @@ -2558,8 +2558,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; From b92de1154ece0b1d8fcf6b6a03bc1a3f908dd1af Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Thu, 22 Jun 2017 15:17:43 +0300 Subject: [PATCH 2/2] Allows starting '=' in environment variable on Windows. --- Modules/_winapi.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Modules/_winapi.c b/Modules/_winapi.c index 3fc2780704c426..671f2dddb1ccdb 100644 --- a/Modules/_winapi.c +++ b/Modules/_winapi.c @@ -750,7 +750,11 @@ getenvironment(PyObject* environment) PyErr_SetString(PyExc_ValueError, "embedded null character"); goto error; } - if (PyUnicode_FindChar(key, '=', 0, PyUnicode_GET_LENGTH(key), 1) != -1) { + /* 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; }