classification
Title: Make SSL suppress_ragged_eofs default more secure
Type: security Stage:
Components: Library (Lib), SSL Versions: Python 3.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: alex, christian.heimes, haypo, martin.panter
Priority: normal Keywords: patch

Created on 2016-08-20 13:19 by martin.panter, last changed 2017-09-12 21:37 by martin.panter.

Files
File name Uploaded Description Edit
truncating_proxy.py martin.panter, 2016-08-20 13:19
ragged-eofs.patch martin.panter, 2016-09-22 09:24 review
ragged-eofs.v2.patch martin.panter, 2016-10-15 23:30 review
Messages (7)
msg273208 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-08-20 13:19
In the SSL module, the wrap_socket() function (and corresponding SSLContext method) take a flag called “suppress_ragged_eofs”. It defaults to True, which makes me uncomfortable. The documentation says:

'''
The parameter “suppress_ragged_eofs” specifies how the SSLSocket.recv() method should signal unexpected EOF from the other end of the connection. If specified as True (the default), it returns a normal EOF (an empty bytes object) in response to unexpected EOF errors raised from the underlying socket; if False, it will raise the exceptions back to the caller.
'''

I understand the “unexpected EOF error” happens when the underlying socket indicates EOF, but the connection was not shut down at the SSL protocol level. As well as EOF from the other end of the connection, it can happen due to a proxy, or anything else on the network that can affect the connection. I think it may be better report this error by default, just like other unsecured network-level errors like connection reset or timeout. Otherwise it is too easy to treat this insecure EOF condition as if it were a secure EOF signal of the remote peer.

The flag was added as part of r64578, for Issue 1223, in Python 2.6. The reason given in that bug report was to help Python work with a HTTP server. However my theory is the server was closing the connection wrongly; if so, the server should have been fixed, rather than Python.

There is plenty of precedence for using suppress_ragged_eofs=True with HTTPS servers. As well as Issue 1223, there is Issue 494762 and Issue 500311. And in my experiments, Curl and Firefox both seem to treat the error the same as a secure EOF. Maybe there should be a way to keep supporting HTTPS servers that trigger the error (though I would rather not by default).

Attached is a HTTP proxy server that will let you break a connection after returning a set number of bytes (or any time by killing the server), which can trigger the error condition.

Example output of proxy server:
$ python truncating_proxy.py --limit 12345
Proxy server at http://127.0.0.1:46687
Proxying connection to www.python.org:443
Forwarded 12345 B to client

Python 2’s httplib module does not treat a short HTTP response as an error, so the following request gets truncated without much indication of a problem:
$ https_proxy=http://127.0.0.1:46687 python2 -c 'import urllib2; print(urllib2.urlopen("https://www.python.org/").read())'
<!doctype html>
[. . .]
        <!--[if lt IE 8]>
        <div id="oldie-warning" class
msg277212 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-09-22 09:24
I have been experimenting with a patch that changes the default to suppress_ragged_eofs=False.

One disadvantage of this change is it could make servers less robust. E.g. in the tests, I explicitly enabled suppress_ragged_eofs=True in a server, because otherwise I would have to add extra cleanup if an exception is raised on the server side. In another test server, based on socketserver, I added special code to silence logging an SSLEOFError exception (a side effect of a particular test case).

But ideally, real-world servers should already handle other exceptions that are triggered outside the server’s control like, ECONNRESET and TLSV1_ALERT_UNKNOWN_CA. Socketserver already logs all these kinds of errors. Plus I think SSLEOFError has always been possible in the handshake phase.

With HTTP I didn’t have to go further than Google to find a server that terminates the response with a non-SSL shutdown (although because truncated JSON is invalid, it is not a big deal). For the record, here is a stripped-down demo. The same problem also happens for a more complete request with valid parameters.

>>> import socket, ssl
>>> s = socket.create_connection(("accounts.google.com", 443))
>>> ss = ssl.wrap_socket(s, suppress_ragged_eofs=False)
>>> ss.sendall(b"POST /o/oauth2/token HTTP/1.1\r\n"
...     b"Host: accounts.google.com\r\n"
...     b"Content-Length: 0\r\n"
...     b"Connection: close\r\n"
...     b"\r\n")
>>> print("\n".join(map(repr, ss.recv(3000).splitlines(keepends=True))))
b'HTTP/1.1 400 Bad Request\r\n'
b'Content-Type: application/json; charset=utf-8\r\n'
b'Cache-Control: no-cache, no-store, max-age=0, must-revalidate\r\n'
b'Pragma: no-cache\r\n'
b'Expires: Mon, 01 Jan 1990 00:00:00 GMT\r\n'
b'Date: Thu, 22 Sep 2016 03:10:20 GMT\r\n'
b'X-Content-Type-Options: nosniff\r\n'
b'X-Frame-Options: SAMEORIGIN\r\n'
b'X-XSS-Protection: 1; mode=block\r\n'
b'Server: GSE\r\n'
b'Alt-Svc: quic=":443"; ma=2592000; v="36,35,34,33,32"\r\n'
b'Accept-Ranges: none\r\n'
b'Vary: Accept-Encoding\r\n'
b'Connection: close\r\n'
b'\r\n'
b'{\n'
b'  "error" : "invalid_request",\n'
b'  "error_description" : "Required parameter is missing: grant_type"\n'
b'}'
>>> ss.recv(3000)  # HTTP-level client does not know that this is the EOF yet
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/proj/python/cpython/Lib/ssl.py", line 987, in recv
    return self.read(buflen)
  File "/home/proj/python/cpython/Lib/ssl.py", line 865, in read
    return self._sslobj.read(len, buffer)
  File "/home/proj/python/cpython/Lib/ssl.py", line 627, in read
    v = self._sslobj.read(len)
