# HG changeset patch # Parent e5888f5b9cf899f05d4f37983db76b9de6473368 Issue #27815: Disable the SSL suppress_ragged_eofs setting by default diff -r e5888f5b9cf8 Doc/library/ssl.rst --- a/Doc/library/ssl.rst Wed Sep 21 19:35:54 2016 +0300 +++ b/Doc/library/ssl.rst Sat Oct 15 23:28:09 2016 +0000 @@ -143,7 +143,7 @@ Python 3.2, it can be more flexible to use :meth:`SSLContext.wrap_socket` instead. -.. function:: wrap_socket(sock, keyfile=None, certfile=None, server_side=False, cert_reqs=CERT_NONE, ssl_version={see docs}, ca_certs=None, do_handshake_on_connect=True, suppress_ragged_eofs=True, ciphers=None) +.. function:: wrap_socket(sock, keyfile=None, certfile=None, server_side=False, cert_reqs=CERT_NONE, ssl_version={see docs}, ca_certs=None, do_handshake_on_connect=True, suppress_ragged_eofs=False, ciphers=None) Takes an instance ``sock`` of :class:`socket.socket`, and returns an instance of :class:`ssl.SSLSocket`, a subtype of :class:`socket.socket`, which wraps @@ -225,15 +225,18 @@ blocking behavior of the socket I/O involved in the handshake. The parameter ``suppress_ragged_eofs`` specifies how the - :meth:`SSLSocket.recv` method should signal unexpected EOF from the other end - of the connection. If specified as :const:`True` (the default), it returns a + :meth:`SSLSocket.recv` method should handle the connection being shut + down outside the protocol. If specified as :const:`True`, it returns a normal EOF (an empty bytes object) in response to unexpected EOF errors - raised from the underlying socket; if :const:`False`, it will raise the - exceptions back to the caller. + raised from the underlying socket; if :const:`False`, it will raise + :exc:`SSLEOFError` back to the caller. .. versionchanged:: 3.2 New optional argument *ciphers*. + .. versionchanged:: 3.7 + *suppress_ragged_eofs* now defaults to :const:`False`. + Context creation ^^^^^^^^^^^^^^^^ @@ -940,7 +943,8 @@ the same limitation) - :meth:`~socket.socket.sendfile()` (but :mod:`os.sendfile` will be used for plain-text sockets only, else :meth:`~socket.socket.send()` will be used) - - :meth:`~socket.socket.shutdown()` + - :meth:`~socket.socket.shutdown()` (bypasses SSL and shuts down the + OS-level socket) However, since the SSL (and TLS) protocol has its own framing atop of TCP, the SSL sockets abstraction can, in certain respects, diverge from @@ -1544,8 +1548,16 @@ `SSL/TLS & Perfect Forward Secrecy `_ Vincent Bernat. +.. attribute:: SSLContext.suppress_ragged_eofs + + This flag has the same meaning as in the top-level :func:`wrap_socket` + function. It affects the :class:`SSLSocket` objects returned by + the context's :meth:`~SSLContext.wrap_socket` method. + + .. versionadded:: 3.7 + .. method:: SSLContext.wrap_socket(sock, server_side=False, \ - do_handshake_on_connect=True, suppress_ragged_eofs=True, \ + do_handshake_on_connect=True, suppress_ragged_eofs=None, \ server_hostname=None, session=None) Wrap an existing Python socket *sock* and return an :class:`SSLSocket` @@ -1553,9 +1565,10 @@ types are unsupported. The returned SSL socket is tied to the context, its settings and - certificates. The parameters *server_side*, *do_handshake_on_connect* - and *suppress_ragged_eofs* have the same meaning as in the top-level - :func:`wrap_socket` function. + certificates. The parameters *server_side* and *do_handshake_on_connect* + have the same meaning as in the top-level :func:`wrap_socket` function. + The parameter *suppress_ragged_eofs* overrides the context's + :attr:`suppress_ragged_eofs` setting. On client connections, the optional parameter *server_hostname* specifies the hostname of the service which we are connecting to. This allows a @@ -1572,6 +1585,9 @@ .. versionchanged:: 3.6 *session* argument was added. + .. versionchanged:: 3.7 + *suppress_ragged_eofs* now defaults to the context's setting. + .. method:: SSLContext.wrap_bio(incoming, outgoing, server_side=False, \ server_hostname=None, session=None) @@ -1954,8 +1970,8 @@ connstream = context.wrap_socket(newsocket, server_side=True) try: deal_with_client(connstream) + connstream = connstream.unwrap() finally: - connstream.shutdown(socket.SHUT_RDWR) connstream.close() Then you'll read data from the ``connstream`` and do something with it till you diff -r e5888f5b9cf8 Doc/whatsnew/3.7.rst --- a/Doc/whatsnew/3.7.rst Wed Sep 21 19:35:54 2016 +0300 +++ b/Doc/whatsnew/3.7.rst Sat Oct 15 23:28:09 2016 +0000 @@ -86,6 +86,16 @@ Improved Modules ================ +SSL +--- + +The *suppress_ragged_eofs* setting is now disabled by default. +This means that by default a TCP shutdown, which may not be secure, +raises :exc:`~ssl.SSLEOFError`, and applications can distinguish a +secure SSL shutdown from a truncation attack. There is a new +:attr:`SSLContext.suppress_ragged_eofs ` +attribute, which can be used to re-enable the setting via a context object. + Optimizations ============= diff -r e5888f5b9cf8 Lib/ssl.py --- a/Lib/ssl.py Wed Sep 21 19:35:54 2016 +0300 +++ b/Lib/ssl.py Sat Oct 15 23:28:09 2016 +0000 @@ -378,7 +378,7 @@ """An SSLContext holds various SSL-related configuration options and data, such as certificates and possibly a private key.""" - __slots__ = ('protocol', '__weakref__') + __slots__ = ('protocol', '__weakref__', 'suppress_ragged_eofs') _windows_cert_stores = ("CA", "ROOT") def __new__(cls, protocol=PROTOCOL_TLS, *args, **kwargs): @@ -389,11 +389,14 @@ def __init__(self, protocol=PROTOCOL_TLS): self.protocol = protocol + self.suppress_ragged_eofs = False def wrap_socket(self, sock, server_side=False, do_handshake_on_connect=True, - suppress_ragged_eofs=True, + suppress_ragged_eofs=None, server_hostname=None, session=None): + if suppress_ragged_eofs is None: + suppress_ragged_eofs = self.suppress_ragged_eofs return SSLSocket(sock=sock, server_side=server_side, do_handshake_on_connect=do_handshake_on_connect, suppress_ragged_eofs=suppress_ragged_eofs, @@ -719,7 +722,7 @@ ssl_version=PROTOCOL_TLS, ca_certs=None, do_handshake_on_connect=True, family=AF_INET, type=SOCK_STREAM, proto=0, fileno=None, - suppress_ragged_eofs=True, npn_protocols=None, ciphers=None, + suppress_ragged_eofs=False, npn_protocols=None, ciphers=None, server_hostname=None, _context=None, _session=None): @@ -1132,7 +1135,7 @@ server_side=False, cert_reqs=CERT_NONE, ssl_version=PROTOCOL_TLS, ca_certs=None, do_handshake_on_connect=True, - suppress_ragged_eofs=True, + suppress_ragged_eofs=False, ciphers=None): return SSLSocket(sock=sock, keyfile=keyfile, certfile=certfile, server_side=server_side, cert_reqs=cert_reqs, diff -r e5888f5b9cf8 Lib/test/ssl_servers.py --- a/Lib/test/ssl_servers.py Wed Sep 21 19:35:54 2016 +0300 +++ b/Lib/test/ssl_servers.py Sat Oct 15 23:28:09 2016 +0000 @@ -42,6 +42,22 @@ raise return sslconn, addr + def finish_request(self, request, client_address): + super().finish_request(request, client_address) + try: + request.unwrap() + except OSError: + # SSL_shutdown() can raise SSL_ERROR_SYSCALL without setting + # errno if the remote end closed the low-level connection (Issue + # 10808) + pass + + def handle_error(self, request, client_address): + [_, exc, _] = sys.exc_info() + # SSLEOFError is triggered when particular tests abort the request + if not isinstance(exc, ssl.SSLEOFError): + super().handle_error(request, client_address) + class RootedHTTPRequestHandler(SimpleHTTPRequestHandler): # need to override translate_path to get a known root, # instead of using os.curdir, since the test could be diff -r e5888f5b9cf8 Lib/test/test_imaplib.py --- a/Lib/test/test_imaplib.py Wed Sep 21 19:35:54 2016 +0300 +++ b/Lib/test/test_imaplib.py Sat Oct 15 23:28:09 2016 +0000 @@ -300,7 +300,11 @@ self.wfile.write(b'* OK') with self.reaped_server(EOFHandler) as server: - self.assertRaises(imaplib.IMAP4.abort, + if isinstance(server, SecureTCPServer): + exception = ssl.SSLEOFError + else: + exception = imaplib.IMAP4.abort + self.assertRaises(exception, self.imap_class, *server.server_address) @reap_threads diff -r e5888f5b9cf8 Lib/test/test_ssl.py --- a/Lib/test/test_ssl.py Wed Sep 21 19:35:54 2016 +0300 +++ b/Lib/test/test_ssl.py Sat Oct 15 23:28:09 2016 +0000 @@ -1768,6 +1768,26 @@ self.assertEqual(buf, b'foo\n') self.ssl_io_loop(sock, incoming, outgoing, sslobj.unwrap) + def test_insecure_shutdown(self): + s = socket.create_connection(self.server_addr) + self.addCleanup(s.close) + s = test_wrap_socket(s) + self.addCleanup(s.close) + s.send(b'shutdown') + self.assertRaises(ssl.SSLEOFError, s.recv, 300) + + def test_context_ragged_eof(self): + s = socket.create_connection(self.server_addr) + self.addCleanup(s.close) + ctx = ssl.SSLContext(ssl.PROTOCOL_TLS) + self.assertFalse(ctx.suppress_ragged_eofs) + ctx.suppress_ragged_eofs = True + self.assertTrue(ctx.suppress_ragged_eofs) + s = ctx.wrap_socket(s) + self.addCleanup(s.close) + s.send(b'shutdown') + self.assertEqual(s.recv(300), b'') + class NetworkedTests(unittest.TestCase): @@ -1925,26 +1945,27 @@ return while self.running: try: - msg = self.read() + try: + msg = self.read() + except ssl.SSLEOFError: + self.running = False + self.close() + continue stripped = msg.strip() if not stripped: # eof, so quit this handler self.running = False - try: - self.sock = self.sslconn.unwrap() - except OSError: - # Many tests shut the TCP connection down - # without an SSL shutdown. This causes - # unwrap() to raise OSError with errno=0! - pass - else: - self.sslconn = None + self.sock = self.sslconn.unwrap() self.close() elif stripped == b'over': if support.verbose and self.server.connectionchatty: sys.stdout.write(" server: client closed connection\n") self.close() return + elif stripped == b'shutdown': + self.sslconn.shutdown(socket.SHUT_RDWR) + self.close() + return elif (self.server.starttls_server and stripped == b'STARTTLS'): if support.verbose and self.server.connectionchatty: @@ -2066,9 +2087,17 @@ class ConnectionHandler (asyncore.dispatcher_with_send): def __init__(self, conn, certfile): + # The "asyncore" module already makes errors like + # ECONNRESET look like secure EOF signals, so there is + # not much benefit in setting suppress_ragged_eofs=False + # (and it would make the server behaviour more + # complicated due to the race between the server sending + # the last unread response and the client shutting the + # connection down) self.socket = test_wrap_socket(conn, server_side=True, certfile=certfile, - do_handshake_on_connect=False) + do_handshake_on_connect=False, + suppress_ragged_eofs=True) asyncore.dispatcher_with_send.__init__(self, self.socket) self._ssl_accepting = True self._do_ssl_handshake() @@ -3041,6 +3070,8 @@ evt.set() remote, peer = server.accept() remote.recv(1) + unwrapped = remote.unwrap() + unwrapped.close() t = threading.Thread(target=serve) t.start() @@ -3049,9 +3080,9 @@ client = context.wrap_socket(socket.socket()) client.connect((host, port)) client_addr = client.getsockname() + client = client.unwrap() client.close() t.join() - remote.close() server.close() # Sanity checks. self.assertIsInstance(remote, ssl.SSLSocket)