[3.9] gh-101283: Improved fallback logic for subprocess with shell=True on Windows (GH-101286) (#101709)

Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>
Co-authored-by: Steve Dower <steve.dower@microsoft.com>

3 files changed, 60 insertions, 1 deletions

diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst
index 10e40635..370ea58 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.9.17
+
+      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.
@@ -468,6 +476,14 @@ functions.
       *executable* parameter accepts a bytes and :term:`path-like object`
       on Windows.
 
+   .. versionchanged:: 3.9.17
+
+      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
@@ -1126,6 +1142,14 @@ calls these functions.
    .. versionchanged:: 3.3
       *timeout* was added.
 
+   .. versionchanged:: 3.9.17
+
+      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)
@@ -1158,6 +1182,14 @@ calls these functions.
    .. versionchanged:: 3.3
       *timeout* was added.
 
+   .. versionchanged:: 3.9.17
+
+      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, \
@@ -1213,6 +1245,14 @@ calls these functions.
    .. versionadded:: 3.7
       *text* was added as a more readable alias for *universal_newlines*.
 
+   .. versionchanged:: 3.9.17
+
+      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 4effc1d..8579d3f 100644
--- a/Lib/subprocess.py
+++ b/Lib/subprocess.py
@@ -1407,7 +1407,23 @@ class Popen(object):
             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)
 
             if cwd is not None:
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 0000000..0efdfa1
--- /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.