Skip to content

Commit a349a5d

Browse files
fix shutdown as well
1 parent 9dd660f commit a349a5d

3 files changed

Lines changed: 86 additions & 5 deletions

File tree

Lib/asyncio/sslproto.py

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -658,6 +658,10 @@ def _do_shutdown(self):
658658
except SSLAgainErrors:
659659
self._process_outgoing()
660660
except ssl.SSLError as exc:
661+
# gh-98078: send what OpenSSL left in the outgoing BIO, e.g.
662+
# the close_notify alert, to the peer before closing (see
663+
# _on_handshake_complete()).
664+
self._process_outgoing()
661665
self._on_shutdown_complete(exc)
662666
else:
663667
self._process_outgoing()
@@ -749,6 +753,11 @@ def _do_read(self):
749753
else:
750754
self._process_outgoing()
751755
self._control_ssl_reading()
756+
except ssl.SSLError as ex:
757+
# gh-98078: send the fatal TLS alert left in the outgoing
758+
# BIO to the peer (see _on_handshake_complete()).
759+
self._process_outgoing()
760+
self._fatal_error(ex, 'Fatal error on SSL protocol')
752761
except Exception as ex:
753762
self._fatal_error(ex, 'Fatal error on SSL protocol')
754763

Lib/test/test_asyncio/test_sslproto.py

Lines changed: 72 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -843,8 +843,10 @@ def test_start_tls_client_corrupted_ssl(self):
843843

844844
sslctx = test_utils.simple_server_sslcontext()
845845
client_sslctx = test_utils.simple_client_sslcontext()
846+
server_err = None
846847

847848
def server(sock):
849+
nonlocal server_err
848850
orig_sock = sock.dup()
849851
try:
850852
sock.start_tls(
@@ -853,8 +855,11 @@ def server(sock):
853855
sock.sendall(b'A\n')
854856
sock.recv_all(1)
855857
orig_sock.send(b'please corrupt the SSL connection')
856-
except ssl.SSLError:
857-
pass
858+
# gh-98078: receive the fatal TLS alert sent by the
859+
# client before it closed the connection
860+
sock.recv(16)
861+
except ssl.SSLError as exc:
862+
server_err = exc
858863
finally:
859864
orig_sock.close()
860865
sock.close()
@@ -880,6 +885,71 @@ async def client(addr):
880885
res = self.loop.run_until_complete(client(srv.addr))
881886

882887
self.assertEqual(res, 'OK')
888+
# gh-98078: the client must send a fatal TLS alert to the
889+
# server instead of just closing the connection, so that the
890+
# server knows why the connection was dropped.
891+
self.assertIsInstance(server_err, ssl.SSLError)
892+
self.assertIn('ALERT_UNEXPECTED_MESSAGE', server_err.reason or '')
893+
894+
def test_shutdown_corrupted_ssl_sends_close_notify(self):
895+
# gh-98078: when the TLS shutdown fails (here: on a corrupted
896+
# record that was buffered while the application had reading
897+
# paused), the close_notify alert that OpenSSL already
898+
# generated must be sent to the peer before the transport is
899+
# closed, so that the peer sees a clean TLS EOF instead of a
900+
# connection reset.
901+
self.loop.set_exception_handler(lambda loop, ctx: None)
902+
903+
sslctx = test_utils.simple_server_sslcontext()
904+
client_sslctx = test_utils.simple_client_sslcontext()
905+
server_err = None
906+
907+
def server(sock):
908+
nonlocal server_err
909+
orig_sock = sock.dup()
910+
try:
911+
sock.start_tls(
912+
sslctx,
913+
server_side=True)
914+
sock.sendall(b'A\n')
915+
sock.recv_all(1)
916+
orig_sock.send(b'please corrupt the SSL connection')
917+
# the client now closes the connection; although its
918+
# TLS shutdown fails on the corrupted record, it must
919+
# still send close_notify, completing our unwrap()
920+
sock.unwrap()
921+
except ssl.SSLError as exc:
922+
server_err = exc
923+
finally:
924+
orig_sock.close()
925+
sock.close()
926+
927+
async def client(addr):
928+
reader, writer = await asyncio.open_connection(
929+
*addr,
930+
ssl=client_sslctx,
931+
server_hostname='')
932+
# drain the post-handshake data (e.g. TLS session tickets)
933+
# so that only the corrupted record can be buffered next
934+
self.assertEqual(await reader.readline(), b'A\n')
935+
# keep the corrupted record buffered in the incoming BIO
936+
writer.transport.pause_reading()
937+
writer.write(b'B')
938+
await writer.drain()
939+
# wait for the corrupted record to arrive in the read buffer
940+
async with asyncio.timeout(support.SHORT_TIMEOUT):
941+
while not writer.transport.get_read_buffer_size():
942+
await asyncio.sleep(0)
943+
writer.close()
944+
with self.assertRaises(ssl.SSLError):
945+
await writer.wait_closed()
946+
947+
with self.tcp_server(server,
948+
max_clients=1,
949+
backlog=1) as srv:
950+
self.loop.run_until_complete(client(srv.addr))
951+
952+
self.assertIsNone(server_err)
883953

884954

885955
@unittest.skipIf(ssl is None, 'No ssl module')
Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
Fix :mod:`asyncio` SSL transports not sending the fatal TLS alert to the
2-
peer when the TLS handshake fails. The peer can now tell why the handshake
3-
failed (for example, certificate verification failure or no TLS version in
4-
common) instead of seeing the connection abruptly closed.
2+
peer when the TLS handshake fails or when receiving corrupted data, and
3+
not sending the ``close_notify`` alert to the peer when the TLS shutdown
4+
fails. The peer can now tell why the connection was dropped (for example,
5+
certificate verification failure or no TLS version in common) instead of
6+
seeing the connection abruptly closed.

0 commit comments

Comments
 (0)