8000 [security][3.3] bpo-30730: Prevent environment variables injection in… · python/cpython@e46f1c1 · GitHub
[go: up one dir, main page]
Skip to content

Commit e46f1c1

Browse files
serhiy-storchakaned-deily
serhiy-storchaka
authored and
ned-deily
committed
[security][3.3] bpo-30730: Prevent environment variables injection in subprocess on Windows. (GH-2325) (#2363)
1 parent 3625f7f commit e46f1c1

File tree

5 files changed

+70
-8
lines changed

5 files changed

+70
-8
lines changed

‎Lib/subprocess.py

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

13651365
if env is not None:
1366-
env_list = [os.fsencode(k) + b' 10000 ;=' + os.fsencode(v)
1367-
for k, v in env.items()]
1366+
env_list = []
1367+
for k, v in env.items():
1368+
k = os.fsencode(k)
1369+
if b'=' in k:
1370+
raise ValueError("illegal environment variable name")
1371+
env_list.append(k + b'=' + os.fsencode(v))
13681372
else:
13691373
env_list = None # Use execv instead of execve.
13701374
executable = os.fsencode(executable)

‎Lib/test/test_subprocess.py

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

591+
def test_invalid_cmd(self):
592+
# null character in the command name
593+
cmd = sys.executable + '\0'
594+
with self.assertRaises((ValueError, TypeError)):
595+
subprocess.Popen([cmd, "-c", "pass"])
596+
597+
# null character in the command argument
598+
with self.assertRaises((ValueError, TypeError)):
599+
subprocess.Popen([sys.executable, "-c", "pass#\0"])
600+
601+
def test_invalid_env(self):
602+
# null character in the enviroment variable name
603+
newenv = os.environ.copy()
604+
newenv["FRUIT\0VEGETABLE"] = "cabbage"
605+
with self.assertRaises((ValueError, TypeError)):
606+
subprocess.Popen([sys.executable, "-c", "pass"], env=newenv)
607+
608+
# null character in the enviroment variable value
609+
newenv = os.environ.copy()
610+
newenv["FRUIT"] = "orange\0VEGETABLE=cabbage"
611+
with self.assertRaises((ValueError, TypeError)):
612+
subprocess.Popen([sys.executable, "-c", "pass"], env=newenv)
613+
614+
# equal character in the enviroment variable name
615 10000 +
newenv = os.environ.copy()
616+
newenv["FRUIT=ORANGE"] = "lemon"
617+
with self.assertRaises(ValueError):
618+
subprocess.Popen([sys.executable, "-c", "pass"], env=newenv)
619+
620+
# equal character in the enviroment variable value
621+
newenv = os.environ.copy()
622+
newenv["FRUIT"] = "orange=lemon"
623+
with subprocess.Popen([sys.executable, "-c",
624+
'import sys, os;'
625+
'sys.stdout.write(os.getenv("FRUIT"))'],
626+
stdout=subprocess.PIPE,
627+
env=newenv) as p:
628+
stdout, stderr = p.communicate()
629+
self.assertEqual(stdout, b"orange=lemon")
630+
591631
def test_communicate_stdin(self):
592632
p = subprocess.Popen([sys.executable, "-c",
593633
'import sys;'

‎Misc/NEWS

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,9 @@ Core and Builtins
3535
Library
3636
-------
3737

38+
- [Security] bpo-30730: Prevent environment variables injection in subprocess on
39+
Windows. Prevent passing other invalid environment variables and command arguments.
40+
3841
- [Security] bpo-30585: Fix TLS stripping vulnerability in smptlib,
3942
CVE-2016-0772. Reported by Team Oststrom
4043

‎Modules/_winapi.c

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -513,6 +513,20 @@ getenvironment(PyObject* environment)
513513
"environment can only contain strings");
514514
goto error;
515515
}
516+
if (PyUnicode_FindChar(key, '\0', 0, PyUnicode_GET_LENGTH(key), 1) != -1 ||
517+
PyUnicode_FindChar(value, '\0', 0, PyUnicode_GET_LENGTH(value), 1) != -1)
518+
{
519+
PyErr_SetString(PyExc_ValueError, "embedded null character");
520+
goto error;
521+
}
522+
/* Search from index 1 because on Windows starting '=' is allowed for
523+
defining hidden environment variables. */
524+
if (PyUnicode_GET_LENGTH(key) == 0 ||
525+
PyUnicode_FindChar(key, '=', 1, PyUnicode_GET_LENGTH(key), 1) != -1)
526+
{
527+
PyErr_SetString(PyExc_ValueError, "illegal environment variable name");
528+
goto error;
529+
}
516530
if (totalsize > PY_SSIZE_T_MAX - PyUnicode_GET_LENGTH(key) - 1) {
517531
PyErr_SetString(PyExc_OverflowError, "environment too long");
518532
goto error;
@@ -621,12 +635,13 @@ winapi_CreateProcess(PyObject* self, PyObject* args)
621635

622636
if (env_mapping != Py_None) {
623637
environment = getenvironment(env_mapping);
624-
if (! environment)
638+
if (environment == NULL) {
625639
return NULL;
640+
}
641+
/* contains embedded null characters */
626642
wenvironment = PyUnicode_AsUnicode(environment);
627-
if (wenvironment == NULL)
628-
{
629-
Py_XDECREF(environment);
643+
if (wenvironment == NULL) {
644+
Py_DECREF(environment);
630645
return NULL;
631646
}
632647
}

‎Objects/abstract.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2682,8 +2682,8 @@ _PySequence_BytesToCharpArray(PyObject* self)
26822682
array[i] = NULL;
26832683
goto fail;
26842684
}
2685-
data = PyBytes_AsString(item);
2686-
if (data == NULL) {
2685+
/* check for embedded null bytes */
2686+
if (PyBytes_AsStringAndSize(item, &data, NULL) < 0) {
26872687
/* NULL ter 4317 minate before freeing. */
26882688
array[i] = NULL;
26892689
goto fail;

0 commit comments

Comments
 (0)
0