Skip to content

Commit 5de40fa

Browse files
sethmlarsonpicnixzhugovk
committed
[3.11] gh-156793: Validate SSLContext.wrap_bio() parameters like wrap_socket() (GH-158503)
(cherry picked from commit 1697ea3) Co-authored-by: Seth Larson <seth@python.org> Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com> Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
1 parent 28f3154 commit 5de40fa

8 files changed

Lines changed: 187 additions & 10 deletions

File tree

‎Doc/library/asyncio-eventloop.rst‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -521,6 +521,10 @@ Opening network connections
521521

522522
Added the *ssl_shutdown_timeout* parameter.
523523

524+
.. versionchanged:: next
525+
Raises a ``ValueError`` if ``ssl.check_hostname`` is ``True``
526+
and ``server_hostname`` is not supplied.
527+
524528
.. seealso::
525529

526530
The :func:`open_connection` function is a high-level alternative

‎Doc/library/asyncio-stream.rst‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -365,6 +365,11 @@ StreamWriter
365365

366366
.. versionadded:: 3.11
367367

368+
.. versionchanged:: next
369+
Raises a ``ValueError`` if ``sslcontext.check_hostname`` is ``True``
370+
and ``server_hostname`` is not supplied.
371+
372+
368373
.. method:: is_closing()
369374

370375
Return ``True`` if the stream is closed or in the process of

‎Doc/library/ssl.rst‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1891,7 +1891,11 @@ to speed up repeated connections from the same clients.
18911891
outgoing BIO.
18921892

18931893
The *server_side*, *server_hostname* and *session* parameters have the
1894-
same meaning as in :meth:`SSLContext.wrap_socket`.
1894+
same meaning as in :meth:`SSLContext.wrap_socket`, and are validated in
1895+
the same way: in particular a :exc:`ValueError` is raised when
1896+
:attr:`~SSLContext.check_hostname` is enabled but no *server_hostname* is
1897+
given, since there would be no name to match the peer's certificate
1898+
against.
18951899

18961900
.. versionchanged:: 3.6
18971901
*session* argument was added.
@@ -1900,6 +1904,13 @@ to speed up repeated connections from the same clients.
19001904
The method returns an instance of :attr:`SSLContext.sslobject_class`
19011905
instead of hard-coded :class:`SSLObject`.
19021906

1907+
.. versionchanged:: next
1908+
The *server_side*, *server_hostname* and *session* parameters are now
1909+
validated as :meth:`SSLContext.wrap_socket` validates them. Previously
1910+
a context with :attr:`~SSLContext.check_hostname` enabled and no
1911+
*server_hostname* was accepted, and verified the certificate chain but
1912+
never the peer's identity.
1913+
19031914
.. attribute:: SSLContext.sslobject_class
19041915

19051916
The return type of :meth:`SSLContext.wrap_bio`, defaults to

‎Lib/ssl.py‎

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -435,6 +435,20 @@ def match_hostname(cert, hostname):
435435
"subjectAltName fields were found")
436436

437437

