Skip to content
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

Embeddable zip allows Windows registry to override module location #73082

Closed
izbyshev mannequin opened this issue Dec 7, 2016 · 14 comments
Closed

Embeddable zip allows Windows registry to override module location #73082

izbyshev mannequin opened this issue Dec 7, 2016 · 14 comments
Assignees
Labels
3.7 (EOL) end of life OS-windows type-bug An unexpected behavior, bug, or error

Comments

@izbyshev
Copy link
Mannequin

izbyshev mannequin commented Dec 7, 2016

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
    Copy link
    Mannequin Author

    izbyshev mannequin commented Dec 7, 2016

    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.

    @izbyshev izbyshev mannequin added OS-windows type-bug An unexpected behavior, bug, or error labels Dec 7, 2016
    @zooba
    Copy link
    Member

    zooba commented Dec 7, 2016

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

    @zooba zooba added the 3.7 (EOL) end of life label Dec 7, 2016
    @pfmoore
    Copy link
    Member

    pfmoore commented Dec 7, 2016

    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
    Copy link
    Member

    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
    Copy link
    Member

    zooba commented Dec 7, 2016

    +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
    Copy link
    Member

    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
    Copy link
    Member

    zooba commented Dec 7, 2016

    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
    Copy link
    Mannequin

    python-dev mannequin commented Dec 8, 2016

    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
    Copy link
    Member

    zooba commented Dec 8, 2016

    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.

    @zooba zooba self-assigned this Dec 8, 2016
    @izbyshev
    Copy link
    Mannequin Author

    izbyshev mannequin commented Dec 8, 2016

    Thanks to Steve and everyone for quick and decisive action!

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 12, 2016

    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
    Copy link
    Member

    zooba commented Dec 12, 2016

    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.

    @zooba zooba closed this as completed Dec 12, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 16, 2016

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

    @ned-deily
    Copy link
    Member

    [cherrypicked for 3.6.0rc2]

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life OS-windows type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants