From 9d50ab563df6307cabbcc9883cb8c52c614b0f22 Mon Sep 17 00:00:00 2001 From: Christian Heimes Date: Tue, 27 Feb 2018 10:17:30 +0100 Subject: bpo-32951: Disable SSLSocket/SSLObject constructor (#5864) Direct instantiation of SSLSocket and SSLObject objects is now prohibited. The constructors were never documented, tested, or designed as public constructors. The SSLSocket constructor had limitations. For example it was not possible to enabled hostname verification except was ssl_version=PROTOCOL_TLS_CLIENT with cert_reqs=CERT_REQUIRED. SSLContext.wrap_socket() and SSLContext.wrap_bio are the recommended API to construct SSLSocket and SSLObject instances. ssl.wrap_socket() is also deprecated. The only test case for direct instantiation was added a couple of days ago for IDNA testing. Signed-off-by: Christian Heimes --- Doc/library/ssl.rst | 24 +++- Doc/whatsnew/3.7.rst | 6 + Lib/ssl.py | 138 ++++++++++----------- Lib/test/test_ssl.py | 20 +-- .../2018-02-25-18-22-01.bpo-32951.gHrCXq.rst | 3 + 5 files changed, 107 insertions(+), 84 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2018-02-25-18-22-01.bpo-32951.gHrCXq.rst diff --git a/Doc/library/ssl.rst b/Doc/library/ssl.rst index 4889a71..d18a505 100644 --- a/Doc/library/ssl.rst +++ b/Doc/library/ssl.rst @@ -998,7 +998,7 @@ SSL Sockets the specification of normal, OS-level sockets. See especially the :ref:`notes on non-blocking sockets `. - :class:`SSLSocket` are not created directly, but using the + Instances of :class:`SSLSocket` must be created using the :meth:`SSLContext.wrap_socket` method. .. versionchanged:: 3.5 @@ -1013,6 +1013,11 @@ SSL Sockets It is deprecated to create a :class:`SSLSocket` instance directly, use :meth:`SSLContext.wrap_socket` to wrap a socket. + .. versionchanged:: 3.7 + :class:`SSLSocket` instances must to created with + :meth:`~SSLContext.wrap_socket`. In earlier versions, it was possible + to create instances directly. This was never documented or officially + supported. SSL sockets also have the following additional methods and attributes: @@ -2249,11 +2254,12 @@ provided. but does not provide any network IO itself. IO needs to be performed through separate "BIO" objects which are OpenSSL's IO abstraction layer. - An :class:`SSLObject` instance can be created using the - :meth:`~SSLContext.wrap_bio` method. This method will create the - :class:`SSLObject` instance and bind it to a pair of BIOs. The *incoming* - BIO is used to pass data from Python to the SSL protocol instance, while the - *outgoing* BIO is used to pass data the other way around. + This class has no public constructor. An :class:`SSLObject` instance + must be created using the :meth:`~SSLContext.wrap_bio` method. This + method will create the :class:`SSLObject` instance and bind it to a + pair of BIOs. The *incoming* BIO is used to pass data from Python to the + SSL protocol instance, while the *outgoing* BIO is used to pass data the + other way around. The following methods are available: @@ -2305,6 +2311,12 @@ provided. :meth:`~SSLContext.wrap_socket`. An :class:`SSLObject` is always created via an :class:`SSLContext`. + .. versionchanged:: 3.7 + :class:`SSLObject` instances must to created with + :meth:`~SSLContext.wrap_bio`. In earlier versions, it was possible to + create instances directly. This was never documented or officially + supported. + An SSLObject communicates with the outside world using memory buffers. The class :class:`MemoryBIO` provides a memory buffer that can be used for this purpose. It wraps an OpenSSL memory BIO (Basic IO) object: diff --git a/Doc/whatsnew/3.7.rst b/Doc/whatsnew/3.7.rst index e25ff10..2d62ffa 100644 --- a/Doc/whatsnew/3.7.rst +++ b/Doc/whatsnew/3.7.rst @@ -677,6 +677,12 @@ OpenSSL 1.1.1. (Contributed by Christian Heimes in :issue:`32947`, recommend :meth:`~ssl.SSLContext.wrap_socket` instead. (Contributed by Christian Heimes in :issue:`28124`.) +:class:`~ssl.SSLSocket` and :class:`~ssl.SSLObject` no longer have a public +constructor. Direct instantiation was never a documented and supported +feature. Instances must be created with :class:`~ssl.SSLContext` methods +:meth:`~ssl.SSLContext.wrap_socket` and :meth:`~ssl.SSLContext.wrap_bio`. +(Contributed by Christian Heimes in :issue:`32951`) + string ------ diff --git a/Lib/ssl.py b/Lib/ssl.py index 94ea35e..75ebcc1 100644 --- a/Lib/ssl.py +++ b/Lib/ssl.py @@ -390,24 +390,24 @@ class SSLContext(_SSLContext): server_hostname=None, session=None): # SSLSocket class handles server_hostname encoding before it calls # ctx._wrap_socket() - return self.sslsocket_class( + return self.sslsocket_class._create( sock=sock, server_side=server_side, do_handshake_on_connect=do_handshake_on_connect, suppress_ragged_eofs=suppress_ragged_eofs, server_hostname=server_hostname, - _context=self, - _session=session + context=self, + session=session ) def wrap_bio(self, incoming, outgoing, server_side=False, server_hostname=None, session=None): # Need to encode server_hostname here because _wrap_bio() can only # handle ASCII str. - return self.sslobject_class( + return self.sslobject_class._create( incoming, outgoing, server_side=server_side, server_hostname=self._encode_hostname(server_hostname), - session=session, _context=self, + session=session, context=self, ) def set_npn_protocols(self, npn_protocols): @@ -612,14 +612,23 @@ class SSLObject: * Any form of network IO incluging methods such as ``recv`` and ``send``. * The ``do_handshake_on_connect`` and ``suppress_ragged_eofs`` machinery. """ + def __init__(self, *args, **kwargs): + raise TypeError( + f"{self.__class__.__name__} does not have a public " + f"constructor. Instances are returned by SSLContext.wrap_bio()." + ) - def __init__(self, incoming, outgoing, server_side=False, - server_hostname=None, session=None, _context=None): - self._sslobj = _context._wrap_bio( + @classmethod + def _create(cls, incoming, outgoing, server_side=False, + server_hostname=None, session=None, context=None): + self = cls.__new__(cls) + sslobj = context._wrap_bio( incoming, outgoing, server_side=server_side, server_hostname=server_hostname, owner=self, session=session ) + self._sslobj = sslobj + return self @property def context(self): @@ -741,72 +750,48 @@ class SSLObject: class SSLSocket(socket): """This class implements a subtype of socket.socket that wraps the underlying OS socket in an SSL context when necessary, and - provides read and write methods over that channel.""" - - def __init__(self, sock=None, keyfile=None, certfile=None, - server_side=False, cert_reqs=CERT_NONE, - ssl_version=PROTOCOL_TLS, ca_certs=None, - do_handshake_on_connect=True, - family=AF_INET, type=SOCK_STREAM, proto=0, fileno=None, - suppress_ragged_eofs=True, npn_protocols=None, ciphers=None, - server_hostname=None, - _context=None, _session=None): - - if _context: - self._context = _context - else: - if server_side and not certfile: - raise ValueError("certfile must be specified for server-side " - "operations") - if keyfile and not certfile: - raise ValueError("certfile must be specified") - if certfile and not keyfile: - keyfile = certfile - self._context = SSLContext(ssl_version) - self._context.verify_mode = cert_reqs - if ca_certs: - self._context.load_verify_locations(ca_certs) - if certfile: - self._context.load_cert_chain(certfile, keyfile) - if npn_protocols: - self._context.set_npn_protocols(npn_protocols) - if ciphers: - self._context.set_ciphers(ciphers) - self.keyfile = keyfile - self.certfile = certfile - self.cert_reqs = cert_reqs - self.ssl_version = ssl_version - self.ca_certs = ca_certs - self.ciphers = ciphers - # Can't use sock.type as other flags (such as SOCK_NONBLOCK) get - # mixed in. + provides read and write methods over that channel. """ + + def __init__(self, *args, **kwargs): + raise TypeError( + f"{self.__class__.__name__} does not have a public " + f"constructor. Instances are returned by " + f"SSLContext.wrap_socket()." + ) + + @classmethod + def _create(cls, sock, server_side=False, do_handshake_on_connect=True, + suppress_ragged_eofs=True, server_hostname=None, + context=None, session=None): if sock.getsockopt(SOL_SOCKET, SO_TYPE) != SOCK_STREAM: raise NotImplementedError("only stream sockets are supported") if server_side: if server_hostname: raise ValueError("server_hostname can only be specified " "in client mode") - if _session is not None: + if session is not None: raise ValueError("session can only be specified in " "client mode") - if self._context.check_hostname and not server_hostname: + if context.check_hostname and not server_hostname: raise ValueError("check_hostname requires server_hostname") - self._session = _session + + kwargs = dict( + family=sock.family, type=sock.type, proto=sock.proto, + fileno=sock.fileno() + ) + self = cls.__new__(cls, **kwargs) + super(SSLSocket, self).__init__(**kwargs) + self.settimeout(sock.gettimeout()) + sock.detach() + + self._context = context + self._session = session + self._closed = False + self._sslobj = None self.server_side = server_side - self.server_hostname = self._context._encode_hostname(server_hostname) + self.server_hostname = context._encode_hostname(server_hostname) self.do_handshake_on_connect = do_handshake_on_connect self.suppress_ragged_eofs = suppress_ragged_eofs - if sock is not None: - super().__init__(family=sock.family, - type=sock.type, - proto=sock.proto, - fileno=sock.fileno()) - self.settimeout(sock.gettimeout()) - sock.detach() - elif fileno is not None: - super().__init__(fileno=fileno) - else: - super().__init__(family=family, type=type, proto=proto) # See if we are connected try: @@ -818,8 +803,6 @@ class SSLSocket(socket): else: connected = True - self._closed = False - self._sslobj = None self._connected = connected if connected: # create the SSL object @@ -834,10 +817,10 @@ class SSLSocket(socket): # non-blocking raise ValueError("do_handshake_on_connect should not be specified for non-blocking sockets") self.do_handshake() - except (OSError, ValueError): self.close() raise + return self @property def context(self): @@ -1184,12 +1167,25 @@ def wrap_socket(sock, keyfile=None, certfile=None, do_handshake_on_connect=True, suppress_ragged_eofs=True, ciphers=None): - return SSLSocket(sock=sock, keyfile=keyfile, certfile=certfile, - server_side=server_side, cert_reqs=cert_reqs, - ssl_version=ssl_version, ca_certs=ca_certs, - do_handshake_on_connect=do_handshake_on_connect, - suppress_ragged_eofs=suppress_ragged_eofs, - ciphers=ciphers) + + if server_side and not certfile: + raise ValueError("certfile must be specified for server-side " + "operations") + if keyfile and not certfile: + raise ValueError("certfile must be specified") + context = SSLContext(ssl_version) + context.verify_mode = cert_reqs + if ca_certs: + context.load_verify_locations(ca_certs) + if certfile: + context.load_cert_chain(certfile, keyfile) + if ciphers: + context.set_ciphers(ciphers) + return context.wrap_socket( + sock=sock, server_side=server_side, + do_handshake_on_connect=do_handshake_on_connect, + suppress_ragged_eofs=suppress_ragged_eofs + ) # some utility functions diff --git a/Lib/test/test_ssl.py b/Lib/test/test_ssl.py index d978a9e..ca2357e 100644 --- a/Lib/test/test_ssl.py +++ b/Lib/test/test_ssl.py @@ -263,6 +263,11 @@ class BasicSocketTests(unittest.TestCase): ssl.OP_NO_TLSv1_2 self.assertEqual(ssl.PROTOCOL_TLS, ssl.PROTOCOL_SSLv23) + def test_private_init(self): + with self.assertRaisesRegex(TypeError, "public constructor"): + with socket.socket() as s: + ssl.SSLSocket(s) + def test_str_for_enums(self): # Make sure that the PROTOCOL_* constants have enum-like string # reprs. @@ -1657,6 +1662,13 @@ class MemoryBIOTests(unittest.TestCase): self.assertRaises(TypeError, bio.write, 1) +class SSLObjectTests(unittest.TestCase): + def test_private_init(self): + bio = ssl.MemoryBIO() + with self.assertRaisesRegex(TypeError, "public constructor"): + ssl.SSLObject(bio, bio) + + class SimpleBackgroundTests(unittest.TestCase): """Tests that connect to a simple server running in the background""" @@ -2735,12 +2747,6 @@ class ThreadedTests(unittest.TestCase): self.assertEqual(s.server_hostname, expected_hostname) self.assertTrue(cert, "Can't get peer certificate.") - with ssl.SSLSocket(socket.socket(), - server_hostname=server_hostname) as s: - s.connect((HOST, server.port)) - s.getpeercert() - self.assertEqual(s.server_hostname, expected_hostname) - # incorrect hostname should raise an exception server = ThreadedEchoServer(context=server_context, chatty=True) with server: @@ -3999,7 +4005,7 @@ def test_main(verbose=False): tests = [ ContextTests, BasicSocketTests, SSLErrorTests, MemoryBIOTests, - SimpleBackgroundTests, ThreadedTests, + SSLObjectTests, SimpleBackgroundTests, ThreadedTests, ] if support.is_resource_enabled('network'): diff --git a/Misc/NEWS.d/next/Library/2018-02-25-18-22-01.bpo-32951.gHrCXq.rst b/Misc/NEWS.d/next/Library/2018-02-25-18-22-01.bpo-32951.gHrCXq.rst new file mode 100644 index 0000000..9c038cf --- /dev/null +++ b/Misc/NEWS.d/next/Library/2018-02-25-18-22-01.bpo-32951.gHrCXq.rst @@ -0,0 +1,3 @@ +Direct instantiation of SSLSocket and SSLObject objects is now prohibited. +The constructors were never documented, tested, or designed as public +constructors. Users were suppose to use ssl.wrap_socket() or SSLContext. -- cgit v0.12