[#9561] Check remote certificates for XMPP TLS

docs/words/examples/xmpp_client.py

@@ -53,8 +53,9 @@ def connected(self, xs):
         xs.rawDataOutFn = self.rawDataOut
 
 
-    def disconnected(self, xs):
+    def disconnected(self, reason):
         print('Disconnected.')
+        print(reason)
 
         self.finished.callback(None)
 

src/twisted/words/newsfragments/9561.bugfix

@@ -0,0 +1 @@
+twisted.words.protocols.jabber.xmlstream.TLSInitiatingInitializer now properly verifies the server's certificate against platform CAs and the stream's domain, mitigating CVE-2019-12855.

src/twisted/words/newsfragments/9561.feature

@@ -0,0 +1 @@
+twisted.words.protocols.jabber.xmlstream.TLSInitiatingInitializer and twisted.words.protocols.jabber.client.XMPPClientFactory now take an optional configurationForTLS for customizing certificate options for StartTLS.

src/twisted/words/protocols/jabber/client.py

@@ -206,14 +206,10 @@ def associateWithStream(self, xs):
         xs.version = (0, 0)
         xmlstream.ConnectAuthenticator.associateWithStream(self, xs)
 
-        inits = [ (xmlstream.TLSInitiatingInitializer, False),
-                  (IQAuthInitializer, True),
-                ]
-
-        for initClass, required in inits:
-            init = initClass(xs)
-            init.required = required
-            xs.initializers.append(init)
+        xs.initializers = [
+            xmlstream.TLSInitiatingInitializer(xs, required=False),
+            IQAuthInitializer(xs),
+        ]
 
     # TODO: move registration into an Initializer?
 
@@ -302,7 +298,7 @@ def start(self):
 
 
 
-def XMPPClientFactory(jid, password):
+def XMPPClientFactory(jid, password, configurationForTLS=None):
     """
     Client factory for XMPP 1.0 (only).
 
@@ -314,12 +310,23 @@ def XMPPClientFactory(jid, password):
 
     @param jid: Jabber ID to connect with.
     @type jid: L{jid.JID}
+
     @param password: password to authenticate with.
     @type password: L{unicode}
+
+    @param configurationForTLS: An object which creates appropriately
+        configured TLS connections. This is passed to C{startTLS} on the
+        transport and is preferably created using
+        L{twisted.internet.ssl.optionsForClientTLS}. If C{None}, the default is
+        to verify the server certificate against the trust roots as provided by
+        the platform. See L{twisted.internet._sslverify.platformTrust}.
+    @type configurationForTLS: L{IOpenSSLClientConnectionCreator} or C{None}
+
     @return: XML stream factory.
     @rtype: L{xmlstream.XmlStreamFactory}
     """
-    a = XMPPAuthenticator(jid, password)
+    a = XMPPAuthenticator(jid, password,
+                          configurationForTLS=configurationForTLS)
     return xmlstream.XmlStreamFactory(a)
 
 
@@ -354,16 +361,29 @@ class XMPPAuthenticator(xmlstream.ConnectAuthenticator):
                resource binding step, and this is stored in this instance
                variable.
     @type jid: L{jid.JID}
+
     @ivar password: password to be used during SASL authentication.
     @type password: L{unicode}
     """
 
     namespace = 'jabber:client'
 
-    def __init__(self, jid, password):
+    def __init__(self, jid, password, configurationForTLS=None):
+        """
+        @param configurationForTLS: An object which creates appropriately
+            configured TLS connections. This is passed to C{startTLS} on the
+            transport and is preferably created using
+            L{twisted.internet.ssl.optionsForClientTLS}. If C{None}, the
+            default is to verify the server certificate against the trust roots
+            as provided by the platform. See
+            L{twisted.internet._sslverify.platformTrust}.
+        @type configurationForTLS: L{IOpenSSLClientConnectionCreator} or
+            C{None}
+        """
         xmlstream.ConnectAuthenticator.__init__(self, jid.host)
         self.jid = jid
         self.password = password
+        self._configurationForTLS = configurationForTLS
 
 
     def associateWithStream(self, xs):
@@ -377,14 +397,12 @@ def associateWithStream(self, xs):
         """
         xmlstream.ConnectAuthenticator.associateWithStream(self, xs)
 
-        xs.initializers = [CheckVersionInitializer(xs)]
-        inits = [ (xmlstream.TLSInitiatingInitializer, False),
-                  (sasl.SASLInitiatingInitializer, True),
-                  (BindInitializer, False),
-                  (SessionInitializer, False),
+        xs.initializers = [
+                CheckVersionInitializer(xs),
+                xmlstream.TLSInitiatingInitializer(
+                    xs, required=True,
+                    configurationForTLS=self._configurationForTLS),
+                sasl.SASLInitiatingInitializer(xs, required=True),
+                BindInitializer(xs, required=True),
+                SessionInitializer(xs, required=False),
                 ]
-
-        for initClass, required in inits:
-            init = initClass(xs)
-            init.required = required
-            xs.initializers.append(init)

src/twisted/words/protocols/jabber/xmlstream.py

@@ -316,16 +316,17 @@ class BaseFeatureInitiatingInitializer(object):
 
     @cvar feature: tuple of (uri, name) of the stream feature root element.
     @type feature: tuple of (C{str}, C{str})
+
     @ivar required: whether the stream feature is required to be advertized
                     by the receiving entity.
     @type required: C{bool}
     """
 
     feature = None
-    required = False
 
-    def __init__(self, xs):
+    def __init__(self, xs, required=False):
         self.xmlstream = xs
+        self.required = required
 
 
     def initialize(self):
@@ -400,21 +401,42 @@ class TLSInitiatingInitializer(BaseFeatureInitiatingInitializer):
     set the C{wanted} attribute to False instead of removing it from the list
     of initializers, so a proper exception L{TLSRequired} can be raised.
 
-    @cvar wanted: indicates if TLS negotiation is wanted.
+    @ivar wanted: indicates if TLS negotiation is wanted.
     @type wanted: C{bool}
     """
 
     feature = (NS_XMPP_TLS, 'starttls')
     wanted = True
     _deferred = None
+    _configurationForTLS = None
+
+    def __init__(self, xs, required=True, configurationForTLS=None):
+        """
+        @param configurationForTLS: An object which creates appropriately
+            configured TLS connections. This is passed to C{startTLS} on the
+            transport and is preferably created using
+            L{twisted.internet.ssl.optionsForClientTLS}.  If C{None}, the
+            default is to verify the server certificate against the trust roots
+            as provided by the platform. See
+            L{twisted.internet._sslverify.platformTrust}.
+        @type configurationForTLS: L{IOpenSSLClientConnectionCreator} or
+            C{None}
+        """
+        super(TLSInitiatingInitializer, self).__init__(
+                xs, required=required)
+        self._configurationForTLS = configurationForTLS
+
 
     def onProceed(self, obj):
         """
         Proceed with TLS negotiation and reset the XML stream.
         """
 
         self.xmlstream.removeObserver('/failure', self.onFailure)
-        ctx = ssl.CertificateOptions()
+        if self._configurationForTLS:
+            ctx = self._configurationForTLS
+        else:
+            ctx = ssl.optionsForClientTLS(self.xmlstream.otherEntity.host)
         self.xmlstream.transport.startTLS(ctx)
         self.xmlstream.reset()
         self.xmlstream.sendHeader()

src/twisted/words/test/test_jabberclient.py

@@ -16,6 +16,14 @@
 from twisted.words.protocols.jabber.sasl import SASLInitiatingInitializer
 from twisted.words.xish import utility
 
+try:
+    from twisted.internet import ssl
+except ImportError:
+    ssl = None
+    skipWhenNoSSL = "SSL not available"
+else:
+    skipWhenNoSSL = None
+
 IQ_AUTH_GET = '/iq[@type="get"]/query[@xmlns="jabber:iq:auth"]'
 IQ_AUTH_SET = '/iq[@type="set"]/query[@xmlns="jabber:iq:auth"]'
 NS_BIND = 'urn:ietf:params:xml:ns:xmpp-bind'
@@ -379,11 +387,51 @@ def onSession(iq):
 
 
 
+class BasicAuthenticatorTests(unittest.TestCase):
+    """
+    Test for both BasicAuthenticator and basicClientFactory.
+    """
+
+    def test_basic(self):
+        """
+        Authenticator and stream are properly constructed by the factory.
+
+        The L{xmlstream.XmlStream} protocol created by the factory has the new
+        L{client.BasicAuthenticator} instance in its C{authenticator}
+        attribute.  It is set up with C{jid} and C{password} as passed to the
+        factory, C{otherHost} taken from the client JID. The stream futher has
+        two initializers, for TLS and authentication, of which the first has
+        its C{required} attribute set to C{True}.
+        """
+        self.client_jid = jid.JID('user@example.com/resource')
+
+        # Get an XmlStream instance. Note that it gets initialized with the
+        # XMPPAuthenticator (that has its associateWithXmlStream called) that
+        # is in turn initialized with the arguments to the factory.
+        xs = client.basicClientFactory(self.client_jid,
+                                       'secret').buildProtocol(None)
+
+        # test authenticator's instance variables
+        self.assertEqual('example.com', xs.authenticator.otherHost)
+        self.assertEqual(self.client_jid, xs.authenticator.jid)
+        self.assertEqual('secret', xs.authenticator.password)
+
+        # test list of initializers
+        tls, auth = xs.initializers
+
+        self.assertIsInstance(tls, xmlstream.TLSInitiatingInitializer)
+        self.assertIsInstance(auth, client.IQAuthInitializer)
+
+        self.assertFalse(tls.required)
+
+
+
 class XMPPAuthenticatorTests(unittest.TestCase):
     """
     Test for both XMPPAuthenticator and XMPPClientFactory.
     """
-    def testBasic(self):
+
+    def test_basic(self):
         """
         Test basic operations.
 
@@ -412,7 +460,38 @@ def testBasic(self):
         self.assertIsInstance(bind, client.BindInitializer)
         self.assertIsInstance(session, client.SessionInitializer)
 
-        self.assertFalse(tls.required)
+        self.assertTrue(tls.required)
         self.assertTrue(sasl.required)
-        self.assertFalse(bind.required)
+        self.assertTrue(bind.required)
         self.assertFalse(session.required)
+
+
+    def test_tlsConfiguration(self):
+        """
+        A TLS configuration is passed to the TLS initializer.
+        """
+        configs = []
+
+        def init(self, xs, required=True, configurationForTLS=None):
+            configs.append(configurationForTLS)
+
+        self.client_jid = jid.JID('user@example.com/resource')
+
+        # Get an XmlStream instance. Note that it gets initialized with the
+        # XMPPAuthenticator (that has its associateWithXmlStream called) that
+        # is in turn initialized with the arguments to the factory.
+        configurationForTLS = ssl.CertificateOptions()
+        factory = client.XMPPClientFactory(
+            self.client_jid, 'secret',
+            configurationForTLS=configurationForTLS)
+        self.patch(xmlstream.TLSInitiatingInitializer, "__init__", init)
+        xs = factory.buildProtocol(None)
+
+        # test list of initializers
+        version, tls, sasl, bind, session = xs.initializers
+
+        self.assertIsInstance(tls, xmlstream.TLSInitiatingInitializer)
+        self.assertIs(configurationForTLS, configs[0])
+
+
+    test_tlsConfiguration.skip = skipWhenNoSSL