From e06a55e79633113341a78bc0f65d7c4b30780cb9 Mon Sep 17 00:00:00 2001 From: Oleg Iarygin Date: Thu, 9 Feb 2023 02:12:19 +0400 Subject: [PATCH 1/2] gh-101283: Improved fallback logic for subprocess with shell=True on Windows (GH-101286) --- Doc/library/subprocess.rst | 43 +++++++++++++++++++ Lib/subprocess.py | 18 +++++++- ...-01-24-16-12-00.gh-issue-101283.9tqu39.rst | 3 ++ 3 files changed, 63 insertions(+), 1 deletion(-) create mode 100644 Misc/NEWS.d/next/Security/2023-01-24-16-12-00.gh-issue-101283.9tqu39.rst diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index 9f2a05662860ee..9f12ba421d5163 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -111,6 +111,14 @@ compatibility with older versions, see the :ref:`call-function-trio` section. Added the *text* parameter, as a more understandable alias of *universal_newlines*. Added the *capture_output* parameter. + .. versionchanged:: 3.11.2 + + Changed Windows shell search order for ``shell=True``. The current + directory and ``%PATH%`` are replaced with ``%COMSPEC%`` and + ``%SystemRoot%\System32\cmd.exe``. As a result, dropping a + malicious program named ``cmd.exe`` into a current directory no + longer works. + .. class:: CompletedProcess The return value from :func:`run`, representing a process that has finished. @@ -442,6 +450,17 @@ functions. :program:`ps`. If ``shell=True``, on POSIX the *executable* argument specifies a replacement shell for the default :file:`/bin/sh`. + .. versionchanged:: 3.6 + *executable* parameter accepts a :term:`path-like object` on POSIX. + + .. versionchanged:: 3.11.2 + + Changed Windows shell search order for ``shell=True``. The current + directory and ``%PATH%`` are replaced with ``%COMSPEC%`` and + ``%SystemRoot%\System32\cmd.exe``. As a result, dropping a + malicious program named ``cmd.exe`` into a current directory no + longer works. + *stdin*, *stdout* and *stderr* specify the executed program's standard input, standard output and standard error file handles, respectively. Valid values are :data:`PIPE`, :data:`DEVNULL`, an existing file descriptor (a positive @@ -1032,6 +1051,14 @@ calls these functions. .. versionchanged:: 3.3 *timeout* was added. + .. versionchanged:: 3.11.2 + + Changed Windows shell search order for ``shell=True``. The current + directory and ``%PATH%`` are replaced with ``%COMSPEC%`` and + ``%SystemRoot%\System32\cmd.exe``. As a result, dropping a + malicious program named ``cmd.exe`` into a current directory no + longer works. + .. function:: check_call(args, *, stdin=None, stdout=None, stderr=None, \ shell=False, cwd=None, timeout=None, \ **other_popen_kwargs) @@ -1062,6 +1089,14 @@ calls these functions. .. versionchanged:: 3.3 *timeout* was added. + .. versionchanged:: 3.11.2 + + Changed Windows shell search order for ``shell=True``. The current + directory and ``%PATH%`` are replaced with ``%COMSPEC%`` and + ``%SystemRoot%\System32\cmd.exe``. As a result, dropping a + malicious program named ``cmd.exe`` into a current directory no + longer works. + .. function:: check_output(args, *, stdin=None, stderr=None, shell=False, \ cwd=None, encoding=None, errors=None, \ @@ -1116,6 +1151,14 @@ calls these functions. .. versionadded:: 3.7 *text* was added as a more readable alias for *universal_newlines*. + .. versionchanged:: 3.11.2 + + Changed Windows shell search order for ``shell=True``. The current + directory and ``%PATH%`` are replaced with ``%COMSPEC%`` and + ``%SystemRoot%\System32\cmd.exe``. As a result, dropping a + malicious program named ``cmd.exe`` into a current directory no + longer works. + .. _subprocess-replacements: diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 3f99be551c5152..fd67945b7e4b33 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -1192,7 +1192,23 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, if shell: startupinfo.dwFlags |= _winapi.STARTF_USESHOWWINDOW startupinfo.wShowWindow = _winapi.SW_HIDE - comspec = os.environ.get("COMSPEC", "cmd.exe") + if not executable: + # gh-101283: without a fully-qualified path, before Windows + # checks the system directories, it first looks in the + # application directory, and also the current directory if + # NeedCurrentDirectoryForExePathW(ExeName) is true, so try + # to avoid executing unqualified "cmd.exe". + comspec = os.environ.get('ComSpec') + if not comspec: + system_root = os.environ.get('SystemRoot', '') + comspec = os.path.join(system_root, 'System32', 'cmd.exe') + if not os.path.isabs(comspec): + raise FileNotFoundError('shell not found: neither %ComSpec% nor %SystemRoot% is set') + if os.path.isabs(comspec): + executable = comspec + else: + comspec = executable + args = '{} /c "{}"'.format (comspec, args) # Start the process diff --git a/Misc/NEWS.d/next/Security/2023-01-24-16-12-00.gh-issue-101283.9tqu39.rst b/Misc/NEWS.d/next/Security/2023-01-24-16-12-00.gh-issue-101283.9tqu39.rst new file mode 100644 index 00000000000000..0efdfa10234185 --- /dev/null +++ b/Misc/NEWS.d/next/Security/2023-01-24-16-12-00.gh-issue-101283.9tqu39.rst @@ -0,0 +1,3 @@ +:class:`subprocess.Popen` now uses a safer approach to find +``cmd.exe`` when launching with ``shell=True``. Patch by Eryk Sun, +based on a patch by Oleg Iarygin. From 31966323c39a9cd168e2301f963e0c59110f38d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Langa?= Date: Thu, 9 Feb 2023 11:01:58 +0100 Subject: [PATCH 2/2] Apply suggestions from code review Co-authored-by: Oleg Iarygin --- Doc/library/subprocess.rst | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index 9f12ba421d5163..4c51f4f1f03c21 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -111,7 +111,7 @@ compatibility with older versions, see the :ref:`call-function-trio` section. Added the *text* parameter, as a more understandable alias of *universal_newlines*. Added the *capture_output* parameter. - .. versionchanged:: 3.11.2 + .. versionchanged:: 3.7.17 Changed Windows shell search order for ``shell=True``. The current directory and ``%PATH%`` are replaced with ``%COMSPEC%`` and @@ -453,7 +453,7 @@ functions. .. versionchanged:: 3.6 *executable* parameter accepts a :term:`path-like object` on POSIX. - .. versionchanged:: 3.11.2 + .. versionchanged:: 3.7.17 Changed Windows shell search order for ``shell=True``. The current directory and ``%PATH%`` are replaced with ``%COMSPEC%`` and @@ -1051,7 +1051,7 @@ calls these functions. .. versionchanged:: 3.3 *timeout* was added. - .. versionchanged:: 3.11.2 + .. versionchanged:: 3.7.17 Changed Windows shell search order for ``shell=True``. The current directory and ``%PATH%`` are replaced with ``%COMSPEC%`` and @@ -1089,7 +1089,7 @@ calls these functions. .. versionchanged:: 3.3 *timeout* was added. - .. versionchanged:: 3.11.2 + .. versionchanged:: 3.7.17 Changed Windows shell search order for ``shell=True``. The current directory and ``%PATH%`` are replaced with ``%COMSPEC%`` and @@ -1151,7 +1151,7 @@ calls these functions. .. versionadded:: 3.7 *text* was added as a more readable alias for *universal_newlines*. - .. versionchanged:: 3.11.2 + .. versionchanged:: 3.7.17 Changed Windows shell search order for ``shell=True``. The current directory and ``%PATH%`` are replaced with ``%COMSPEC%`` and