Skip to content

Commit a7c0264

Browse files
[3.5] bpo-30730: Prevent environment variables injection in subprocess on Windows. (GH-2325) (#2361)
Prevent passing other invalid environment variables and command arguments.. (cherry picked from commit d174d24)
1 parent f42ce17 commit a7c0264

File tree

5 files changed

+72
-9
lines changed

5 files changed

+72
-9
lines changed

Lib/subprocess.py

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1200,8 +1200,12 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
12001200
# and pass it to fork_exec()
12011201

12021202
if env is not None:
1203-
env_list = [os.fsencode(k) + b'=' + os.fsencode(v)
1204-
for k, v in env.items()]
1203+
env_list = []
1204+
for k, v in env.items():
1205+
k = os.fsencode(k)
1206+
if b'=' in k:
1207+
raise ValueError("illegal environment variable name")
1208+
env_list.append(k + b'=' + os.fsencode(v))
12051209
else:
12061210
env_list = None # Use execv instead of execve.
12071211
executable = os.fsencode(executable)

Lib/test/test_subprocess.py

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -634,6 +634,46 @@ def test_empty_env(self):
634634
# environment
635635
b"['__CF_USER_TEXT_ENCODING']"))
636636

637+
def test_invalid_cmd(self):
638+
# null character in the command name
639+
cmd = sys.executable + '\0'
640+
with self.assertRaises(ValueError):
641+
subprocess.Popen([cmd, "-c", "pass"])
642+
643+
# null character in the command argument
644+
with self.assertRaises(ValueError):
645+
subprocess.Popen([sys.executable, "-c", "pass#\0"])
646+
647+
def test_invalid_env(self):
648+
# null character in the enviroment variable name
649+
newenv = os.environ.copy()
650+
newenv["FRUIT\0VEGETABLE"] = "cabbage"
651+
with self.assertRaises(ValueError):
652+
subprocess.Popen([sys.executable, "-c", "pass"], env=newenv)
653+
654+
# null character in the enviroment variable value
655+
newenv = os.environ.copy()
656+
newenv["FRUIT"] = "orange\0VEGETABLE=cabbage"
657+
with self.assertRaises(ValueError):
658+
subprocess.Popen([sys.executable, "-c", "pass"], env=newenv)
659+
660+
# equal character in the enviroment variable name
661+
newenv = os.environ.copy()
662+
newenv["FRUIT=ORANGE"] = "lemon"
663+
with self.assertRaises(ValueError):
664+
subprocess.Popen([sys.executable, "-c", "pass"], env=newenv)
665+
666+
# equal character in the enviroment variable value
667+
newenv = os.environ.copy()
668+
newenv["FRUIT"] = "orange=lemon"
669+
with subprocess.Popen([sys.executable, "-c",
670+
'import sys, os;'
671+
'sys.stdout.write(os.getenv("FRUIT"))'],
672+
stdout=subprocess.PIPE,
673+
env=newenv) as p:
674+
stdout, stderr = p.communicate()
675+
self.assertEqual(stdout, b"orange=lemon")
676+
637677
def test_communicate_stdin(self):
638678
p = subprocess.Popen([sys.executable, "-c",
639679
'import sys;'

Misc/NEWS

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,9 @@ Extension Modules
5959
Library
6060
-------
6161

62+
- [Security] bpo-30730: Prevent environment variables injection in subprocess on
63+
Windows. Prevent passing other environment variables and command arguments.
64+
6265
- [Security] bpo-30694: Upgrade expat copy from 2.2.0 to 2.2.1 to get fixes
6366
of multiple security vulnerabilities including: CVE-2017-9233 (External
6467
entity infinite loop DoS), CVE-2016-9063 (Integer overflow, re-fix),

Modules/_winapi.c

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -744,6 +744,20 @@ getenvironment(PyObject* environment)
744744
"environment can only contain strings");
745745
goto error;
746746
}
747+
if (PyUnicode_FindChar(key, '\0', 0, PyUnicode_GET_LENGTH(key), 1) != -1 ||
748+
PyUnicode_FindChar(value, '\0', 0, PyUnicode_GET_LENGTH(value), 1) != -1)
749+
{
750+
PyErr_SetString(PyExc_ValueError, "embedded null character");
751+
goto error;
752+
}
753+
/* Search from index 1 because on Windows starting '=' is allowed for
754+
defining hidden environment variables. */
755+
if (PyUnicode_GET_LENGTH(key) == 0 ||
756+
PyUnicode_FindChar(key, '=', 1, PyUnicode_GET_LENGTH(key), 1) != -1)
757+
{
758+
PyErr_SetString(PyExc_ValueError, "illegal environment variable name");
759+
goto error;
760+
}
747761
if (totalsize > PY_SSIZE_T_MAX - PyUnicode_GET_LENGTH(key) - 1) {
748762
PyErr_SetString(PyExc_OverflowError, "environment too long");
749763
goto error;
@@ -830,7 +844,8 @@ _winapi_CreateProcess_impl(PyObject *module, Py_UNICODE *application_name,
830844
PROCESS_INFORMATION pi;
831845
STARTUPINFOW si;
832846
PyObject* environment;
833-
wchar_t *wenvironment;
847+
const wchar_t *wenvironment;
848+
Py_ssize_t wenvironment_size;
834849

835850
ZeroMemory(&si, sizeof(si));
836851
si.cb = sizeof(si);
@@ -846,12 +861,13 @@ _winapi_CreateProcess_impl(PyObject *module, Py_UNICODE *application_name,
846861

847862
if (env_mapping != Py_None) {
848863
environment = getenvironment(env_mapping);
849-
if (! environment)
864+
if (environment == NULL) {
850865
return NULL;
866+
}
867+
/* contains embedded null characters */
851868
wenvironment = PyUnicode_AsUnicode(environment);
852-
if (wenvironment == NULL)
853-
{
854-
Py_XDECREF(environment);
869+
if (wenvironment == NULL) {
870+
Py_DECREF(environment);
855871
return NULL;
856872
}
857873
}

Objects/abstract.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2825,8 +2825,8 @@ _PySequence_BytesToCharpArray(PyObject* self)
28252825
array[i] = NULL;
28262826
goto fail;
28272827
}
2828-
data = PyBytes_AsString(item);
2829-
if (data == NULL) {
2828+
/* check for embedded null bytes */
2829+
if (PyBytes_AsStringAndSize(item, &data, NULL) < 0) {
28302830
/* NULL terminate before freeing. */
28312831
array[i] = NULL;
28322832
goto fail;

0 commit comments

Comments
 (0)