ssl.SSLEOFError: EOF occurred in violation of protocol (_ssl.c:2176)

In this case, if the client does not send “Connection: close”, the server uses chunked encoding and there is no problem. So this is another instance of Issue 12849 (Python’s unusual request triggering a server bug).

I wonder if a solution would be to use suppress_ragged_eofs=False by default, but add a way to let the user of the http.client module explicitly allow SSLEOFError to signal a proper EOF.
msg278737 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-10-15 23:30
Patch v2 also adds a new attribute to context objects. With this I can work around my Google server bug:

context = ssl.create_default_context(ssl.Purpose.SERVER_AUTH)
context.suppress_ragged_eofs = True
handler = urllib.request.HTTPSHandler(context=context)
urlopen = urllib.request.build_opener(handler).open
urlopen(Request(url="https://accounts.google.com/o/oauth2/token", ...))
msg301711 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-09-08 18:13
I don't consider myself qualified enough to make a decision. Alex, Victor, what do you think?
msg301751 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-09-08 22:49
Martin: can you please create a pull request? It would be easier to review your change.
msg301752 - (view) Author: Alex Gaynor (alex) * (Python committer) Date: 2017-09-08 22:51
Mmmm, my understanding is that ignoring TCP-FIN/RST-without-TLS-closenotify is pretty common for a lot of different clients.

We should probably survey the landscape, see what both browsers and non-browse clients (e.g. curl) do before making a decision.
msg301996 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2017-09-12 21:37
Even if some use cases depend on suppress_ragged_eofs=True, I think it is best to avoid that as the default. There could be a deprecation period if necessary.

I tested some HTTP clients I had handy. In summary, most seemed to handle a truncation attack on the Set-Cookie field sensibly (but Python is vulnerable without my patch or other solution). On the other hand, all the clients I tried handled one case of an insecurely-truncated body the same as a normal EOF (while I propose to treat this as an error).

The clients I tested: Firefox/46.0, curl/7.43.0, Wget/1.13.4, Links 2.12, ELinks/0.13.GIT, Python-urllib/3.5 (unpatched), and Python-urllib/3.7 with my patch. I tried three test cases:

1. Truncate Set-Cookie field, with no terminating newline. The client should not accept the cookie, in case an attribute such as “Secure” was removed, like in <https://bugs.chromium.org/p/chromium/issues/detail?id=244260>.
>>> c.sendall(b"Set-Cookie: COOKIE=meant-to-be-Secure")
>>> c.shutdown(SHUT_RDWR)

Python (unpatched) treats the Set-Cookie field as valid. It appears in the HTTPResponse object, with no clue that it was truncated. Wget was also vulnerable. Firefox and Curl did not record the cookie, but did not indicate any error either. Links does not support cookies, while Elinks tried 3 times and reported an error.

2. Content-Length response with truncated text/html. IMO a client should inform the user that the response was cut off (with or without SSL), but sometimes the user may want to see the first half of the response anyway.
>>> c.sendall(b"Content-Length: 100\r\n\r\n" b"Truncat")
>>> c.shutdown(SHUT_RDWR)

Curl, wget, Links and Elinks all outputted the incomplete response, and reported an error. Firefox displayed the truncated page without indicating any error. In most cases, Python raised an IncompleteRead exception, but it depended on the pattern of read calls, and some or all of the truncated data was hidden in the undocumented “IncompleteRead.partial” attribute.

3. “Connection: close” response with truncated HTML:
>>> c.sendall(b"Connection: close\r\n\r\n" b"Truncat")
>>> c.shutdown(SHUT_RDWR)

This is the case where all the clients (other than my patched Python) treated this like a valid non-truncated response. But IMO in general it should be dealt with like the Content-Length case if the SSL level wasn’t shut down properly.

Victor: Sorry, I’m unlikely to make a Git Hub pull request any time soon, but I don’t mind if someone else does.
History
Date User Action Args
2017-09-12 21:37:08martin.pantersetmessages: + msg301996
2017-09-08 22:51:53alexsetmessages: + msg301752
2017-09-08 22:49:08hayposetmessages: + msg301751
2017-09-08 18:13:22christian.heimessetassignee: christian.heimes ->

messages: + msg301711
nosy: + haypo, alex
2016-10-15 23:30:19martin.pantersetfiles: + ragged-eofs.v2.patch

messages: + msg278737
2016-09-22 09:24:50martin.pantersetfiles: + ragged-eofs.patch
keywords: + patch
messages: + msg277212

versions: + Python 3.7, - Python 3.6
2016-09-15 08:01:10christian.heimessetassignee: christian.heimes

components: + SSL
nosy: + christian.heimes
2016-08-20 13:19:11martin.pantercreate