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

requests[security] / PyOpenSSL does not make requests more secure but more brittle #5267

Closed
tiran opened this issue Nov 18, 2019 · 3 comments

Comments

@tiran
Copy link
Contributor

tiran commented Nov 18, 2019

Requests added the requests[security] dependency many, many years ago because the ssl module in Python's stdlib was lacking some features. Since PEP 466 has been implemented in Python 2.7.9, the ssl module supports hostname verification and SNI. 2.7.9 was released in 2014. Python 3.7.0 improved the situation even further by delegating hostname verification to OpenSSL. The PyOpenSSL compatibility layer in urllib3 uses the deprecated ssl.match_hostname function or a backport.

Unconditional monkey-patching of urllib3 with urllib3.contrib.pyopenssl.inject_into_urllib3 is causing issues, too.

PyOpenSSL uses dynamic libffi callbacks (also known as cffi old-style callbacks). The callbacks are implemented with trampolines and dynamic creation of native machine code. This either uses executable and writeable memory pages or some hacks with shared mmap() regions. Dynamic callbacks are a security risk and blocked by security frameworks like SELinux for good reasons (deny_execmem). Armin Rigo and I explored various ways to work around these problems, but there is simply solution. Eventually Armin implemented a new callback system for cffi. Some of the issues with old callbacks are documented at https://cffi.readthedocs.io/en/latest/using.html#callbacks-old-style .

PyOpenSSL currently pulls in a versions of cryptography, which itself depends on asn1crypto. A problem with asn1crypto and ctypes causes Python to segfault on recent macOS, pyca/pyopenssl#874 and wbond/asn1crypto#158 .

Due to bugs like https://bugzilla.redhat.com/show_bug.cgi?id=1535689 I convinced the Fedora and RHEL maintainers to patch requests and drop inject_into_urllib3 a while ago. The distros have been running with patch https://src.fedoraproject.org/rpms/python-requests/blob/master/f/Don-t-inject-pyopenssl-into-urllib3.patch for over a year without any reported issues.

I propose to:

  1. Remove requests[security] extra requires.
  2. Remove unconditionally monkey-patch of urllib3 from requests.
@sethmlarson
Copy link
Member

A data-point to support this direction for requests and urllib3: 91% (7,298,352 / 7,993,356) of Python 2.7.X downloads for urllib3 were on Python 2.7.9+. Taken from the public download data-set for today.

@tiran
Copy link
Contributor Author

tiran commented Nov 18, 2019

The ratio is even larger. Python 2.7.5 on RHEL 7 machines have a backport of PEP 466, too.

$ rpm -qa python
python-2.7.5-88.el7.x86_64
$ python
Python 2.7.5 (default, Sep 26 2019, 13:23:47) 
[GCC 4.8.5 20150623 (Red Hat 4.8.5-39)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import ssl
>>> ssl.SSLContext(ssl.PROTOCOL_SSLv23).check_hostname
False

@nateprewitt
Copy link
Member

It looks like this wasn't closed after merging #5443. I think the only outstanding item is requests[security]. I'm inclined to leave it for now for whomever is still out there using < Python 2.7.9. Given we will likely be releasing a new minor version this week with urllib3, we can add a deprecation notice on the extra for the next minor release.

I'll go ahead and resolve this tomorrow unless anyone has more feedback. I think the initial request is now resolved.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants