Embeddable zip allows Windows registry to override module location

[izbyshev]

BPO	28896
Nosy	@brettcannon, @pfmoore, @ncoghlan, @tjguk, @ned-deily, @ericsnowcurrently, @zware, @zooba, @izbyshev
PRs	[Do Not Merge] Convert Misc/NEWS so that it is managed by towncrier #552
Files	28896_doc.patch

^{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 2016-12-12.19:21:48.032>
created_at = <Date 2016-12-07.16:38:28.268>
labels = ['type-bug', '3.7', 'OS-windows']
title = 'Embeddable zip allows Windows registry to override module location'
updated_at = <Date 2017-03-31.16:36:30.579>
user = 'https://github.com/izbyshev'

bugs.python.org fields:

activity = <Date 2017-03-31.16:36:30.579>
actor = 'dstufft'
assignee = 'steve.dower'
closed = True
closed_date = <Date 2016-12-12.19:21:48.032>
closer = 'steve.dower'
components = ['Windows']
creation = <Date 2016-12-07.16:38:28.268>
creator = 'izbyshev'
dependencies = []
files = ['45794']
hgrepos = []
issue_num = 28896
keywords = ['patch']
message_count = 14.0
messages = ['282632', '282637', '282638', '282644', '282654', '282656', '282667', '282718', '282720', '282724', '283039', '283040', '283376', '283382']
nosy_count = 10.0
nosy_names = ['brett.cannon', 'paul.moore', 'ncoghlan', 'tim.golden', 'ned.deily', 'python-dev', 'eric.snow', 'zach.ware', 'steve.dower', 'izbyshev']
pr_nums = ['552']
priority = None
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue28896'
versions = ['Python 3.6', 'Python 3.7']

[izbyshev]

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.

[zooba]

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...)

[pfmoore]

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.

[brettcannon]

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).

[zooba]

+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.

[ned-deily]

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.

[zooba]

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
```==================

[python-dev]

New changeset 25df9671663b by Steve Dower in branch '3.6':
Issue bpo-28896: Deprecate WindowsRegistryFinder
https://hg.python.org/cpython/rev/25df9671663b

New changeset 5376b3a168c8 by Steve Dower in branch 'default':
Issue bpo-28896: Deprecate WindowsRegistryFinder
https://hg.python.org/cpython/rev/5376b3a168c8

[zooba]

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.

[izbyshev]

Thanks to Steve and everyone for quick and decisive action!

[python-dev]

New changeset 5bd248c2cc75 by Steve Dower in branch '3.6':
Issue bpo-28896: Disable WindowsRegistryFinder by default.
https://hg.python.org/cpython/rev/5bd248c2cc75

New changeset 4bd131b028ce by Steve Dower in branch 'default':
Issue bpo-28896: Disable WindowsRegistryFinder by default.
https://hg.python.org/cpython/rev/4bd131b028ce

[zooba]

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.

[python-dev]

New changeset 6249350e654a by Steve Dower in branch '3.6':
Issue bpo-28896: Deprecate WindowsRegistryFinder
https://hg.python.org/cpython/rev/6249350e654a

[ned-deily]

[cherrypicked for 3.6.0rc2]