[security][3.3] bpo-30730: Prevent environment variables injection in subprocess on Windows. (GH-2325) (#2363)

Author: serhiy-storchaka

Lib/subprocess.py

@@ -1363,8 +1363,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)

Lib/test/test_subprocess.py

@@ -588,6 +588,46 @@ def test_empty_env(self):
  # 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, TypeError)):
+ subprocess.Popen([cmd, "-c", "pass"])
+
+ # null character in the command argument
+ with self.assertRaises((ValueError, TypeError)):
+ 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, TypeError)):
+ 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, TypeError)):
+ 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;'

Misc/NEWS

@@ -35,6 +35,9 @@ Core and Builtins
 Library
 -------
 
+- [Security] bpo-30730: Prevent environment variables injection in subprocess on
+ Windows. Prevent passing other invalid environment variables and command arguments.
+
 - [Security] bpo-30585: Fix TLS stripping vulnerability in smptlib,
  CVE-2016-0772. Reported by Team Oststrom
 

Modules/_winapi.c

@@ -513,6 +513,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;
@@ -621,12 +635,13 @@ winapi_CreateProcess(PyObject* self, PyObject* args)
 
  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;
  }
  }

Objects/abstract.c

@@ -2682,8 +2682,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;