diff options
| -rw-r--r-- | Lib/subprocess.py | 8 | ||||
| -rw-r--r-- | Lib/test/test_subprocess.py | 40 | ||||
| -rw-r--r-- | Misc/NEWS | 3 | ||||
| -rw-r--r-- | Modules/_winapi.c | 26 | ||||
| -rw-r--r-- | Objects/abstract.c | 4 |
5 files changed, 72 insertions, 9 deletions
diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 3241758..99bca47 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -1238,8 +1238,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 52b05c1..4c6e159 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -655,6 +655,46 @@ class ProcessTestCase(BaseTestCase): 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;' @@ -374,6 +374,9 @@ Extension Modules Library ------- +- [Security] bpo-30730: Prevent environment variables injection in subprocess on + Windows. Prevent passing other environment variables and command arguments. + - bpo-21071: struct.Struct.format type is now :class:`str` instead of :class:`bytes`. diff --git a/Modules/_winapi.c b/Modules/_winapi.c index 8d01b37..671f2dd 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 0f1ee9d..cb026c0 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; |