bpo-33734: asyncio/ssl: a bunch of bugfixes (GH-7321) (GH-7396)

* Fix AttributeError (not all SSL exceptions have 'errno' attribute)

* Increase default handshake timeout from 10 to 60 seconds
* Make sure start_tls can be cancelled correctly
* Make sure any error in SSLProtocol gets propagated (instead of just being logged)
(cherry picked from commit 9602643)

Co-authored-by: Yury Selivanov <[email protected]>

Author: miss-islington

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
 

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,

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.

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,

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:
@@ -488,6 +490,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)
@@ -515,11 +523,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 %s (reason %s)',
-                               self, e.errno, e.reason)
-            self._abort()
+        except Exception as e:
+            self._fatal_error(e, 'SSL error in data received')
             return
 
         for chunk in ssldata:
@@ -602,8 +607,12 @@ def _start_handshake(self):
 
     def _check_handshake_timeout(self):
         if self._in_handshake is True:
-            logger.warning("%r stalled during handshake", self)
-            self._abort()
+            msg = (
+                f"SSL handshake 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
@@ -615,21 +624,13 @@ 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 = 'SSL handshake failed on verifying the certificate'
             else:
-                raise
+                msg = 'SSL handshake failed'
+            self._fatal_error(exc, msg)
+            return
 
         if self._loop.get_debug():
             dt = self._loop.time() - self._handshake_start_time
@@ -686,18 +687,14 @@ 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'):
-        # Should be called from exception handler only.
         if isinstance(exc, base_events._FATAL_ERROR_IGNORE):
             if self._loop.get_debug():
                 logger.debug("%r: %s", self, message, exc_info=True)