From 1ceae5dce6a73faf6665ed1f1d6a3687c10b170a Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Fri, 1 Jun 2018 10:55:56 -0400 Subject: [PATCH 01/10] bpo-33734: asyncio/ssl: Fix AttributeError, increase default handshake timeout --- Doc/library/asyncio-eventloop.rst | 8 ++++---- Lib/asyncio/constants.py | 3 ++- Lib/asyncio/sslproto.py | 20 +++++++++++-------- .../2018-06-01-10-55-48.bpo-33734.x1W9x0.rst | 1 + 4 files changed, 19 insertions(+), 13 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2018-06-01-10-55-48.bpo-33734.x1W9x0.rst diff --git a/Doc/library/asyncio-eventloop.rst b/Doc/library/asyncio-eventloop.rst index 9d7f2362b3d19b..a38dab0a7251ec 100644 --- a/Doc/library/asyncio-eventloop.rst +++ b/Doc/library/asyncio-eventloop.rst @@ -351,7 +351,7 @@ Creating connections * *ssl_handshake_timeout* is (for an SSL connection) the time in seconds to wait for the SSL handshake to complete before aborting the connection. - ``10.0`` seconds if ``None`` (default). + ``60.0`` seconds if ``None`` (default). .. versionadded:: 3.7 @@ -497,7 +497,7 @@ Creating listening connections * *ssl_handshake_timeout* is (for an SSL server) the time in seconds to wait for the SSL handshake to complete before aborting the connection. - ``10.0`` seconds if ``None`` (default). + ``60.0`` seconds if ``None`` (default). * *start_serving* set to ``True`` (the default) causes the created server to start accepting connections immediately. When set to ``False``, @@ -559,7 +559,7 @@ Creating listening connections * *ssl_handshake_timeout* is (for an SSL connection) the time in seconds to wait for the SSL handshake to complete before aborting the connection. - ``10.0`` seconds if ``None`` (default). + ``60.0`` seconds if ``None`` (default). When completed it returns a ``(transport, protocol)`` pair. @@ -628,7 +628,7 @@ TLS Upgrade * *ssl_handshake_timeout* is (for an SSL connection) the time in seconds to wait for the SSL handshake to complete before aborting the connection. - ``10.0`` seconds if ``None`` (default). + ``60.0`` seconds if ``None`` (default). .. versionadded:: 3.7 diff --git a/Lib/asyncio/constants.py b/Lib/asyncio/constants.py index d7ba4969428969..33feed60e55b00 100644 --- a/Lib/asyncio/constants.py +++ b/Lib/asyncio/constants.py @@ -12,7 +12,8 @@ DEBUG_STACK_DEPTH = 10 # Number of seconds to wait for SSL handshake to complete -SSL_HANDSHAKE_TIMEOUT = 10.0 +# The default timeout matches that of Nginx. +SSL_HANDSHAKE_TIMEOUT = 60.0 # Used in sendfile fallback code. We use fallback for platforms # that don't support sendfile, or for TLS connections. diff --git a/Lib/asyncio/sslproto.py b/Lib/asyncio/sslproto.py index a6d382ecd3de63..44e95dc07f1f4e 100644 --- a/Lib/asyncio/sslproto.py +++ b/Lib/asyncio/sslproto.py @@ -214,13 +214,14 @@ def feed_ssldata(self, data, only_handshake=False): # Drain possible plaintext data after close_notify. appdata.append(self._incoming.read()) except (ssl.SSLError, ssl.CertificateError) as exc: - if getattr(exc, 'errno', None) not in ( + exc_errno = getattr(exc, 'errno', None) + if exc_errno not in ( ssl.SSL_ERROR_WANT_READ, ssl.SSL_ERROR_WANT_WRITE, ssl.SSL_ERROR_SYSCALL): if self._state == _DO_HANDSHAKE and self._handshake_cb: self._handshake_cb(exc) raise - self._need_ssldata = (exc.errno == ssl.SSL_ERROR_WANT_READ) + self._need_ssldata = (exc_errno == ssl.SSL_ERROR_WANT_READ) # Check for record level data that needs to be sent back. # Happens for the initial handshake and renegotiations. @@ -263,13 +264,14 @@ def feed_appdata(self, data, offset=0): # It is not allowed to call write() after unwrap() until the # close_notify is acknowledged. We return the condition to the # caller as a short write. + exc_errno = getattr(exc, 'errno', None) if exc.reason == 'PROTOCOL_IS_SHUTDOWN': - exc.errno = ssl.SSL_ERROR_WANT_READ - if exc.errno not in (ssl.SSL_ERROR_WANT_READ, + exc_errno = exc.errno = ssl.SSL_ERROR_WANT_READ + if exc_errno not in (ssl.SSL_ERROR_WANT_READ, ssl.SSL_ERROR_WANT_WRITE, ssl.SSL_ERROR_SYSCALL): raise - self._need_ssldata = (exc.errno == ssl.SSL_ERROR_WANT_READ) + self._need_ssldata = (exc_errno == ssl.SSL_ERROR_WANT_READ) # See if there's any record level data back for us. if self._outgoing.pending: @@ -517,8 +519,8 @@ def data_received(self, data): ssldata, appdata = self._sslpipe.feed_ssldata(data) except ssl.SSLError as e: if self._loop.get_debug(): - logger.warning('%r: SSL error %s (reason %s)', - self, e.errno, e.reason) + logger.warning('%r: SSL error errno: %s (reason %s)', + self, getattr(e, 'errno', None), e.reason) self._abort() return @@ -602,7 +604,9 @@ def _start_handshake(self): def _check_handshake_timeout(self): if self._in_handshake is True: - logger.warning("%r stalled during handshake", self) + logger.warning( + "SSL handshake for %r is taking longer than %r seconds: " + "aborting the connection", self, self._ssl_handshake_timeout) self._abort() def _on_handshake_complete(self, handshake_exc): diff --git a/Misc/NEWS.d/next/Library/2018-06-01-10-55-48.bpo-33734.x1W9x0.rst b/Misc/NEWS.d/next/Library/2018-06-01-10-55-48.bpo-33734.x1W9x0.rst new file mode 100644 index 00000000000000..305d40ed8a1ba9 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2018-06-01-10-55-48.bpo-33734.x1W9x0.rst @@ -0,0 +1 @@ +asyncio/ssl: Fix AttributeError, increase default handshake timeout From 9414957a8886ccf90a493b1632e1d22a0792064c Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Fri, 1 Jun 2018 11:10:34 -0400 Subject: [PATCH 02/10] Make warning text clearer --- Lib/asyncio/sslproto.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Lib/asyncio/sslproto.py b/Lib/asyncio/sslproto.py index 44e95dc07f1f4e..87f584f598cd6b 100644 --- a/Lib/asyncio/sslproto.py +++ b/Lib/asyncio/sslproto.py @@ -519,8 +519,9 @@ def data_received(self, data): ssldata, appdata = self._sslpipe.feed_ssldata(data) except ssl.SSLError as e: if self._loop.get_debug(): - logger.warning('%r: SSL error errno: %s (reason %s)', - self, getattr(e, 'errno', None), e.reason) + logger.warning('%r: SSL error errno:%s (reason %s)', + self, getattr(e, 'errno', 'missing'), + e.reason) self._abort() return From bd27898f89580efc74d137d90cd88ccc1272462c Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Fri, 1 Jun 2018 11:27:07 -0400 Subject: [PATCH 03/10] Fix docstrings --- Lib/asyncio/events.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Lib/asyncio/events.py b/Lib/asyncio/events.py index 40946bbf65299d..e4e632206af1bc 100644 --- a/Lib/asyncio/events.py +++ b/Lib/asyncio/events.py @@ -352,8 +352,7 @@ async def create_server( ssl_handshake_timeout is the time in seconds that an SSL server will wait for completion of the SSL handshake before aborting the - connection. Default is 10s, longer timeouts may increase vulnerability - to DoS attacks (see https://support.f5.com/csp/article/K13834) + connection. Default is 60s. start_serving set to True (default) causes the created server to start accepting connections immediately. When set to False, @@ -411,7 +410,7 @@ async def create_unix_server( accepted connections. ssl_handshake_timeout is the time in seconds that an SSL server - will wait for the SSL handshake to complete (defaults to 10s). + will wait for the SSL handshake to complete (defaults to 60s). start_serving set to True (default) causes the created server to start accepting connections immediately. When set to False, From d6dfc80bcf4f6dc71e4e5481a888c71e785b6738 Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Fri, 1 Jun 2018 14:47:33 -0400 Subject: [PATCH 04/10] Fix SSLProtocol to correctly propagate errors and abort connections Add functional tests --- Lib/asyncio/sslproto.py | 48 +++++---- Lib/test/test_asyncio/test_sslproto.py | 139 +++++++++++++++++++------ Lib/test/test_asyncio/utils.py | 5 +- 3 files changed, 139 insertions(+), 53 deletions(-) diff --git a/Lib/asyncio/sslproto.py b/Lib/asyncio/sslproto.py index 87f584f598cd6b..0742f5397d40ad 100644 --- a/Lib/asyncio/sslproto.py +++ b/Lib/asyncio/sslproto.py @@ -7,6 +7,7 @@ from . import base_events from . import constants +from . import futures from . import protocols from . import transports from .log import logger @@ -490,6 +491,12 @@ def connection_lost(self, exc): if self._session_established: self._session_established = False self._loop.call_soon(self._app_protocol.connection_lost, exc) + else: + # Most likely an exception occurred while in SSL handshake. + # Just mark the app transport as closed so that its __del__ + # doesn't complain. + if self._app_transport is not None: + self._app_transport._closed = True self._transport = None self._app_transport = None self._wakeup_waiter(exc) @@ -605,10 +612,12 @@ def _start_handshake(self): def _check_handshake_timeout(self): if self._in_handshake is True: - logger.warning( - "SSL handshake for %r is taking longer than %r seconds: " - "aborting the connection", self, self._ssl_handshake_timeout) - self._abort() + msg = ( + f"SSL handshake for {self} is taking longer than " + f"{self._ssl_handshake_timeout} seconds: " + f"aborting the connection" + ) + self._fatal_error(ConnectionAbortedError(msg)) def _on_handshake_complete(self, handshake_exc): self._in_handshake = False @@ -620,21 +629,16 @@ def _on_handshake_complete(self, handshake_exc): raise handshake_exc peercert = sslobj.getpeercert() - except BaseException as exc: - if self._loop.get_debug(): - if isinstance(exc, ssl.CertificateError): - logger.warning("%r: SSL handshake failed " - "on verifying the certificate", - self, exc_info=True) - else: - logger.warning("%r: SSL handshake failed", - self, exc_info=True) - self._transport.close() - if isinstance(exc, Exception): - self._wakeup_waiter(exc) - return + except Exception as exc: + if isinstance(exc, ssl.CertificateError): + msg = ( + f'{self}: SSL handshake failed on verifying ' + f'the certificate' + ) else: - raise + msg = f'{self}: SSL handshake failed' + self._fatal_error(exc, msg) + return if self._loop.get_debug(): dt = self._loop.time() - self._handshake_start_time @@ -702,19 +706,19 @@ def _process_write_backlog(self): raise def _fatal_error(self, exc, message='Fatal error on transport'): - # Should be called from exception handler only. + if self._transport: + self._transport._force_close(exc) + if isinstance(exc, base_events._FATAL_ERROR_IGNORE): if self._loop.get_debug(): logger.debug("%r: %s", self, message, exc_info=True) - else: + elif not isinstance(exc, futures.CancelledError): self._loop.call_exception_handler({ 'message': message, 'exception': exc, 'transport': self._transport, 'protocol': self, }) - if self._transport: - self._transport._force_close(exc) def _finalize(self): self._sslpipe = None diff --git a/Lib/test/test_asyncio/test_sslproto.py b/Lib/test/test_asyncio/test_sslproto.py index fa9cbd56ed424b..7a7fa109f063df 100644 --- a/Lib/test/test_asyncio/test_sslproto.py +++ b/Lib/test/test_asyncio/test_sslproto.py @@ -49,35 +49,6 @@ def mock_handshake(callback): ssl_proto.connection_made(transport) return transport - def test_cancel_handshake(self): - # Python issue #23197: cancelling a handshake must not raise an - # exception or log an error, even if the handshake failed - waiter = asyncio.Future(loop=self.loop) - ssl_proto = self.ssl_protocol(waiter=waiter) - handshake_fut = asyncio.Future(loop=self.loop) - - def do_handshake(callback): - exc = Exception() - callback(exc) - handshake_fut.set_result(None) - return [] - - waiter.cancel() - self.connection_made(ssl_proto, do_handshake=do_handshake) - - with test_utils.disable_logger(): - self.loop.run_until_complete(handshake_fut) - - def test_handshake_timeout(self): - # bpo-29970: Check that a connection is aborted if handshake is not - # completed in timeout period, instead of remaining open indefinitely - ssl_proto = self.ssl_protocol() - transport = self.connection_made(ssl_proto) - - with test_utils.disable_logger(): - self.loop.run_until_complete(tasks.sleep(0.2, loop=self.loop)) - self.assertTrue(transport.abort.called) - def test_handshake_timeout_zero(self): sslcontext = test_utils.dummy_ssl_context() app_proto = mock.Mock() @@ -477,6 +448,116 @@ async def main(): self.loop.run_until_complete(main()) + def test_handshake_timeout(self): + # bpo-29970: Check that a connection is aborted if handshake is not + # completed in timeout period, instead of remaining open indefinitely + client_sslctx = test_utils.simple_client_sslcontext() + + # silence error logger + messages = [] + self.loop.set_exception_handler(lambda loop, ctx: messages.append(ctx)) + + server_side_aborted = False + + def server(sock): + nonlocal server_side_aborted + try: + sock.recv_all(1024 * 1024) + except ConnectionAbortedError: + server_side_aborted = True + finally: + sock.close() + + async def client(addr): + await asyncio.wait_for( + self.loop.create_connection( + asyncio.Protocol, + *addr, + ssl=client_sslctx, + server_hostname='', + ssl_handshake_timeout=10.0), + 0.5, + loop=self.loop) + + with self.tcp_server(server, + max_clients=1, + backlog=1) as srv: + + with self.assertRaises(asyncio.TimeoutError): + self.loop.run_until_complete(client(srv.addr)) + + self.assertTrue(server_side_aborted) + + # Python issue #23197: cancelling a handshake must not raise an + # exception or log an error, even if the handshake failed + self.assertEqual(messages, []) + + def test_create_connection_ssl_slow_handshake(self): + client_sslctx = test_utils.simple_client_sslcontext() + + # silence error logger + self.loop.set_exception_handler(lambda *args: None) + + def server(sock): + try: + sock.recv_all(1024 * 1024) + except ConnectionAbortedError: + pass + finally: + sock.close() + + async def client(addr): + reader, writer = await asyncio.open_connection( + *addr, + ssl=client_sslctx, + server_hostname='', + loop=self.loop, + ssl_handshake_timeout=1.0) + + with self.tcp_server(server, + max_clients=1, + backlog=1) as srv: + + with self.assertRaisesRegex( + ConnectionAbortedError, + r'SSL handshake.*is taking longer'): + + self.loop.run_until_complete(client(srv.addr)) + + def test_create_connection_ssl_failed_certificate(self): + # silence error logger + self.loop.set_exception_handler(lambda *args: None) + + sslctx = test_utils.simple_server_sslcontext() + client_sslctx = test_utils.simple_client_sslcontext( + disable_verify=False) + + def server(sock): + try: + sock.start_tls( + sslctx, + server_side=True) + sock.connect() + except ssl.SSLError: + pass + finally: + sock.close() + + async def client(addr): + reader, writer = await asyncio.open_connection( + *addr, + ssl=client_sslctx, + server_hostname='', + loop=self.loop, + ssl_handshake_timeout=1.0) + + with self.tcp_server(server, + max_clients=1, + backlog=1) as srv: + + with self.assertRaises(ssl.SSLCertVerificationError): + self.loop.run_until_complete(client(srv.addr)) + @unittest.skipIf(ssl is None, 'No ssl module') class SelectorStartTLSTests(BaseStartTLS, unittest.TestCase): diff --git a/Lib/test/test_asyncio/utils.py b/Lib/test/test_asyncio/utils.py index 96dfe2f85b4de1..5362591b5d7380 100644 --- a/Lib/test/test_asyncio/utils.py +++ b/Lib/test/test_asyncio/utils.py @@ -77,10 +77,11 @@ def simple_server_sslcontext(): return server_context -def simple_client_sslcontext(): +def simple_client_sslcontext(*, disable_verify=True): client_context = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT) client_context.check_hostname = False - client_context.verify_mode = ssl.CERT_NONE + if disable_verify: + client_context.verify_mode = ssl.CERT_NONE return client_context From 0220c96ae55ebbe125931888d35969e095e48d16 Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Fri, 1 Jun 2018 17:16:45 -0400 Subject: [PATCH 05/10] start_tls should close the transport if waiter is cancelled --- Lib/asyncio/base_events.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Lib/asyncio/base_events.py b/Lib/asyncio/base_events.py index 61938e90c375df..34cc6252e77cb6 100644 --- a/Lib/asyncio/base_events.py +++ b/Lib/asyncio/base_events.py @@ -1114,7 +1114,12 @@ async def start_tls(self, transport, protocol, sslcontext, *, self.call_soon(ssl_protocol.connection_made, transport) self.call_soon(transport.resume_reading) - await waiter + try: + await waiter + except Exception: + transport.close() + raise + return ssl_protocol._app_transport async def create_datagram_endpoint(self, protocol_factory, From 24e5520d202637d2f7ba4f5282d468d3494e9e23 Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Fri, 1 Jun 2018 17:23:57 -0400 Subject: [PATCH 06/10] Add a test for proper waiter handling in start_tls --- Lib/test/test_asyncio/test_sslproto.py | 61 ++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/Lib/test/test_asyncio/test_sslproto.py b/Lib/test/test_asyncio/test_sslproto.py index 7a7fa109f063df..24354de8d63017 100644 --- a/Lib/test/test_asyncio/test_sslproto.py +++ b/Lib/test/test_asyncio/test_sslproto.py @@ -359,6 +359,67 @@ async def client(addr): asyncio.wait_for(client(srv.addr), loop=self.loop, timeout=self.TIMEOUT)) + def test_start_tls_slow_client_cancel(self): + HELLO_MSG = b'1' * self.PAYLOAD_SIZE + + client_context = test_utils.simple_client_sslcontext() + server_waits_on_handshake = self.loop.create_future() + + def serve(sock): + sock.settimeout(self.TIMEOUT) + + data = sock.recv_all(len(HELLO_MSG)) + self.assertEqual(len(data), len(HELLO_MSG)) + + try: + self.loop.call_soon_threadsafe( + server_waits_on_handshake.set_result, None) + data = sock.recv_all(1024 * 1024) + except ConnectionAbortedError: + pass + finally: + sock.close() + + class ClientProto(asyncio.Protocol): + def __init__(self, on_data, on_eof): + self.on_data = on_data + self.on_eof = on_eof + self.con_made_cnt = 0 + + def connection_made(proto, tr): + proto.con_made_cnt += 1 + # Ensure connection_made gets called only once. + self.assertEqual(proto.con_made_cnt, 1) + + def data_received(self, data): + self.on_data.set_result(data) + + def eof_received(self): + self.on_eof.set_result(True) + + async def client(addr): + await asyncio.sleep(0.5, loop=self.loop) + + on_data = self.loop.create_future() + on_eof = self.loop.create_future() + + tr, proto = await self.loop.create_connection( + lambda: ClientProto(on_data, on_eof), *addr) + + tr.write(HELLO_MSG) + + await server_waits_on_handshake + + with self.assertRaises(asyncio.TimeoutError): + await asyncio.wait_for( + self.loop.start_tls(tr, proto, client_context), + 0.5, + loop=self.loop) + + with self.tcp_server(serve, timeout=self.TIMEOUT) as srv: + self.loop.run_until_complete( + asyncio.wait_for(client(srv.addr), loop=self.loop, timeout=10)) + def test_start_tls_server_1(self): HELLO_MSG = b'1' * self.PAYLOAD_SIZE From 4e4c386ef54626b92d23df4d6f1b6256bd2638fa Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Fri, 1 Jun 2018 20:47:19 -0400 Subject: [PATCH 07/10] Do not log exceptions (they are propagated anyways) --- Lib/asyncio/sslproto.py | 13 +++---------- Lib/test/test_asyncio/test_sslproto.py | 13 ++++++++----- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/Lib/asyncio/sslproto.py b/Lib/asyncio/sslproto.py index 0742f5397d40ad..6410f33c5c1dfd 100644 --- a/Lib/asyncio/sslproto.py +++ b/Lib/asyncio/sslproto.py @@ -709,16 +709,9 @@ def _fatal_error(self, exc, message='Fatal error on transport'): if self._transport: self._transport._force_close(exc) - if isinstance(exc, base_events._FATAL_ERROR_IGNORE): - if self._loop.get_debug(): - logger.debug("%r: %s", self, message, exc_info=True) - elif not isinstance(exc, futures.CancelledError): - self._loop.call_exception_handler({ - 'message': message, - 'exception': exc, - 'transport': self._transport, - 'protocol': self, - }) + if (self._loop.get_debug() and + isinstance(exc, base_events._FATAL_ERROR_IGNORE)): + logger.debug("%r: %s", self, message, exc_info=True) def _finalize(self): self._sslpipe = None diff --git a/Lib/test/test_asyncio/test_sslproto.py b/Lib/test/test_asyncio/test_sslproto.py index 24354de8d63017..a35d8ae2f26a89 100644 --- a/Lib/test/test_asyncio/test_sslproto.py +++ b/Lib/test/test_asyncio/test_sslproto.py @@ -514,7 +514,6 @@ def test_handshake_timeout(self): # completed in timeout period, instead of remaining open indefinitely client_sslctx = test_utils.simple_client_sslcontext() - # silence error logger messages = [] self.loop.set_exception_handler(lambda loop, ctx: messages.append(ctx)) @@ -556,8 +555,8 @@ async def client(addr): def test_create_connection_ssl_slow_handshake(self): client_sslctx = test_utils.simple_client_sslcontext() - # silence error logger - self.loop.set_exception_handler(lambda *args: None) + messages = [] + self.loop.set_exception_handler(lambda loop, ctx: messages.append(ctx)) def server(sock): try: @@ -585,9 +584,11 @@ async def client(addr): self.loop.run_until_complete(client(srv.addr)) + self.assertEqual(messages, []) + def test_create_connection_ssl_failed_certificate(self): - # silence error logger - self.loop.set_exception_handler(lambda *args: None) + messages = [] + self.loop.set_exception_handler(lambda loop, ctx: messages.append(ctx)) sslctx = test_utils.simple_server_sslcontext() client_sslctx = test_utils.simple_client_sslcontext( @@ -619,6 +620,8 @@ async def client(addr): with self.assertRaises(ssl.SSLCertVerificationError): self.loop.run_until_complete(client(srv.addr)) + self.assertEqual(messages, []) + @unittest.skipIf(ssl is None, 'No ssl module') class SelectorStartTLSTests(BaseStartTLS, unittest.TestCase): From 9e412f0a077eff7c759f2801930ec20334f885db Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Fri, 1 Jun 2018 21:01:26 -0400 Subject: [PATCH 08/10] Fix SSLProtocol.data_received() to propagate errors correctly --- Lib/asyncio/sslproto.py | 8 ++--- Lib/test/test_asyncio/test_sslproto.py | 44 +++++++++++++++++++++++++- 2 files changed, 45 insertions(+), 7 deletions(-) diff --git a/Lib/asyncio/sslproto.py b/Lib/asyncio/sslproto.py index 6410f33c5c1dfd..50235a76e42723 100644 --- a/Lib/asyncio/sslproto.py +++ b/Lib/asyncio/sslproto.py @@ -524,12 +524,8 @@ def data_received(self, data): try: ssldata, appdata = self._sslpipe.feed_ssldata(data) - except ssl.SSLError as e: - if self._loop.get_debug(): - logger.warning('%r: SSL error errno:%s (reason %s)', - self, getattr(e, 'errno', 'missing'), - e.reason) - self._abort() + except Exception as e: + self._fatal_error(e, 'SSL error in data received') return for chunk in ssldata: diff --git a/Lib/test/test_asyncio/test_sslproto.py b/Lib/test/test_asyncio/test_sslproto.py index a35d8ae2f26a89..1c21f1fe3506d5 100644 --- a/Lib/test/test_asyncio/test_sslproto.py +++ b/Lib/test/test_asyncio/test_sslproto.py @@ -599,7 +599,6 @@ def server(sock): sock.start_tls( sslctx, server_side=True) - sock.connect() except ssl.SSLError: pass finally: @@ -622,6 +621,49 @@ async def client(addr): self.assertEqual(messages, []) + def test_start_tls_client_corrupted_ssl(self): + messages = [] + self.loop.set_exception_handler(lambda loop, ctx: messages.append(ctx)) + + sslctx = test_utils.simple_server_sslcontext() + client_sslctx = test_utils.simple_client_sslcontext() + + def server(sock): + orig_sock = sock.dup() + try: + sock.start_tls( + sslctx, + server_side=True) + sock.sendall(b'A\n') + sock.recv_all(1) + orig_sock.send(b'please corrupt the SSL connection') + except ssl.SSLError: + pass + finally: + sock.close() + + async def client(addr): + reader, writer = await asyncio.open_connection( + *addr, + ssl=client_sslctx, + server_hostname='', + loop=self.loop) + + self.assertEqual(await reader.readline(), b'A\n') + writer.write(b'B') + with self.assertRaises(ssl.SSLError): + await reader.readline() + return 'OK' + + with self.tcp_server(server, + max_clients=1, + backlog=1) as srv: + + res = self.loop.run_until_complete(client(srv.addr)) + + self.assertEqual(res, 'OK') + self.assertEqual(messages, []) + @unittest.skipIf(ssl is None, 'No ssl module') class SelectorStartTLSTests(BaseStartTLS, unittest.TestCase): From edd3a0bc61d69c07d36f0814e8ac1dcc78ed7c73 Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Sat, 2 Jun 2018 22:45:58 -0400 Subject: [PATCH 09/10] adjust error messages; restore old _fatal_error implementation --- Lib/asyncio/sslproto.py | 24 +++++++++++++----------- Lib/test/test_asyncio/test_sslproto.py | 9 ++------- 2 files changed, 15 insertions(+), 18 deletions(-) diff --git a/Lib/asyncio/sslproto.py b/Lib/asyncio/sslproto.py index 50235a76e42723..71d9fca9c0797d 100644 --- a/Lib/asyncio/sslproto.py +++ b/Lib/asyncio/sslproto.py @@ -7,7 +7,6 @@ from . import base_events from . import constants -from . import futures from . import protocols from . import transports from .log import logger @@ -609,7 +608,7 @@ def _start_handshake(self): def _check_handshake_timeout(self): if self._in_handshake is True: msg = ( - f"SSL handshake for {self} is taking longer than " + f"SSL handshake is taking longer than " f"{self._ssl_handshake_timeout} seconds: " f"aborting the connection" ) @@ -627,12 +626,9 @@ def _on_handshake_complete(self, handshake_exc): peercert = sslobj.getpeercert() except Exception as exc: if isinstance(exc, ssl.CertificateError): - msg = ( - f'{self}: SSL handshake failed on verifying ' - f'the certificate' - ) + msg = 'SSL handshake failed on verifying the certificate' else: - msg = f'{self}: SSL handshake failed' + msg = 'SSL handshake failed' self._fatal_error(exc, msg) return @@ -702,13 +698,19 @@ def _process_write_backlog(self): raise def _fatal_error(self, exc, message='Fatal error on transport'): + if isinstance(exc, base_events._FATAL_ERROR_IGNORE): + if self._loop.get_debug(): + logger.debug("%r: %s", self, message, exc_info=True) + else: + self._loop.call_exception_handler({ + 'message': message, + 'exception': exc, + 'transport': self._transport, + 'protocol': self, + }) if self._transport: self._transport._force_close(exc) - if (self._loop.get_debug() and - isinstance(exc, base_events._FATAL_ERROR_IGNORE)): - logger.debug("%r: %s", self, message, exc_info=True) - def _finalize(self): self._sslpipe = None diff --git a/Lib/test/test_asyncio/test_sslproto.py b/Lib/test/test_asyncio/test_sslproto.py index 1c21f1fe3506d5..2357130b5f9702 100644 --- a/Lib/test/test_asyncio/test_sslproto.py +++ b/Lib/test/test_asyncio/test_sslproto.py @@ -587,8 +587,7 @@ async def client(addr): self.assertEqual(messages, []) def test_create_connection_ssl_failed_certificate(self): - messages = [] - self.loop.set_exception_handler(lambda loop, ctx: messages.append(ctx)) + self.loop.set_exception_handler(lambda loop, ctx: None) sslctx = test_utils.simple_server_sslcontext() client_sslctx = test_utils.simple_client_sslcontext( @@ -619,11 +618,8 @@ async def client(addr): with self.assertRaises(ssl.SSLCertVerificationError): self.loop.run_until_complete(client(srv.addr)) - self.assertEqual(messages, []) - def test_start_tls_client_corrupted_ssl(self): - messages = [] - self.loop.set_exception_handler(lambda loop, ctx: messages.append(ctx)) + self.loop.set_exception_handler(lambda loop, ctx: None) sslctx = test_utils.simple_server_sslcontext() client_sslctx = test_utils.simple_client_sslcontext() @@ -662,7 +658,6 @@ async def client(addr): res = self.loop.run_until_complete(client(srv.addr)) self.assertEqual(res, 'OK') - self.assertEqual(messages, []) @unittest.skipIf(ssl is None, 'No ssl module') From c9ffdbae0514bb36fa8e602065afb6bb5791346c Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Sat, 2 Jun 2018 22:52:39 -0400 Subject: [PATCH 10/10] Don't catch BaseExceptions: asyncio doesn't handle them correctly anyways --- Lib/asyncio/sslproto.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/Lib/asyncio/sslproto.py b/Lib/asyncio/sslproto.py index 71d9fca9c0797d..8515ec5eebd32e 100644 --- a/Lib/asyncio/sslproto.py +++ b/Lib/asyncio/sslproto.py @@ -687,15 +687,12 @@ def _process_write_backlog(self): # delete it and reduce the outstanding buffer size. del self._write_backlog[0] self._write_buffer_size -= len(data) - except BaseException as exc: + except Exception as exc: if self._in_handshake: - # BaseExceptions will be re-raised in _on_handshake_complete. + # Exceptions will be re-raised in _on_handshake_complete. self._on_handshake_complete(exc) else: self._fatal_error(exc, 'Fatal error on SSL transport') - if not isinstance(exc, Exception): - # BaseException - raise def _fatal_error(self, exc, message='Fatal error on transport'): if isinstance(exc, base_events._FATAL_ERROR_IGNORE):