438+
def _check_sslobject_params(server_side, context=None, server_hostname=None, session=None):
439+
"""Raises a ValueError if SSLObject._create() parameters aren't valid.
440+
"""
441+
if server_side:
442+
if server_hostname:
443+
raise ValueError("server_hostname can only be specified "
444+
"in client mode")
445+
if session is not None:
446+
raise ValueError("session can only be specified in "
447+
"client mode")
448+
if context.check_hostname and not server_hostname:
449+
raise ValueError("check_hostname requires server_hostname")
450+
451+
438452
DefaultVerifyPaths = namedtuple("DefaultVerifyPaths",
439453
"cafile capath openssl_cafile_env openssl_cafile openssl_capath_env "
440454
"openssl_capath")
@@ -866,6 +880,8 @@ def __init__(self, *args, **kwargs):
866880
@classmethod
867881
def _create(cls, incoming, outgoing, server_side=False,
868882
server_hostname=None, session=None, context=None):
883+
_check_sslobject_params(server_side=server_side, context=context,
884+
server_hostname=server_hostname, session=session)
869885
self = cls.__new__(cls)
870886
sslobj = context._wrap_bio(
871887
incoming, outgoing, server_side=server_side,
@@ -1021,15 +1037,8 @@ def _create(cls, sock, server_side=False, do_handshake_on_connect=True,
10211037
context=None, session=None):
10221038
if sock.getsockopt(SOL_SOCKET, SO_TYPE) != SOCK_STREAM:
10231039
raise NotImplementedError("only stream sockets are supported")
1024-
if server_side:
1025-
if server_hostname:
1026-
raise ValueError("server_hostname can only be specified "
1027-
"in client mode")
1028-
if session is not None:
1029-
raise ValueError("session can only be specified in "
1030-
"client mode")
1031-
if context.check_hostname and not server_hostname:
1032-
raise ValueError("check_hostname requires server_hostname")
1040+
_check_sslobject_params(server_side=server_side, context=context,
1041+
server_hostname=server_hostname, session=session)
10331042

10341043
sock_timeout = sock.gettimeout()
10351044
kwargs = dict(

‎Lib/test/test_asyncio/test_sslproto.py‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,50 @@ def test_handshake_timeout_negative(self):
7070
sslproto.SSLProtocol(self.loop, app_proto, sslcontext, waiter,
7171
ssl_handshake_timeout=-10)
7272

73+
def test_check_hostname_accepts_server_hostname(self):
74+
# Supplying a server_hostname succeeds with check_hostname enabled.
75+
sslcontext = test_utils.simple_client_sslcontext(disable_verify=False)
76+
sslcontext.check_hostname = True
77+
app_proto = mock.Mock()
78+
waiter = mock.Mock()
79+
80+
# No ValueError is raised from SSLProtocol with 'server_hostname'.
81+
ssl_proto = sslproto.SSLProtocol(self.loop, app_proto, sslcontext, waiter,
82+
server_hostname='example.org')
83+
self.addCleanup(ssl_proto._app_transport.close)
84+
85+
@support.subTests("server_hostname", [None, ''])
86+
def test_check_hostname_requires_server_hostname(self, server_hostname):
87+
# A caller-supplied context asking for hostname checking used to be
88+
# taken through wrap_bio() with no name to check against, verifying
89+
# the certificate chain but never the peer's identity.
90+
# loop.start_tls() defaults server_hostname to None, and
91+
# loop.create_connection() turns server_hostname='' into None here,
92+
# so both reached that state.
93+
sslcontext = test_utils.simple_client_sslcontext(disable_verify=False)
94+
sslcontext.check_hostname = True
95+
app_proto = mock.Mock()
96+
waiter = mock.Mock()
97+
98+
# Supplying an empty server_hostname fails with check_hostname enabled.
99+
with self.assertRaisesRegex(
100+
ValueError,
101+
'check_hostname requires server_hostname'):
102+
sslproto.SSLProtocol(self.loop, app_proto, sslcontext,
103+
waiter,
104+
server_hostname=server_hostname)
105+
106+
# Disabling check_hostname allows for an empty or unset server_hostname.
107+
sslcontext.check_hostname = False
108+
109+
ssl_proto = sslproto.SSLProtocol(self.loop, app_proto, sslcontext, waiter)
110+
self.addCleanup(ssl_proto._app_transport.close)
111+
112+
ssl_proto = sslproto.SSLProtocol(self.loop, app_proto, sslcontext,
113+
waiter,
114+
server_hostname=server_hostname)
115+
self.addCleanup(ssl_proto._app_transport.close)
116+
73117
def test_eof_received_waiter(self):
74118
waiter = self.loop.create_future()
75119
ssl_proto = self.ssl_protocol(waiter=waiter)

‎Lib/test/test_ssl.py‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
from test.support import socket_helper
1010
from test.support import threading_helper
1111
from test.support import warnings_helper
12+
import contextlib
1213
import array
1314
import re
1415
import socket
@@ -328,6 +329,34 @@ def testing_context(server_cert=SIGNED_CERTFILE, *, server_chain=True):
328329
return client_context, server_context, hostname
329330

330331

332+
def connected_bio_pair(client_context, server_context, hostname, max_retry=5):
333+
"""Handshake a client and a server SSLObject against each other.
334+
335+
Everything happens in memory, so this needs no socket and no thread.
336+
Returns the two objects followed by their four BIOs, in the order
337+
client, server, c_in, c_out, s_in, s_out.
338+
"""
339+
c_in, c_out = ssl.MemoryBIO(), ssl.MemoryBIO()
340+
s_in, s_out = ssl.MemoryBIO(), ssl.MemoryBIO()
341+
client = client_context.wrap_bio(c_in, c_out, server_hostname=hostname)
342+
server = server_context.wrap_bio(s_in, s_out, server_side=True)
343+
344+
# Loop on the handshake for a bit to get it settled
345+
for _ in range(max_retry):
346+
with contextlib.suppress(ssl.SSLWantReadError):
347+
client.do_handshake()
348+
if c_out.pending:
349+
s_in.write(c_out.read())
350+
with contextlib.suppress(ssl.SSLWantReadError):
351+
server.do_handshake()
352+
if s_out.pending:
353+
c_in.write(s_out.read())
354+
# Now the handshakes should be complete (don't raise WantReadError)
355+
client.do_handshake()
356+
server.do_handshake()
357+
return client, server, c_in, c_out, s_in, s_out
358+
359+
331360
class BasicSocketTests(unittest.TestCase):
332361

333362
def test_constants(self):
@@ -1914,6 +1943,10 @@ def test_subclass(self):
19141943

19151944
def test_bad_server_hostname(self):
19161945
ctx = ssl.create_default_context()
1946+
# Omitting the name entirely is bad too: this context checks it.
1947+
with self.assertRaises(ValueError):
1948+
ctx.wrap_bio(ssl.MemoryBIO(), ssl.MemoryBIO(),
1949+
server_hostname=None)
19171950
with self.assertRaises(ValueError):
19181951
ctx.wrap_bio(ssl.MemoryBIO(), ssl.MemoryBIO(),
19191952
server_hostname="")
@@ -1998,6 +2031,64 @@ def test_private_init(self):
19982031
with self.assertRaisesRegex(TypeError, "public constructor"):
19992032
ssl.SSLObject(bio, bio)
20002033

2034+
def test_check_hostname_requires_server_hostname(self):
2035+
# wrap_bio() used to accept a context asking for hostname checking
2036+
# without a name to check against, and then verify the certificate
2037+
# chain but never the peer's identity, with check_hostname still
2038+
# reporting True and nothing reporting the check had been skipped.
2039+
# It must refuse that call, as wrap_socket() already did.
2040+
client_context, _, hostname = testing_context()
2041+
self.assertTrue(client_context.check_hostname)
2042+
2043+
for server_hostname in (None, ""):
2044+
with self.subTest(server_hostname=server_hostname):
2045+
with self.assertRaisesRegex(
2046+
ValueError,
2047+
"check_hostname requires server_hostname"):
2048+
client_context.wrap_bio(ssl.MemoryBIO(), ssl.MemoryBIO(),
2049+
server_hostname=server_hostname)
2050+
# The sibling constructor refuses the very same call.
2051+
with socket.socket() as sock:
2052+
with self.assertRaisesRegex(
2053+
ValueError,
2054+
"check_hostname requires server_hostname"):
2055+
client_context.wrap_socket(
2056+
sock, server_hostname=server_hostname)
2057+
2058+
# A name was all that was missing.
2059+
client_context.wrap_bio(ssl.MemoryBIO(), ssl.MemoryBIO(),
2060+
server_hostname=hostname)
2061+
2062+
# Asking for no hostname check remains a way to say so explicitly.
2063+
context = make_test_context()
2064+
self.assertFalse(context.check_hostname)
2065+
context.wrap_bio(ssl.MemoryBIO(), ssl.MemoryBIO())
2066+
2067+
def test_server_side_bad_params(self):
2068+
# A server neither sends a hostname nor resumes a client's session,
2069+
# so wrap_bio() rejects both in server mode like wrap_socket()
2070+
client_context, server_context, hostname = testing_context()
2071+
2072+
with self.assertRaisesRegex(
2073+
ValueError,
2074+
"server_hostname can only be specified in client mode"):
2075+
server_context.wrap_bio(ssl.MemoryBIO(), ssl.MemoryBIO(),
2076+
server_side=True,
2077+
server_hostname=hostname)
2078+
2079+
client, server, *_ = connected_bio_pair(
2080+
client_context, server_context, hostname)
2081+
session = client.session
2082+
self.assertIsNotNone(session)
2083+
with self.assertRaisesRegex(
2084+
ValueError, "session can only be specified in client mode"):
2085+
server_context.wrap_bio(ssl.MemoryBIO(), ssl.MemoryBIO(),
2086+
server_side=True, session=session)
2087+
2088+
# Neither argument is what a server passes, so this still works.
2089+
server_context.wrap_bio(ssl.MemoryBIO(), ssl.MemoryBIO(),
2090+
server_side=True)
2091+
20012092
def test_unwrap(self):
20022093
client_ctx, server_ctx, hostname = testing_context()
20032094
c_in = ssl.MemoryBIO()
Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
:meth:`ssl.SSLContext.wrap_bio` now validates its *server_side*,
2+
*server_hostname* and *session* arguments similar to
3+
:meth:`ssl.SSLContext.wrap_socket`.
4+
5+
In particular, a context with :attr:`~ssl.SSLContext.check_hostname` enabled
6+
and no *server_hostname* passed to :meth:`!wrap_bio` now raises :exc:`ValueError`
7+
instead of completing a handshake that verified the certificate chain
8+
without verifying the peer's identity, with no indication that the
9+
check had been skipped.
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
:mod:`asyncio`: :meth:`loop.start_tls() <asyncio.loop.start_tls>` and
2+
:meth:`loop.create_connection() <asyncio.loop.create_connection>` now
3+
validate the *server_hostname* argument if an :class:`ssl.SSLContext` is
4+
passed with *check_hostname* set to ``True``.

0 commit comments

Comments
 (0)