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
No SSL match_hostname() in ftp, imap, nntp, pop, smtp modules #63708
Comments
None of the TLS/SSL classes for ftp, imap, nntp, pop and smtp have support for match_hostname() in order to verify that the SSL cert matches the host name. I'm not sure how we can handle the problem without creating backwards incompatibilities. |
The patch implements check_hostname in order to match SSL certs with the peer's hostname in ftp, imap, nntp, pop and smtp library. So far the patch needs more tests and doc updates. I consider the new feature a security fix. Right now everybody with any valid TLS/SSL certificate can claim that its certificate is valid for 'smtp.google.com'. |
I don't know enough about SSL to make a "feature vs bug/security fix" ruling on this. Can a second person who does--maybe Bill Janssen?--chime in? |
I agree with Christian, mail.stufft.io should not be able to masquerade as smtp.google.com and being able to do so is a pretty big security hole. |
I think adding an API in bugfix releases must be an exceptional decision and I'm honestly not convinced this issue justifies it. |
Probably the higher level libraries don't even realize it's not happening, it's similar to the issue of SSL validation for HTTPS connections where a vast swathe of people didn't even realize that their certificates weren't being validated. |
I agree with Antoine. The five attached bug reports only refer to Python 3.4. |
Exactly. And we didn't add certificate checking in bugfix releases in Now I appreciate that it would be a bit of a pity to wait for 3.5 to add |
For a true security fix, the default for check_hostname would have to be True. However, that will create a lot of backward compatibility problems for questionable gain. I think Larry should make an exception for 3.4 and allow this new feature. |
Okay, you have my permission to add match_hostname() and fix the security hole you cite. Can you have it done soon, so it can bake for a while in trunk before we cut beta 2? |
I assumed we were talking about 3.4 and didn't even notice that the issues had 3.3 and 3.2 attached to it. |
My patch could be much simpler and easier if we could just drop support for ancient versions of OpenSSL. My idea requires at least OpenSSL 0.9.8f (release 2007) with SNI support. Six years are a lot for crypto software. All relevant platforms with vendor support have a more recent version of OpenSSL, too.
server_hostname is used to for server name indicator (SNI) as well as the hostname for match_hostname(). It would remove lots and lots of code duplication, too. The check_hostname takes care about invalid combinations, too: >>> context = ssl.SSLContext(ssl.PROTOCOL_TLSv1)
>>> context.verify_mode == ssl.CERT_NONE
True
>>> context.check_hostname = True
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ValueError: check_hostname needs a SSL context with either CERT_OPTIONAL or CERT_REQUIRED |
What are you talking about? Your patch doesn't use HAS_SNI. |
The patches in the dependency tickets are using SNI. The problem is, a non-None server_hostname argument raises an error when OpenSSL doesn't support the feature. Here is a demo patch for my idea. It makes it very easy to add hostname matching to existing code. All it takes is the "server_hostname" argument to wrap_socket() and a new property "check_hostname" for the SSLContext object. The rest is done in do_handshake(). |
They all check for HAS_SNI, which is simple enough. If you think your patches are not ready, then please say so, so that I don't review them. |
New changeset aa531135bc6b by Christian Heimes in branch 'default': |
I have addressed the five libraries in five sub-issues. http.client and asyncio.selector_events need small adjustments to use the new feature. Please review the patch. |
The asyncio patch looks fine, but I'd like to see a unittest that actually fails (or mocks failing) the hostname check where it is now performed (in wrap_socket() IIUC?), to make sure that the exception is propagated out properly. |
I'd rather have integration test with a real SSL connection in order to check the entire stack. asyncio doesn't have a test for CERT_REQUIRED yet. The attached patch tests three common cases: missing CA, missing server_hostname and a successful connection with full verification. |
PS: The test uses keycert3.pem and pycacert.pem from the 3.4 test directory. keycert3.pem contains privat + public key and is signed by the CA in pycacert.pem. |
I left my comment on the review. I forgot to mail the review. |
See my messages on the review. Is it absolutely necessary to close the socket when the hostname verification fails? |
New changeset b6fce698e467 by Christian Heimes in branch 'default': |
New changeset 1a945fb875bf by Christian Heimes in branch 'default': |
The new patch splits the test into three separate test cases. Guido, Python 3.3 doesn't have the necessary CA and server cert files. The files were added to 3.4 for some other tests. The patch tries to load the files from 3.4's test directory and falls back to local copies. You have to copy/delete these files to test the patch: $ hg rm Lib/test/test_asyncio/sample.*
$ hg cp Lib/test/ssl_key.pem Lib/test/ssl_cert.pem Lib/test/pycacert.pem Lib/test/keycert3.pem Lib/test/test_asyncio/ |
Thanks Christian, this is almost right. I'm uploading a patch that covers all bases:
|
New changeset 1605eda93392 by Christian Heimes in branch 'default': |
With the latest (revision 1605eda93392) I get four failures on OS X. Three are like this (in all three selector types -- kqueue, select, poll): ====================================================================== Traceback (most recent call last):
File "tests/test_events.py", line 532, in test_create_ssl_connection
tr, pr = self.loop.run_until_complete(f)
File "/Users/guido/tulip/asyncio/base_events.py", line 177, in run_until_complete
return future.result()
File "/Users/guido/tulip/asyncio/futures.py", line 221, in result
raise self._exception
File "/Users/guido/tulip/asyncio/tasks.py", line 276, in _step
result = coro.throw(exc)
File "/Users/guido/tulip/asyncio/base_events.py", line 388, in create_connection
yield from waiter
File "/Users/guido/tulip/asyncio/futures.py", line 320, in __iter__
yield self # This tells Task to wait for completion.
File "/Users/guido/tulip/asyncio/tasks.py", line 329, in _wakeup
value = future.result()
File "/Users/guido/tulip/asyncio/futures.py", line 221, in result
raise self._exception
File "/Users/guido/tulip/asyncio/selector_events.py", line 618, in _on_handshake
self._sock.do_handshake()
File "/usr/local/lib/python3.4/ssl.py", line 748, in do_handshake
self._sslobj.do_handshake()
ssl.SSLEOFError: EOF occurred in violation of protocol (_ssl.c:599) The last is similar in test_streams.py: ====================================================================== Traceback (most recent call last):
File "tests/test_streams.py", line 58, in test_open_connection_no_loop_ssl
reader, writer = self.loop.run_until_complete(f)
File "/Users/guido/tulip/asyncio/base_events.py", line 177, in run_until_complete
return future.result()
File "/Users/guido/tulip/asyncio/futures.py", line 221, in result
raise self._exception
File "/Users/guido/tulip/asyncio/tasks.py", line 276, in _step
result = coro.throw(exc)
File "/Users/guido/tulip/asyncio/streams.py", line 43, in open_connection
lambda: protocol, host, port, **kwds)
File "/Users/guido/tulip/asyncio/base_events.py", line 388, in create_connection
yield from waiter
File "/Users/guido/tulip/asyncio/futures.py", line 320, in __iter__
yield self # This tells Task to wait for completion.
File "/Users/guido/tulip/asyncio/tasks.py", line 329, in _wakeup
value = future.result()
File "/Users/guido/tulip/asyncio/futures.py", line 221, in result
raise self._exception
File "/Users/guido/tulip/asyncio/selector_events.py", line 618, in _on_handshake
self._sock.do_handshake()
File "/usr/local/lib/python3.4/ssl.py", line 748, in do_handshake
self._sslobj.do_handshake()
ssl.SSLEOFError: EOF occurred in violation of protocol (_ssl.c:599) I get the same failures when I copy the changes to the Tulip repo. |
That's fixed by revision ec1e7fc9b5a4. Christian, can this bug be closed now, or is there more? |
I'm closing this ticket. All features have been implemented and tests are passing, too. I have created bpo-19909 as a reminder for doc improvements. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: