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

[yan12125]

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]

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

[zooba]

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]

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]

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]

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]

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

[zooba]

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]

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]

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]

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

[benjaminp]

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.