Repository navigation
No SSL match_hostname() in ftp, imap, nntp, pop, smtp modules #63708
Description
| BPO | 19509 |
|---|---|
| Nosy | @gvanrossum, @birkenfeld, @pitrou, @larryhastings, @giampaolo, @tiran, @dstufft, @vajrasky |
| Dependencies | |
| Files |
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 2013-12-06.13:16:29.681>
created_at = <Date 2013-11-05.22:57:12.554>
labels = ['type-security']
title = 'No SSL match_hostname() in ftp, imap, nntp, pop, smtp modules'
updated_at = <Date 2013-12-06.13:16:29.680>
user = 'https://github.com/tiran'bugs.python.org fields:
activity = <Date 2013-12-06.13:16:29.680>
actor = 'christian.heimes'
assignee = 'none'
closed = True
closed_date = <Date 2013-12-06.13:16:29.681>
closer = 'christian.heimes'
components = []
creation = <Date 2013-11-05.22:57:12.554>
creator = 'christian.heimes'
dependencies = ['19781', '19782', '19783', '19784', '19785']
files = ['32881', '32940', '32967', '32991', '32996']
hgrepos = []
issue_num = 19509
keywords = ['patch']
message_count = 30.0
messages = ['202246', '204318', '204518', '204519', '204520', '204521', '204522', '204523', '204524', '204526', '204532', '204672', '204673', '204681', '204683', '204989', '205052', '205064', '205198', '205199', '205203', '205231', '205241', '205279', '205309', '205319', '205336', '205338', '205351', '205368']
nosy_count = 11.0
nosy_names = ['gvanrossum', 'georg.brandl', 'janssen', 'pitrou', 'larry', 'giampaolo.rodola', 'christian.heimes', 'Arfrever', 'python-dev', 'dstufft', 'vajrasky']
pr_nums = []
priority = 'high'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue19509'
versions = ['Python 3.4']
Activity
tiran commentedon Nov 5, 2013
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.
tiran commentedon Nov 25, 2013
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'.
larryhastings commentedon Nov 26, 2013
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?
dstufft commentedon Nov 26, 2013
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.
pitrou commentedon Nov 26, 2013
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).
dstufft commentedon Nov 26, 2013
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.
tiran commentedon Nov 26, 2013
I agree with Antoine. The five attached bug reports only refer to Python 3.4.
pitrou commentedon Nov 26, 2013
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).
birkenfeld commentedon Nov 26, 2013
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.
larryhastings commentedon Nov 26, 2013
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?
dstufft commentedon Nov 26, 2013
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.
tiran commentedon Nov 28, 2013
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:
pitrou commentedon Nov 28, 2013
What are you talking about? Your patch doesn't use HAS_SNI.
tiran commentedon Nov 28, 2013
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().
3 remaining items
gvanrossum commentedon Dec 2, 2013
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.
tiran commentedon Dec 4, 2013
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.
tiran commentedon Dec 4, 2013
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.
vajrasky commentedon Dec 4, 2013
I left my comment on the review. I forgot to mail the review.
gvanrossum commentedon Dec 4, 2013
See my messages on the review. Is it absolutely necessary to close the socket when the hostname verification fails?
python-dev commentedon Dec 4, 2013
New changeset b6fce698e467 by Christian Heimes in branch 'default':
Issue bpo-19509: Don't close the socket in do_handshake() when hostname verification fails.
http://hg.python.org/cpython/rev/b6fce698e467
python-dev commentedon Dec 5, 2013
New changeset 1a945fb875bf by Christian Heimes in branch 'default':
bpo-19509: Don't call match_hostname() twice in http.client.
http://hg.python.org/cpython/rev/1a945fb875bf
tiran commentedon Dec 5, 2013
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/gvanrossum commentedon Dec 5, 2013
Thanks Christian, this is almost right. I'm uploading a patch that covers all bases:
python-dev commentedon Dec 5, 2013
New changeset 1605eda93392 by Christian Heimes in branch 'default':
Issue bpo-19509: Finish implementation of check_hostname
http://hg.python.org/cpython/rev/1605eda93392
gvanrossum commentedon Dec 6, 2013
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)
----------------------------------------------------------------------
The last is similar in test_streams.py:
======================================================================
ERROR: test_open_connection_no_loop_ssl (test_streams.StreamReaderTests)
----------------------------------------------------------------------
I get the same failures when I copy the changes to the Tulip repo.
gvanrossum commentedon Dec 6, 2013
That's fixed by revision ec1e7fc9b5a4.
Christian, can this bug be closed now, or is there more?
tiran commentedon Dec 6, 2013
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.