| msg202246 - (view) |
Author: Christian Heimes (christian.heimes) *  |
Date: 2013-11-05 22:57 |
| 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. |
|
|
| msg204318 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2013-11-25 09:48 |
| 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'. |
|
|
| msg204518 - (view) |
Author: Larry Hastings (larry) * |
Date: 2013-11-26 19:47 |
| 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? |
|
|
| msg204519 - (view) |
Author: Donald Stufft (dstufft) * |
Date: 2013-11-26 19:50 |
| 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. |
|
|
| msg204520 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2013-11-26 19:50 |
| I think adding an API in bugfix releases must be an exceptional decision and I'm honestly not convinced this issue justifies it. It'd be more convincing if there was actually demand for this feature (e.g. from higher-level library authors). |
|
|
| msg204521 - (view) |
Author: Donald Stufft (dstufft) * |
Date: 2013-11-26 19:51 |
| 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. |
|
|
| msg204522 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2013-11-26 19:56 |
| I agree with Antoine. The five attached bug reports only refer to Python 3.4. |
|
|
| msg204523 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2013-11-26 19:56 |
| > 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. Exactly. And we didn't add certificate checking in bugfix releases in the name of security. Now I appreciate that it would be a bit of a pity to wait for 3.5 to add this, so perhaps Larry wants to make an exception to the 3.4 feature freeze so that this feature can come in (assuming Christian's patch is ready). |
|
|
| msg204524 - (view) |
Author: Georg Brandl (georg.brandl) * |
Date: 2013-11-26 19:57 |
| 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. |
|
|
| msg204526 - (view) |
Author: Larry Hastings (larry) * |
Date: 2013-11-26 20:12 |
| 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? |
|
|
| msg204532 - (view) |
Author: Donald Stufft (dstufft) * |
Date: 2013-11-26 20:41 |
| 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. |
|
|
| msg204672 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2013-11-28 15:38 |
| 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. >>> context = ssl.SSLContext(ssl.PROTOCOL_TLSv1) >>> context.verify_mode = ssl.CERT_REQUIRED >>> context.check_hostname = True >>> context.wrap_socket(sock, server_hostname="www.example.org") 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 "", line 1, in ValueError: check_hostname needs a SSL context with either CERT_OPTIONAL or CERT_REQUIRED |
|
|
| msg204673 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2013-11-28 15:45 |
| > 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. What are you talking about? Your patch doesn't use HAS_SNI. |
|
|
| msg204681 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2013-11-28 17:25 |
| 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(). |
|
|
| msg204683 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2013-11-28 17:36 |
| > The patches in the dependency tickets are using SNI. They all check for HAS_SNI, which is simple enough. If you want to discuss a minimum supported OpenSSL version, that's fine with me, but it should be a separate discussion (on python-dev?). If you think your patches are not ready, then please say so, so that I don't review them. |
|
|
| msg204989 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2013-12-02 01:58 |
| New changeset aa531135bc6b by Christian Heimes in branch 'default': Issue #19509: Add SSLContext.check_hostname to match the peer's certificate http://hg.python.org/cpython/rev/aa531135bc6b |
|
|
| msg205052 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2013-12-02 20:20 |
| 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. |
|
|
| msg205064 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2013-12-02 22:58 |
| 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. |
|
|
| msg205198 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2013-12-04 07:14 |
| 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. |
|
|
| msg205199 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2013-12-04 07:15 |
| 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. |
|
|
| msg205203 - (view) |
Author: Vajrasky Kok (vajrasky) * |
Date: 2013-12-04 07:30 |
| I left my comment on the review. I forgot to mail the review. |
|
|
| msg205231 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2013-12-04 18:24 |
| See my messages on the review. Is it absolutely necessary to close the socket when the hostname verification fails? |
|
|
| msg205241 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2013-12-04 19:46 |
| New changeset b6fce698e467 by Christian Heimes in branch 'default': Issue #19509: Don't close the socket in do_handshake() when hostname verification fails. http://hg.python.org/cpython/rev/b6fce698e467 |
|
|
| msg205279 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2013-12-05 06:53 |
| New changeset 1a945fb875bf by Christian Heimes in branch 'default': Issue 19509: Don't call match_hostname() twice in http.client. http://hg.python.org/cpython/rev/1a945fb875bf |
|
|
| msg205309 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2013-12-05 16:13 |
| 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/ |
|
|
| msg205319 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2013-12-05 20:47 |
| Thanks Christian, this is almost right. I'm uploading a patch that covers all bases: - Works on Python 3.3 (TEST_HOME_DIR isn't defined there) - Works on older Python 3.4 versions (check_hostname may not exist) - Works on latest Python 3.4 version |
|
|
| msg205336 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2013-12-05 23:29 |
| New changeset 1605eda93392 by Christian Heimes in branch 'default': Issue #19509: Finish implementation of check_hostname http://hg.python.org/cpython/rev/1605eda93392 |
|
|
| msg205338 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2013-12-06 00:04 |
| With the latest (revision 1605eda93392) I get four failures on OS X. Three are like this (in all three selector types -- kqueue, select, poll): ====================================================================== ERROR: test_create_ssl_connection (test_events.SelectEventLoopTests) ---------------------------------------------------------------------- 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: ====================================================================== ERROR: test_open_connection_no_loop_ssl (test_streams.StreamReaderTests) ---------------------------------------------------------------------- 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. |
|
|
| msg205351 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2013-12-06 04:10 |
| That's fixed by revision ec1e7fc9b5a4. Christian, can this bug be closed now, or is there more? |
|
|
| msg205368 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2013-12-06 13:16 |
| I'm closing this ticket. All features have been implemented and tests are passing, too. I have created #19909 as a reminder for doc improvements. |
|
|