classification
Title: No SSL match_hostname() in ftp, imap, nntp, pop, smtp modules
Type: security Stage: resolved
Components: Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: 19781 19782 19783 19784 19785 Superseder:
Assigned To: Nosy List: Arfrever, christian.heimes, dstufft, georg.brandl, giampaolo.rodola, gvanrossum, janssen, larry, pitrou, python-dev, vajrasky
Priority: high Keywords: patch

Created on 2013-11-05 22:57 by christian.heimes, last changed 2013-12-06 13:16 by christian.heimes. This issue is now closed.

Files
File name Uploaded Description Edit
sslctx_check_hostname.patch christian.heimes, 2013-11-28 17:25 review
check_hostname_adjustments.patch christian.heimes, 2013-12-02 20:20 review
asyncio_ssl_verify.patch christian.heimes, 2013-12-04 07:14 review
asyncio_ssl_verify2.patch christian.heimes, 2013-12-05 16:13 review
asyncio_ssl_verify3.patch gvanrossum, 2013-12-05 20:47 review
Messages (30)
msg202246 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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 "<stdin>", line 1, in <module>
ValueError: check_hostname needs a SSL context with either CERT_OPTIONAL or CERT_REQUIRED
msg204673 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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.
History
Date User Action Args
2013-12-06 13:16:29christian.heimessetstatus: open -> closed
resolution: fixed
messages: + msg205368

stage: patch review -> resolved
2013-12-06 04:10:08gvanrossumsetmessages: + msg205351
2013-12-06 00:04:48gvanrossumsetmessages: + msg205338
2013-12-05 23:29:09python-devsetmessages: + msg205336
2013-12-05 20:47:39gvanrossumsetfiles: + asyncio_ssl_verify3.patch

messages: + msg205319
2013-12-05 16:13:37christian.heimessetfiles: + asyncio_ssl_verify2.patch

messages: + msg205309
2013-12-05 06:53:46python-devsetmessages: + msg205279
2013-12-04 19:46:29python-devsetmessages: + msg205241
2013-12-04 18:24:43gvanrossumsetmessages: + msg205231
2013-12-04 07:30:57vajraskysetnosy: + vajrasky
messages: + msg205203
2013-12-04 07:15:20christian.heimessetmessages: + msg205199
2013-12-04 07:14:17christian.heimessetfiles: + asyncio_ssl_verify.patch

messages: + msg205198
2013-12-02 22:58:44gvanrossumsetmessages: + msg205064
2013-12-02 20:20:04christian.heimessetfiles: + check_hostname_adjustments.patch
nosy: + gvanrossum
messages: + msg205052

2013-12-02 01:58:21python-devsetnosy: + python-dev
messages: + msg204989
2013-11-28 17:36:05pitrousetmessages: + msg204683
2013-11-28 17:25:38christian.heimessetfiles: + sslctx_check_hostname.patch

messages: + msg204681
2013-11-28 17:05:06christian.heimessetfiles: - check_hostname.patch
2013-11-28 15:45:57pitrousetmessages: + msg204673
2013-11-28 15:38:20christian.heimessetmessages: + msg204672
2013-11-26 20:41:02dstufftsetmessages: + msg204532
2013-11-26 20:30:02pitrousetstage: needs patch -> patch review
versions: - Python 3.2, Python 3.3
2013-11-26 20:12:23larrysetmessages: + msg204526
2013-11-26 19:57:47georg.brandlsetmessages: + msg204524
2013-11-26 19:56:38pitrousetmessages: + msg204523
2013-11-26 19:56:02christian.heimessetmessages: + msg204522
2013-11-26 19:51:36dstufftsetmessages: + msg204521
2013-11-26 19:50:39pitrousetmessages: + msg204520
2013-11-26 19:50:25dstufftsetnosy: + dstufft
messages: + msg204519
2013-11-26 19:47:48larrysetmessages: + msg204518
2013-11-26 02:24:21Arfreversetnosy: + Arfrever
2013-11-25 23:48:56christian.heimessetdependencies: + No SSL match_hostname() in ftplib, No SSL match_hostname() in imaplib, No SSL match_hostname() in nntplib, No SSL match_hostname() in poplib, No SSL match_hostname() in smtplib
2013-11-25 09:48:57christian.heimessetfiles: + check_hostname.patch

nosy: + georg.brandl, larry
messages: + msg204318

keywords: + patch
2013-11-05 22:57:12christian.heimescreate