[3.6] bpo-30730: Prevent environment variables injection in subprocess on Windows...

Prevent passing other invalid environment variables and command arguments..
(cherry picked from commit d174d24a5d37d1516b885dc7c82f71ecd5930700)

diff --git a/Lib/subprocess.py b/Lib/subprocess.py
index e626a8afdbb7f07288818416f701d935ef17e5d7..b790cbd65ba20848b86b93ad623f72b5d04d5973 100644 (file)
--- 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 d8d6d82713cdeda296198d329007491bfbe560cb..9a9e3ca579125634ee2403dcc4a2cba6fc8d8d57 100644 (file)
--- 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 70a531c76f371120d2454054fec45ca43df82ac1..94a29d0363c6adee4bca29a3ffc7b8b6ee14b0f8 100644 (file)
--- 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 248f4582c60fb40504e2b4954d116ca4bba0a3d8..71426bc12dc655371832161ed649ac934eda33ad 100644 (file)
--- 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 3585b34ca4d8bdf38c5f146d805331f993429748..38de774de3a4db6dd0ab859cd226b50584d8eb71 100644 (file)
--- 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;