Skip to content

Commit 869069d

Browse files
hugovksethmlarsonpicnixzYhg1s
authored
[3.12] gh-156793: Validate SSLContext.wrap_bio() parameters like wrap_socket() (GH-158503) (#158513)
* [3.12] gh-156793: Validate SSLContext.wrap_bio() parameters like wrap_socket() (GH-158503) (cherry picked from commit 1697ea3) * Raise a DeprecationWarning instead of ValueError 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> Co-authored-by: T. Wouters <thomas@python.org>
1 parent 09a2e7e commit 869069d

8 files changed

Lines changed: 193 additions & 1 deletion

File tree

‎Doc/library/asyncio-eventloop.rst‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -544,6 +544,11 @@ Opening network connections
544544
.. versionchanged:: 3.12
545545
*all_errors* was added.
546546

547+
.. versionchanged:: next
548+
Raises a ``DeprecationWarning`` if ``ssl.check_hostname`` is ``True``
549+
and ``server_hostname`` is not supplied. In Python 3.13 and
550+
later a ``ValueError`` is raised instead.
551+
547552
.. seealso::
548553

549554
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
@@ -382,6 +382,11 @@ StreamWriter
382382
.. versionchanged:: 3.12
383383
Added the *ssl_shutdown_timeout* parameter.
384384

385+
.. versionchanged:: next
386+
Raises a ``DeprecationWarning`` if ``ssl.check_hostname`` is ``True``
387+
and ``server_hostname`` is not supplied. In Python 3.13 and
388+
later a ``ValueError`` is raised instead.
389+
385390

386391
.. method:: is_closing()
387392

‎Doc/library/ssl.rst‎

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

18221822
The *server_side*, *server_hostname* and *session* parameters have the
1823-
same meaning as in :meth:`SSLContext.wrap_socket`.
1823+
same meaning as in :meth:`SSLContext.wrap_socket`, and are validated in
1824+
the same way: in particular a :exc:`DeprecationWarning` is raised when
1825+
:attr:`~SSLContext.check_hostname` is enabled but no *server_hostname* is
1826+
given, since there would be no name to match the peer's certificate
1827+
against. In Python 3.13 and later a ``ValueError`` is raised instead.
18241828

18251829
.. versionchanged:: 3.6
18261830
*session* argument was added.
@@ -1829,6 +1833,13 @@ to speed up repeated connections from the same clients.
18291833
The method returns an instance of :attr:`SSLContext.sslobject_class`
18301834
instead of hard-coded :class:`SSLObject`.
18311835

1836+
.. versionchanged:: next
1837+
The *server_side*, *server_hostname* and *session* parameters are now
1838+
validated as :meth:`SSLContext.wrap_socket` validates them. Previously
1839+
a context with :attr:`~SSLContext.check_hostname` enabled and no
1840+
*server_hostname* was accepted, and verified the certificate chain but
1841+
never the peer's identity.
1842+
18321843
.. attribute:: SSLContext.sslobject_class
18331844

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

‎Lib/ssl.py‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -803,6 +803,19 @@ def __init__(self, *args, **kwargs):
803803
@classmethod
804804
def _create(cls, incoming, outgoing, server_side=False,
805805
server_hostname=None, session=None, context=None):
806+
if server_side:
807+
if server_hostname:
808+
raise ValueError("server_hostname can only be specified "
809+
"in client mode")
810+
if session is not None:
811+
raise ValueError("session can only be specified in "
812+
"client mode")
813+
if context.check_hostname and server_hostname is None:
814+
# Note: server_hostname='' is handled within _wrap_bio().
815+
warnings.warn("check_hostname requires server_hostname",
816+
category=DeprecationWarning,
817+
stacklevel=3)
818+
806819
self = cls.__new__(cls)
807820
sslobj = context._wrap_bio(
808821
incoming, outgoing, server_side=server_side,

‎Lib/test/test_asyncio/test_sslproto.py‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,56 @@ 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+
def test_check_hostname_requires_server_hostname(self):
86+
# A caller-supplied context asking for hostname checking used to be
87+
# taken through wrap_bio() with no name to check against, verifying
88+
# the certificate chain but never the peer's identity.
89+
# loop.start_tls() defaults server_hostname to None, and
90+
# loop.create_connection() turns server_hostname='' into None here,
91+
# so both reached that state.
92+
sslcontext = test_utils.simple_client_sslcontext(disable_verify=False)
93+
sslcontext.check_hostname = True
94+
app_proto = mock.Mock()
95+
waiter = mock.Mock()
96+
server_hostname = None
97+
98+
# Supplying no server_hostname warns with check_hostname enabled.
99+
with self.assertWarnsRegex(
100+
DeprecationWarning,
101+
'check_hostname requires server_hostname'):
102+
sslproto.SSLProtocol(self.loop, app_proto, sslcontext,
103+
waiter)
104+
105+
with self.assertWarnsRegex(
106+
DeprecationWarning,
107+
'check_hostname requires server_hostname'):
108+
sslproto.SSLProtocol(self.loop, app_proto, sslcontext,
109+
waiter,
110+
server_hostname=server_hostname)
111+
112+
# Disabling check_hostname allows for an empty or unset server_hostname.
113+
sslcontext.check_hostname = False
114+
115+
ssl_proto = sslproto.SSLProtocol(self.loop, app_proto, sslcontext, waiter)
116+
self.addCleanup(ssl_proto._app_transport.close)
117+
118+
ssl_proto = sslproto.SSLProtocol(self.loop, app_proto, sslcontext,
119+
waiter,
120+
server_hostname=server_hostname)
121+
self.addCleanup(ssl_proto._app_transport.close)
122+
73123
def test_eof_received_waiter(self):
74124
waiter = self.loop.create_future()
75125
ssl_proto = self.ssl_protocol(waiter=waiter)

‎Lib/test/test_ssl.py‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
from test.support import threading_helper
1111
from test.support import warnings_helper
1212
from test.support import asyncore
13+
import contextlib
1314
import array
1415
import re
1516
import socket
@@ -322,6 +323,34 @@ def testing_context(server_cert=SIGNED_CERTFILE, *, server_chain=True):
322323
return client_context, server_context, hostname
323324

324325

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

327356
def test_constants(self):
@@ -1692,6 +1721,10 @@ def test_subclass(self):
16921721

16931722
def test_bad_server_hostname(self):
16941723
ctx = ssl.create_default_context()
1724+
# Omitting the name entirely is bad too: this context checks it.
1725+
with self.assertWarns(DeprecationWarning):
1726+
ctx.wrap_bio(ssl.MemoryBIO(), ssl.MemoryBIO(),
1727+
server_hostname=None)
16951728
with self.assertRaises(ValueError):
16961729
ctx.wrap_bio(ssl.MemoryBIO(), ssl.MemoryBIO(),
16971730
server_hostname="")
@@ -1776,6 +1809,66 @@ def test_private_init(self):
17761809
with self.assertRaisesRegex(TypeError, "public constructor"):
17771810
ssl.SSLObject(bio, bio)
17781811

1812+
def test_check_hostname_requires_server_hostname(self):
1813+
# wrap_bio() used to accept a context asking for hostname checking
1814+
# without a name to check against, and then verify the certificate
1815+
# chain but never the peer's identity without a warning. Now
1816+
# a warning is emitted in this scenario.
1817+
client_context, _, hostname = testing_context()
1818+
self.assertTrue(client_context.check_hostname)
1819+
1820+
server_hostname = None
1821+
with self.assertWarnsRegex(
1822+
DeprecationWarning,
1823+
"check_hostname requires server_hostname"):
1824+
client_context.wrap_bio(ssl.MemoryBIO(), ssl.MemoryBIO(),
1825+
server_hostname=server_hostname)
1826+
# The sibling constructor refuses the very same call, but with
1827+
# a ValueError instead of DeprecationWarning.
1828+
with socket.socket() as sock:
1829+
with self.assertRaisesRegex(
1830+
ValueError,
1831+
"check_hostname requires server_hostname"):
1832+
client_context.wrap_socket(
1833+
sock, server_hostname=server_hostname)
1834+
1835+
# A name was all that was missing.
1836+
with warnings_helper.check_no_warnings(self):
1837+
client_context.wrap_bio(ssl.MemoryBIO(), ssl.MemoryBIO(),
1838+
server_hostname=hostname)
1839+
1840+
# Asking for no hostname check remains a way to say so explicitly.
1841+
context = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT)
1842+
context.check_hostname = False
1843+
self.assertFalse(context.check_hostname)
1844+
with warnings_helper.check_no_warnings(self):
1845+
context.wrap_bio(ssl.MemoryBIO(), ssl.MemoryBIO())
1846+
1847+
def test_server_side_bad_params(self):
1848+
# A server neither sends a hostname nor resumes a client's session,
1849+
# so wrap_bio() rejects both in server mode like wrap_socket()
1850+
client_context, server_context, hostname = testing_context()
1851+
1852+
with self.assertRaisesRegex(
1853+
ValueError,
1854+
"server_hostname can only be specified in client mode"):
1855+
server_context.wrap_bio(ssl.MemoryBIO(), ssl.MemoryBIO(),
1856+
server_side=True,
1857+
server_hostname=hostname)
1858+
1859+
client, server, *_ = connected_bio_pair(
1860+
client_context, server_context, hostname)
1861+
session = client.session
1862+
self.assertIsNotNone(session)
1863+
with self.assertRaisesRegex(
1864+
ValueError, "session can only be specified in client mode"):
1865+
server_context.wrap_bio(ssl.MemoryBIO(), ssl.MemoryBIO(),
1866+
server_side=True, session=session)
1867+
1868+
# Neither argument is what a server passes, so this still works.
1869+
server_context.wrap_bio(ssl.MemoryBIO(), ssl.MemoryBIO(),
1870+
server_side=True)
1871+
17791872
def test_unwrap(self):
17801873
client_ctx, server_ctx, hostname = testing_context()
17811874
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`, but for backward compatiblity reasons
4+
emits :exc:`DeprecationWarning` instead of :exc:`ValueError`.
5+
6+
In particular, a context with :attr:`~ssl.SSLContext.check_hostname` enabled
7+
and no *server_hostname* passed to :meth:`!wrap_bio` now emits
8+
:exc:`DeprecationWarning` to indicate the hostname wasn't checked.
9+
(In Python 3.13 and later, this raises :exc:`ValueError`.)
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
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``, emitting
5+
:exc:`DeprecationWarning` if *server_hostname* is missing. (This will raise
6+
:exc:`ValueError` in Python 3.13 and later.)

0 commit comments

Comments
 (0)