New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CVE-2020-15523] _Py_CheckPython3 uses uninitialized dllpath when embedder sets module path with Py_SetPath #73964
Comments
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. |
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? |
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. |
I fixed this issue in Python 3.8 with this commit: commit 410759f
I modified Py_SetPath() like that:
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. |
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)"? |
You're right. Care to create a pull request to fix it? |
Thanks, Anthony! And congratulations on becoming a CPython contributor! |
Oops, I'm guilty of pushing this change! Sorry & thanks for the fix. if (_Py_dll_path == NULL) {
/* Already set: nothing to do */
return _PyStatus_OK();
} |
Thank you Steve! I'm still seeing python3.dll being loaded from \DLLs\python3.dll. _Py_CheckPython3() uses Py_GetPrefix() as a prefix for \DLLs\python3.dll. It looks like Py_SetPath() sets the _Py_path_config.prefix to "", but I'm not seeing anything else set it to a real value? Line 508 in 7b79dc9
|
In the master branch, _Py_CheckPython3() doesn't use _Py_path_config.prefix. _PyPathConfig_InitDLLPath() calls GetModuleFileNameW(PyWin_DLLhModule, dll_path, MAXPATHLEN) if PyWin_DLLhModule is initialized. For example, _PyPathConfig_InitDLLPath() is called by Py_Initialize() and Py_SetPath(). PyWin_DLLhModule is initialized by DllMain(). The code in the 3.8 branch looks very similar (I backported my "Remove _PyPathConfig.dll_path" change to 3.8: commit 9f3dcf8). |
Hm, I'm seeing _Py_CheckPython3() use Py_GetPrefix(), which uses _Py_path_config.prefix? Line 1185 in c02b41b
|
Oh right, that's the initial issue:
I reopen the issue. |
_Py_CheckPython3() tries to load "python3.dll" from two directories:
I understand that LoadLibraryExW() must not be attempted if _Py_dll_path is empty, or if Py_GetPrefix() is empty. Am I right? |
I understand that Python 3.5, 3.6 and 3.7 are also affected. It's not a regression. On Python 3.5, 3.6 and 3.7, when Py_SetPath(path) is called, Py_GetPrefix() also returns an empty path. So at least the directory based on Py_GetPrefix() should also be skipped on Python 3.5-3.7 if Py_GetPrefix() is empty, right? |
More likely those should never be empty. Perhaps sys.prefix is optional, but the DLL path is the current executing module, and should always be set. I suspect you're right, that 3.7 is also affected. But earlier versions would only _not_ fill the DLL path for static (non-shared) builds. It looks like Py_SetPath in 3.7 started clearing it unnecessarily, so that may be the cause. |
Sorry, I take that back. Earlier versions would indeed skip initialization in some cases. I propose we deprecate the dll_path field in PathConfig and just get the path directly in the three places it's necessary. The path calculations have security exposure, so let's just avoid trying to manage additional state around it unnecessarily. I'll work on a patch this week unless someone else gets to it first. |
Bumping to release blocker and adding RMs. Should definitely get this fix merged within the next week, and I don't want the next round of releases to go out without it. |
Fixes are in. Also adding the CVE number to the bug title. |
Announcement post: https://mail.python.org/archives/list/security-announce@python.org/thread/C5RIXC2ZIML3NOEIOGFPA6ISGU5L2QXL/ CVE-2020-15523 is an invalid search path in Python 3.6 and later on This issue is not triggered when running python.exe. It only applies Issue: https://bugs.python.org/issue29778 The next patched releases will be: 3.9.0b5, 3.8.4, 3.7.9 (source only), Other than applying the patch, applications may mitigate the Thanks to Eric Gantumur for detecting and reporting the issue to the |
FYI this vulnerability is now tracked by: |
Steve: Python 3.5 is also vulnerable, no? This branch still gets security fixes, do you plan to backport the fix? I can do it if you are not available. |
You're right. I thought because the backport tag was gone on GitHub that it was EOL already. I can do the backport. |
Correction: the original discovery credit goes to Eran Shimony <Eran.Shimony@cyberark.com> and Ido Hoorvitch <Ido.Hoorvitch@cyberark.com> from CyberArk. |
FYI, bpo-41304 fixed a regression in this patch in 3.7 and later. The regression shipped in 3.8.4 and 3.9.0b4, but will be fixed in the subsequent releases. |
I must have taken my stupid pills today. Why is this considered a "security" "release blocker"? If you can put files in the root of the hard drive where Windows was installed, surely you have other, easier attack vectors. |
A rooted path is resolved relative to the process working directory, and Python can be started with any current working directory. The default access control set on the root directory of a filesystem allows any authenticated user to create files or directories, such as "D:\python3.dll". That's if a filesystem even supports security. Removable drives are often formatted as FAT32 or exFAT, and FAT filesystems have no security. The system drive (almost always "C:") has to be an NTFS filesystem, and its root directory is locked down a bit more. It's at high integrity level with a no-write-up rule for files, but not for directories. Only a logon at elevated integrity level (high or system level) can create "C:\python3.dll". OTOH, any authenticated user is still allowed to create a directory, such as "C:\DLLs", and is granted the right to create files in it such as "C:\DLLs\python3.dll". |
I still don't understand why this is considered a Python security problem. If the user can put a malicious "python3.dll" at some arbitrary spot in the filesystem (e.g. a USB flash drive), and fool Python.exe into loading it, then surely they could put an arbitrary executable at that same spot and launch it directly. And that seems way more straightforward. Why would anyone bother with this? |
What would be the point of adding an arbitrary executable in "C:\spam" or "D:\"? It's not in the system PATH, "App Paths", or any file-association template command. But if you can inject code into vulnerable processes that embed Python by simply creating "C:\DLLs\python3.dll", that seems like low-hanging fruit to me. Just wait for it to be run with administrator access, and then you can own the entire system. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: