classification
Title: Embeddable zip allows Windows registry to override module location
Type: behavior Stage: resolved
Components: Windows Versions: Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: steve.dower Nosy List: brett.cannon, eric.snow, izbyshev, ncoghlan, ned.deily, paul.moore, python-dev, steve.dower, tim.golden, zach.ware
Priority: Keywords: patch

Created on 2016-12-07 16:38 by izbyshev, last changed 2017-03-31 16:36 by dstufft. This issue is now closed.

Files
File name Uploaded Description Edit
28896_doc.patch steve.dower, 2016-12-07 21:02 review
Pull Requests
URL Status Linked Edit
PR 552 closed dstufft, 2017-03-31 16:36
Messages (14)
msg282632 - (view) Author: Alexey Izbyshev (izbyshev) * (Python triager) Date: 2016-12-07 16:38
The docs claim: "... the embedded distribution is (almost) fully isolated from the user’s system, including environment variables, system registry settings, and installed packages."

Via ProcessMonitor tool I've discovered that python.exe still accesses keys like "HKLM\Software\Python\PythonCore\3.5\Modules\collections" on every module import, allowing registry settings to override the location of any non-builtin module.

Digging into the 3.5.2 code revealed that WindowsRegistryFinder is unconditionally added to sys.meta_path (Lib/importlib/_bootstrap_external.py, line 1422):

    if _os.__name__ == 'nt':
        sys.meta_path.append(WindowsRegistryFinder)

It can also be confirmed in runtime:

Python 3.5.2 (v3.5.2:4def2a2901a5, Jun 25 2016, 22:18:55) [MSC v.1900 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> print(sys.meta_path)
[<class '_frozen_importlib.BuiltinImporter'>, <class '_frozen_importlib.FrozenImporter'>, <class '_frozen_importlib_external.WindowsRegistryFinder'>, <class '_frozen_importlib_external.PathFinder'>]

Is this behavior intended? It seems to be against doc claims and the goal of embeddability.
msg282637 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-12-07 17:25
It's not intentional, but we clearly haven't done anything to prevent it.

Arguably this finder should be omitted when you run in isolated mode, and I'm on the fence about deprecating it entirely. Adding the importlib experts in case they have opinions (relevant ones, ideally).

A one line workaround that can be added to any code base is:

>>> sys.meta_path[:] = [m for m in sys.meta_path if m.__name__ != 'WindowsRegistryFinder']

But it would also be good to close off this hole. Thoughts on the best option? (-I, move to site.py and -S, something new...)
msg282638 - (view) Author: Paul Moore (paul.moore) * (Python committer) Date: 2016-12-07 17:31
I thought that most of the users of this functionality had stopped doing so (the only one I recall for certain was pywin32, and last time this came up, I think someone said they had stopped).

If the functionality isn't used in any of the well-known modules, I'm +1 on deprecating it (if it is, I'm all for encouraging them to stop and then *still* deprecating it :-))

Use of the registry keys would prevent virtual environments from managing proper isolation, as well, so it's not just an issue for the embedded distribution.
msg282644 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-12-07 17:53
Deprecate the importer. If I remember correctly it took us a while to even notice it was missing due to missing tests prior to importlib coming into existence (and getting anyone to care enough to help write those tests also took a lot of effort).
msg282654 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-12-07 19:16
+Ned

Could we get a doc patch into 3.6 marking this class as deprecated? It appears like the importlib docs are the only ones that refer to the class, and none of the docs describe the functionality or indicate that it is enabled by default.

I could also pitch this as a security vulnerability and push for removing the default .append() right now? Since we wouldn't remove the class itself, restoring the previous behavior just requires inserting it into meta_path again. And Alexey is right that it actually allows a non-admin user to shadow any non-builtin module.

Looking at the latest pywin32 installer, they actually *remove* the keys they used to add here because they cause problems. So I think we're fairly safe to disable the finder by default and deprecate it into the future.
msg282656 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2016-12-07 19:50
I'm OK with adding a doc change before 3.6.0 final.  But since this behavior is not new with 3.6, I would rather save any code changes for 3.6.1 unless there is a consensus that this is an urgent security issue.
msg282667 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-12-07 21:01
Here's my proposed doc change for 3.6.0. Any concerns about wording? (The change to remove the line from _bootstrap_external.py will be separate, for ease of cherry-picking.)


diff --git a/Doc/library/importlib.rst b/Doc/library/importlib.rst
--- a/Doc/library/importlib.rst
+++ b/Doc/library/importlib.rst
@@ -806,6 +806,10 @@
 
    .. versionadded:: 3.3
 
+   .. deprecated:: 3.6
+      Use :mod:`site` configuration instead. Future versions of Python may
+      not enable this finder by default.
+
 
 .. class:: PathFinder
 
diff --git a/Doc/using/windows.rst b/Doc/using/windows.rst
--- a/Doc/using/windows.rst
+++ b/Doc/using/windows.rst
@@ -823,6 +823,14 @@
       * Adds ``pythonXX.zip`` as a potential landmark when directly adjacent
         to the executable.
 
+.. deprecated::
+   3.6
+
+      Modules specified in the registry under ``Modules`` (not ``PythonPath``)
+      may be imported by :class:`importlib.machinery.WindowsRegistryFinder`.
+      This finder is enabled on Windows in 3.6.0 and earlier, but may need to
+      be explicitly added to :attr:`sys.meta_path` in the future.
+
 Additional modules
 ==================
msg282718 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-12-08 17:02
New changeset 25df9671663b by Steve Dower in branch '3.6':
Issue #28896: Deprecate WindowsRegistryFinder
https://hg.python.org/cpython/rev/25df9671663b

New changeset 5376b3a168c8 by Steve Dower in branch 'default':
Issue #28896: Deprecate WindowsRegistryFinder
https://hg.python.org/cpython/rev/5376b3a168c8
msg282720 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-12-08 17:04
I assumed silence meant everyone was happy with the wording, so I extended it to whatsnew and NEWS and pushed.

Ned - the changeset above should be good for you to cherrypick.

I'll leave this issue open to cover actually removing the finder from sys.meta_path in 3.7 and potentially 3.6.1.
msg282724 - (view) Author: Alexey Izbyshev (izbyshev) * (Python triager) Date: 2016-12-08 18:07
Thanks to Steve and everyone for quick and decisive action!
msg283039 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-12-12 19:20
New changeset 5bd248c2cc75 by Steve Dower in branch '3.6':
Issue #28896: Disable WindowsRegistryFinder by default.
https://hg.python.org/cpython/rev/5bd248c2cc75

New changeset 4bd131b028ce by Steve Dower in branch 'default':
Issue #28896: Disable WindowsRegistryFinder by default.
https://hg.python.org/cpython/rev/4bd131b028ce
msg283040 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-12-12 19:21
And that commit removes WindowsRegistryFinder from sys.meta_path on startup (as well as fixing regeneration of importlib when building on Windows).

It should *not* be cherry picked for 3.6.0.
msg283376 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-12-16 07:44
New changeset 6249350e654a by Steve Dower in branch '3.6':
Issue #28896: Deprecate WindowsRegistryFinder
https://hg.python.org/cpython/rev/6249350e654a
msg283382 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2016-12-16 07:52
[cherrypicked for 3.6.0rc2]
History
Date User Action Args
2017-03-31 16:36:30dstufftsetpull_requests: + pull_request1033
2016-12-16 07:52:09ned.deilysetpriority: release blocker ->

messages: + msg283382
2016-12-16 07:44:45python-devsetmessages: + msg283376
2016-12-12 19:21:48steve.dowersetstatus: open -> closed
resolution: fixed
messages: + msg283040

stage: commit review -> resolved
2016-12-12 19:20:38python-devsetmessages: + msg283039
2016-12-08 18:07:47izbyshevsetmessages: + msg282724
2016-12-08 17:04:18steve.dowersetpriority: normal -> release blocker
versions: - Python 3.5
messages: + msg282720

assignee: steve.dower
stage: commit review
2016-12-08 17:02:13python-devsetnosy: + python-dev
messages: + msg282718
2016-12-07 21:02:47steve.dowersetfiles: + 28896_doc.patch
keywords: + patch
2016-12-07 21:01:28steve.dowersetmessages: + msg282667
2016-12-07 19:50:36ned.deilysetmessages: + msg282656
2016-12-07 19:16:38steve.dowersetnosy: + ned.deily
messages: + msg282654
2016-12-07 17:53:21brett.cannonsetmessages: + msg282644
2016-12-07 17:31:41paul.mooresetmessages: + msg282638
2016-12-07 17:25:02steve.dowersetnosy: + brett.cannon, ncoghlan, eric.snow

messages: + msg282637
versions: + Python 3.6, Python 3.7
2016-12-07 16:38:28izbyshevcreate