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

_ssl.enum_certificates() fails with ERROR_ACCESS_DENIED if python.exe run with low integrity level #70127

Closed
yan12125 mannequin opened this issue Dec 24, 2015 · 16 comments
Labels
extension-modules C modules in the Modules dir OS-windows type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@yan12125
Copy link
Mannequin

yan12125 mannequin commented Dec 24, 2015

BPO 25939
Nosy @pfmoore, @tiran, @tjguk, @benjaminp, @zware, @eryksun, @zooba, @yan12125
Files
  • open-system-store-readonly.patch
  • lower_integrity_level.c
  • test-ssl-with-lower-integrity-level.patch
  • integrity_level.py
  • 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 = None
    closed_at = <Date 2016-09-08.17:34:35.721>
    created_at = <Date 2015-12-24.20:40:28.225>
    labels = ['extension-modules', 'OS-windows', 'type-crash']
    title = '_ssl.enum_certificates() fails with ERROR_ACCESS_DENIED if python.exe run with low integrity level'
    updated_at = <Date 2016-09-08.17:34:35.720>
    user = 'https://github.com/yan12125'

    bugs.python.org fields:

    activity = <Date 2016-09-08.17:34:35.720>
    actor = 'steve.dower'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-09-08.17:34:35.721>
    closer = 'steve.dower'
    components = ['Extension Modules', 'Windows']
    creation = <Date 2015-12-24.20:40:28.225>
    creator = 'yan12125'
    dependencies = []
    files = ['41405', '41411', '41432', '41439']
    hgrepos = []
    issue_num = 25939
    keywords = ['patch']
    message_count = 16.0
    messages = ['256968', '256971', '256972', '256978', '256979', '257078', '257098', '257102', '257116', '260429', '260430', '260891', '260901', '260922', '275052', '275074']
    nosy_count = 9.0
    nosy_names = ['paul.moore', 'christian.heimes', 'tim.golden', 'benjamin.peterson', 'python-dev', 'zach.ware', 'eryksun', 'steve.dower', 'yan12125']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'crash'
    url = 'https://bugs.python.org/issue25939'
    versions = ['Python 2.7', 'Python 3.4', 'Python 3.5', 'Python 3.6']

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Dec 24, 2015

    Originally reported at ytdl-org/youtube-dl#7951

    Steps to reproduce:

    1. Build 99665:dcf9e9ae5393 with Visual Studio 2015
    2. Download and extract PsTools [1]
    3. PsExec.exe -l python.exe
    4. In Python, run:
        import _ssl
        _ssl.enum_certificates("CA")
        _ssl.enum_crls("CA")
    Results:
    Python 3.6.0a0 (default, Dec 25 2015, 02:42:42) [MSC v.1900 32 bit (Intel)] on win32
    Type "help", "copyright", "credits" or "license" for more information.
    >>> import _ssl
    >>> _ssl.enum_certificates("CA")
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    PermissionError: [WinError 5] Access is denied
    >>> _ssl.enum_crls("CA")
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    PermissionError: [WinError 5] Access is denied
    >>>

    Windows Vista and above have a security mechanism called "Low Integrity Level". [2] With that, only some specific registry keys are writable. In the original _ssl.c, both enum_certificates() and enum_crls() calls CertOpenSystemStore(). At least on my system CertOpenSystemStore() tries to open HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\SystemCertificates\CA with read/write permissions. (Observed with Process Monitor [3]) The request fails in Low Integrity Level processes as it's not in the range of writable registry keys.

    Here I propose a fix: open certificate stores with the read-only flag. There are some points I'm not sure in this patch:

    1. CERT_STORE_PROV_SYSTEM_A: I guess strings are bytestrings in C level?
    2. CERT_SYSTEM_STORE_LOCAL_MACHINE: In accounts of Administrators, CertOpenSystemStore() tries to open keys under HKLM only, while in restricted (standard) accounts, this function tries to open keys under HKCU with R/W permission and keys under HKLM read-only. I think open system global stores is OK here.
      A different perspective: Wine developers always open keys under HKCU in CertOpenSystemStore()

    Environment: Windows 7 SP1 (6.1.7601) x86, an account in Administrators group. Tested with python.exe Lib\test\test_ssl.py both in a normal shell and within PsExec -l

    Ref: bpo-17134, where these codes appear the first time

    [1] https://technet.microsoft.com/en-us/sysinternals/pstools.aspx
    [2] https://msdn.microsoft.com/en-us/library/bb625960.aspx
    [3] https://technet.microsoft.com/en-us/sysinternals/processmonitor.aspx
    [4] https://github.com/wine-mirror/wine/blob/master/dlls/crypt32/store.c

    @yan12125 yan12125 mannequin added extension-modules C modules in the Modules dir OS-windows type-crash A hard crash of the interpreter, possibly with a core dump labels Dec 24, 2015
    @zooba
    Copy link
    Member

    zooba commented Dec 24, 2015

    Looks good to me.

    Is it worth dropping psexec.exe into the test suite so we can add a test for this (or maybe into tools so we can run it from a build without redistributing the exe)? It'll probably be helpful elsewhere too (symlink tests, for example).

    @eryksun
    Copy link
    Contributor

    eryksun commented Dec 24, 2015

    psexec.exe can be run from the the live server.

    >>> subprocess.call(r'\\live.sysinternals.com\tools\psexec.exe -s whoami')
    
    PsExec v2.11 - Execute processes remotely
    Copyright (C) 2001-2014 Mark Russinovich
    Sysinternals - www.sysinternals.com
    
    
    nt authority\system
    whoami exited on THISPC with error code 0.
    0
    

    But the executable could also be cached on the test system.

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Dec 25, 2015

    PsExec.exe seems not redistributable. PAExec is an alternative but I've not tried it. [1] Another option is re-implementing a tiny program for lowering the integrity level based on example codes provided in [2], which I've not tried yet, either. The latter option seems better to me as I didn't find codes for lowering the integrity level in PAExec's source code. [3]

    [1] https://www.poweradmin.com/paexec/
    [2] https://msdn.microsoft.com/en-us/library/bb625960.aspx
    [3] https://github.com/poweradminllc/PAExec

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Dec 25, 2015

    OK I've just succeeded in creating a low integrity level process with my own codes. Now the problem is: how can I integrate this tool into the test system? Seems the integrity level is per-process, while all tests are run in the same process.

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Dec 27, 2015

    Added tests for ssl.enum_certificates() and ssl.enum_crls() within low integrity processes.

    @zooba
    Copy link
    Member

    zooba commented Dec 27, 2015

    Can we use ctypes in the test suite? That would probably be simpler here, as the C code is straightforward. (Question is for the other core devs, not Chi, as I know we're not supposed to use ctypes in the stdlib.)

    @zware
    Copy link
    Member

    zware commented Dec 27, 2015

    ctypes in the test suite is fine, just be sure it's guarded properly (with either ctypes = test.support.import_module("ctypes") or if sys.platform == 'win32'): ....

    @eryksun
    Copy link
    Contributor

    eryksun commented Dec 28, 2015

    Testing based on integrity level doesn't require creating a child process. I'm attaching a ctypes-based example that defines a context manager that temporarily sets the integrity level of the current thread's impersonation token.

    To get the impersonation token, I initially tried using ImpersonateSelf / RevertToSelf, but I was unhappy with how it fails for nested contexts since RevertToSelf always switches back to the process token. I opted to instead call OpenThreadToken / OpenProcessToken, DuplicateTokenEx, and SetThreadToken.

    I chose to use the WELL_KNOWN_SID_TYPE enum values to get the label SIDs via CreateWellKnownSid. Note that I omitted the GetLengthSid call when passing the size of the TOKEN_MANDATORY_LABEL to SetTokenInformation. It only needs the size of the primary buffer. The SID it points at is a sized structure (i.e. SubAuthorityCount).

    Example:

        import winreg
    
        HKLM = winreg.HKEY_LOCAL_MACHINE
        subkey = r'SOFTWARE\Microsoft\SystemCertificates\CA'
        access = winreg.KEY_ALL_ACCESS
        >>> key = winreg.OpenKey(HKLM, subkey, 0, access)    
        >>> print(key)
        <PyHKEY:0x0000000000000178>
        >>> key.Close()

    Repeat with low integrity level:

        >>> with token_integrity_level('low'):
        ...      winreg.OpenKey(HKLM, subkey, 0, access)
        ...
        Traceback (most recent call last):
          File "<stdin>", line 2, in <module>
        PermissionError: [WinError 5] Access is denied 

    A context manager like this could be added to the test helper module that was proposed in bpo-22080. It could also add the ability to impersonate with a restricted copy of the process token -- like what UAC creates. psexec -l does this by calling CreateRestrictedToken followed by SetInformationToken for the TokenIntegrityLevel and TokenDefaultDacl.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 18, 2016

    New changeset 8ff4c1827499 by Benjamin Peterson in branch '3.5':
    merge 3.4 (closes bpo-25939)
    https://hg.python.org/cpython/rev/8ff4c1827499

    New changeset d6474257ef38 by Benjamin Peterson in branch 'default':
    merge 3.5 (closes bpo-25939)
    https://hg.python.org/cpython/rev/d6474257ef38

    @python-dev python-dev mannequin closed this as completed Feb 18, 2016
    @benjaminp
    Copy link
    Contributor

    The patch itself seems fine, so I committed that. It doesn't seem like how best to test this has been figured out, so leaving the issue open.

    @benjaminp benjaminp reopened this Feb 18, 2016
    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Feb 26, 2016

    Thanks for accepting my patch. I'm curious: any reason not applying to 2.7 branch? We're building youtube-dl.exe with py2exe on Python 2.7 as py2exe on 3.x sometimes fails. (ytdl-org/youtube-dl#5094)

    @zooba
    Copy link
    Member

    zooba commented Feb 26, 2016

    It was fixed in 2.7 - https://hg.python.org/cpython/rev/3cddcf471c70

    The issue number wasn't in the commit, so it didn't appear here.

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Feb 27, 2016

    Didn't see it. Sorry for bothering.

    @tiran
    Copy link
    Member

    tiran commented Sep 8, 2016

    Benjamin, Steve, can we close this ticket?

    @zooba
    Copy link
    Member

    zooba commented Sep 8, 2016

    Yes, and done.

    @zooba zooba closed this as completed Sep 8, 2016
    @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
    extension-modules C modules in the Modules dir OS-windows type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants