[CVE-2020-15523] _Py_CheckPython3 uses uninitialized dllpath when embedder sets module path with Py_SetPath

[TiborCsonka]

BPO	29778
Nosy	@pfmoore, @vstinner, @larryhastings, @tjguk, @ned-deily, @ambv, @zware, @eryksun, @zooba, @miss-islington, @anthonywee
PRs	bpo-29778: Fix incorrect NULL check #17818 [3.8] bpo-29778: Fix incorrect NULL check in _PyPathConfig_InitDLLPath() (GH-17818) #17871 bpo-29778: Ensure python3.dll is loaded from correct locations when Python is embedded #21297 [3.7] bpo-29778: Ensure python3.dll is loaded from correct locations when Python is embedded (GH-21297) #21298 bpo-29778: test_embed tests the path configuration #21306 [3.9] bpo-29778: Ensure python3.dll is loaded from correct locations when Python is embedded (GH-21297) #21351 [3.8] bpo-29778: Ensure python3.dll is loaded from correct locations when Python is embedded (GH-21297) #21352 [3.6] bpo-29778: Ensure python3.dll is loaded from correct locations when Python is embedded (GH-21298) #21354 [3.5] bpo-29778: Ensure python3.dll is loaded from correct locations when Python is embedded (GH-21297) #21377

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/zooba'
closed_at = <Date 2020-07-06.18:57:21.099>
created_at = <Date 2017-03-10.04:58:18.536>
labels = ['type-security', '3.8', '3.9', '3.10', 'release-blocker', '3.7', 'OS-windows']
title = '[CVE-2020-15523] _Py_CheckPython3 uses uninitialized dllpath when embedder sets module path with Py_SetPath'
updated_at = <Date 2020-08-04.02:16:28.217>
user = 'https://bugs.python.org/TiborCsonka'

bugs.python.org fields:

activity = <Date 2020-08-04.02:16:28.217>
actor = 'larry'
assignee = 'steve.dower'
closed = True
closed_date = <Date 2020-07-06.18:57:21.099>
closer = 'steve.dower'
components = ['Windows']
creation = <Date 2017-03-10.04:58:18.536>
creator = 'Tibor Csonka'
dependencies = []
files = []
hgrepos = []
issue_num = 29778
keywords = ['patch']
message_count = 37.0
messages = ['289334', '289364', '289412', '343638', '359136', '359245', '359438', '359439', '359440', '359441', '359498', '359544', '359549', '359550', '372555', '372556', '372595', '372596', '372961', '373138', '373142', '373147', '373150', '373155', '373156', '373164', '373213', '373214', '373228', '373257', '373618', '373757', '373789', '373919', '374024', '374029', '374785']
nosy_count = 12.0
nosy_names = ['paul.moore', 'vstinner', 'larry', 'tim.golden', 'ned.deily', 'lukasz.langa', 'zach.ware', 'eryksun', 'steve.dower', 'Tibor Csonka', 'miss-islington', 'anthonywee']
pr_nums = ['17818', '17871', '21297', '21298', '21306', '21351', '21352', '21354', '21377']
priority = 'release blocker'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue29778'
versions = ['Python 3.5', 'Python 3.6', 'Python 3.7', 'Python 3.8', 'Python 3.9', 'Python 3.10']

[TiborCsonka]

When Py_SetPath is used to set up module path at initialization, the Py_SetPath causes getpathp.c::calculate_path not to be called. However, calculate path is the only function calling getpathp.c::get_progpath which initializes the local dllpath static variable.

Later the interpreter tries to load python3.dll and uses dllpath which is empty by default. This empty path gets joined with \python3.dll and \DLLs\python3.dll which is used in the LoadLibraryExW resulting in loading python3.dll from the root location of the windows drive the application is running from.

The behavior was reproduced using PyInstaller but it is present in any embedding application which uses Py_SetPath.

[zooba]

I thought we'd documented that if you set the path when embedding you should also set the program name, but perhaps not (didn't check just now). If not, we should do that.

We shouldn't be loading python3.dll anywhere. Are you sure that's in CPython? Do you have a reference to the source file?

[zooba]

Ah, I see. We force load it in PC/getpathp.c to ensure that it's ours and not another version's python3.dll.

We should probably refactor the GetModuleFileNameW call into its own function so we can call it from anywhere we need.

[vstinner]

When Py_SetPath is used to set up module path at initialization, the Py_SetPath causes getpathp.c::calculate_path not to be called. However, calculate path is the only function calling getpathp.c::get_progpath which initializes the local dllpath static variable.

I fixed this issue in Python 3.8 with this commit:

commit 410759f
Author: Victor Stinner <vstinner@redhat.com>
Date: Sat May 18 04:17:01 2019 +0200

bpo-36763: Remove _PyCoreConfig.dll_path (GH-13402)

I modified Py_SetPath() like that:

• new_config.dll_path = _PyMem_RawWcsdup(L"");
  + new_config.dll_path = _Py_GetDLLPath();

Py_SetPath() no longer sets dll_path to an empty string.

Since we only got one bug report and I believe that Tibor Csonka found a way to workaround the issue since he reported it, I close the issue.

Please reopen/comment the issue if you would like to get this issue fixed in Python 3.7 as well.

--

Moreover, the PEP-587 now has a better API to configure embedded Python. I just implemented this PEP in bpo-36763.

[anthonywee]

It looks like there has been a regression in the fix for this issue.

The commit below introduced a NULL check which causes a call to _PyPathConfig_Init() to be skipped if _Py_dll_path == NULL. It seems like the check should be "if (_Py_dll_path != NULL)"?

c422167#diff-87aed37b4704d4e1513be6378c9c7fe6R169

[zooba]

It looks like there has been a regression in the fix for this issue.

You're right. Care to create a pull request to fix it?

[zooba]

New changeset 7b79dc9 by Steve Dower (Anthony Wee) in branch 'master':
bpo-29778: Fix incorrect NULL check in _PyPathConfig_InitDLLPath() (GH-17818)
7b79dc9

[zooba]

Thanks, Anthony! And congratulations on becoming a CPython contributor!