Repository: pyca/pyopenssl
Pull request #804 (2019-01-08 18:04:18)

Handle NULL bytes in get_components() values
Some old software may generate "bogus" CN with each character preceded
by a NULL.

This is already handled in commonName, but wasn't in get_components()

romuald: (2 failed tests here, but they are the same as the tests failing on the master branch)
reaperhulk: You'll need to use `b'CN'` so that the test can work on both python 2 and 3. Additionally, this can be simplified to: `value = _ffi.buffer(_lib.ASN1_STRING_data(fval), _lib.ASN1_STRING_length(fval))[:]` rather than unpack call.
romuald: @reaperhulk done. I'm not familiar with FFI yet thanks :)
reaperhulk: Thanks for the quick update. Now the maintainers (that includes me, sigh) need to fix the other tests so we can get this merged!

Patch:
diff --git a/setup.py b/setup.py
index 3728795..5387d2a 100755
--- a/setup.py
+++ b/setup.py
@@ -49,7 +49,7 @@ LONG = (
     read_file("README.rst") + "\n\n" +
     "Release Information\n" +
     "===================\n\n" +
-    re.search("(\d{2}.\d.\d \(.*?\)\n.*?)\n\n\n----\n",
+    re.search(r"(\d{2}.\d.\d \(.*?\)\n.*?)\n\n\n----\n",
               read_file("CHANGELOG.rst"), re.S).group(1) +
     "\n\n`Full changelog " +
     "<{uri}en/stable/changelog.html>`_.\n\n"
diff --git a/src/OpenSSL/SSL.py b/src/OpenSSL/SSL.py
index 5cf39c0..0687fc3 100644
--- a/src/OpenSSL/SSL.py
+++ b/src/OpenSSL/SSL.py
@@ -1185,6 +1185,19 @@ class Context(object):
         _openssl_assert(
             _lib.SSL_CTX_set_cipher_list(self._context, cipher_list) == 1
         )
+        # In OpenSSL 1.1.1 setting the cipher list will always return TLS 1.3
+        # ciphers even if you pass an invalid cipher. Applications (like
+        # Twisted) have tests that depend on an error being raised if an
+        # invalid cipher string is passed, but without the following check
+        # for the TLS 1.3 specific cipher suites it would never error.
+        tmpconn = Connection(self, None)
+        _openssl_assert(
+            tmpconn.get_cipher_list() != [
+                'TLS_AES_256_GCM_SHA384',
+                'TLS_CHACHA20_POLY1305_SHA256',
+                'TLS_AES_128_GCM_SHA256'
+            ]
+        )
 
     def set_client_ca_list(self, certificate_authorities):
         """
diff --git a/src/OpenSSL/crypto.py b/src/OpenSSL/crypto.py
index d555083..715e1ae 100644
--- a/src/OpenSSL/crypto.py
+++ b/src/OpenSSL/crypto.py
@@ -695,11 +695,11 @@ class X509Name(object):
             nid = _lib.OBJ_obj2nid(fname)
             name = _lib.OBJ_nid2sn(nid)
 
-            result.append((
-                _ffi.string(name),
-                _ffi.string(
-                    _lib.ASN1_STRING_data(fval),
-                    _lib.ASN1_STRING_length(fval))))
+            # ffi.string does not handle strings containing NULL bytes
+            # (which may have been generated by old, broken software)
+            value = _ffi.buffer(_lib.ASN1_STRING_data(fval),
+                                _lib.ASN1_STRING_length(fval))[:]
+            result.append((_ffi.string(name), value))
 
         return result
 
diff --git a/tox.ini b/tox.ini
index 8400a5d..8bef9e3 100644
--- a/tox.ini
+++ b/tox.ini
@@ -27,6 +27,7 @@ deps =
     git+https://github.com/twisted/twisted
     idna
     service_identity
+    bcrypt
 passenv = ARCHFLAGS CFLAGS LC_ALL LDFLAGS PATH LD_LIBRARY_PATH TERM
 commands =
     python -c "import OpenSSL.SSL; print(OpenSSL.SSL.SSLeay_version(OpenSSL.SSL.SSLEAY_VERSION))"
@@ -38,7 +39,7 @@ basepython=python3.5
 deps =
     pyasn1
     ndg-httpsclient
-passenv = ARCHFLAGS CFLAGS LC_ALL LDFLAGS PATH LD_LIBRARY_PATH TERM
+passenv = ARCHFLAGS CFLAGS LC_ALL LDFLAGS PATH LD_LIBRARY_PATH TERM TRAVIS_INFRA
 whitelist_externals =
     rm
 commands =


Test patch:
diff --git a/tests/test_crypto.py b/tests/test_crypto.py
index eb4590d..c938021 100644
--- a/tests/test_crypto.py
+++ b/tests/test_crypto.py
@@ -1214,6 +1214,17 @@ class TestX509Name(object):
         subject = cert.get_subject()
         assert "null.python.org\x00example.org" == subject.commonName
 
+    def test_load_nul_byte_components(self):
+        """
+        An `X509Name` from an `X509` instance loaded from a file can have a
+        NUL byte in the value of its components
+        """
+        cert = load_certificate(FILETYPE_PEM, nulbyteSubjectAltNamePEM)
+        subject = cert.get_subject()
+        components = subject.get_components()
+        ccn = [value for name, value in components if name == b'CN']
+        assert ccn[0] == b'null.python.org\x00example.org'
+
     def test_set_attribute_failure(self):
         """
         If the value of an attribute cannot be set for some reason then
@@ -3156,20 +3167,20 @@ class TestCRL(object):
         representing a serial number, a revoked reason, and certificate issuer
         information.
         """
-        crl = self._get_crl()
         # PEM format
-        dumped_crl = crl.export(
+        dumped_crl = self._get_crl().export(
             self.cert, self.pkey, days=20, digest=b"sha256"
         )
-        text = _runopenssl(dumped_crl, b"crl", b"-noout", b"-text")
-
-        # These magic values are based on the way the CRL above was constructed
-        # and with what certificate it was exported.
-        text.index(b'Serial Number: 03AB')
-        text.index(b'Superseded')
-        text.index(
-            b'Issuer: /C=US/ST=IL/L=Chicago/O=Testing/CN=Testing Root CA'
-        )
+        crl = x509.load_pem_x509_crl(dumped_crl, backend)
+        revoked = crl.get_revoked_certificate_by_serial_number(0x03AB)
+        assert revoked is not None
+        assert crl.issuer == x509.Name([
+            x509.NameAttribute(x509.NameOID.COUNTRY_NAME, u"US"),
+            x509.NameAttribute(x509.NameOID.STATE_OR_PROVINCE_NAME, u"IL"),
+            x509.NameAttribute(x509.NameOID.LOCALITY_NAME, u"Chicago"),
+            x509.NameAttribute(x509.NameOID.ORGANIZATION_NAME, u"Testing"),
+            x509.NameAttribute(x509.NameOID.COMMON_NAME, u"Testing Root CA"),
+        ])
 
     def test_export_der(self):
         """
@@ -3180,17 +3191,19 @@ class TestCRL(object):
         crl = self._get_crl()
 
         # DER format
-        dumped_crl = crl.export(
+        dumped_crl = self._get_crl().export(
             self.cert, self.pkey, FILETYPE_ASN1, digest=b"md5"
         )
-        text = _runopenssl(
-            dumped_crl, b"crl", b"-noout", b"-text", b"-inform", b"DER"
-        )
-        text.index(b'Serial Number: 03AB')
-        text.index(b'Superseded')
-        text.index(
-            b'Issuer: /C=US/ST=IL/L=Chicago/O=Testing/CN=Testing Root CA'
-        )
+        crl = x509.load_der_x509_crl(dumped_crl, backend)
+        revoked = crl.get_revoked_certificate_by_serial_number(0x03AB)
+        assert revoked is not None
+        assert crl.issuer == x509.Name([
+            x509.NameAttribute(x509.NameOID.COUNTRY_NAME, u"US"),
+            x509.NameAttribute(x509.NameOID.STATE_OR_PROVINCE_NAME, u"IL"),
+            x509.NameAttribute(x509.NameOID.LOCALITY_NAME, u"Chicago"),
+            x509.NameAttribute(x509.NameOID.ORGANIZATION_NAME, u"Testing"),
+            x509.NameAttribute(x509.NameOID.COMMON_NAME, u"Testing Root CA"),
+        ])
 
     # Flaky because we compare the output of running commands which sometimes
     # varies by 1 second
@@ -3207,7 +3220,8 @@ class TestCRL(object):
             self.cert, self.pkey, FILETYPE_ASN1, digest=b"md5"
         )
         text = _runopenssl(
-            dumped_crl, b"crl", b"-noout", b"-text", b"-inform", b"DER"
+            dumped_crl, b"crl", b"-noout", b"-text", b"-inform", b"DER",
+            b"-nameopt", b""
         )
 
         # text format
@@ -3778,7 +3792,7 @@ class EllipticCurveFactory(object):
 
 class TestEllipticCurveEquality(EqualityTestsMixin):
     """
-    Tests `_EllipticCurve`\ 's implementation of ``==`` and ``!=``.
+    Tests `_EllipticCurve`'s implementation of ``==`` and ``!=``.
     """
     curve_factory = EllipticCurveFactory()
 
diff --git a/tests/test_ssl.py b/tests/test_ssl.py
index fbf0760..bddeaa9 100644
--- a/tests/test_ssl.py
+++ b/tests/test_ssl.py
@@ -216,14 +216,14 @@ def _create_certificate_chain():
     return [(cakey, cacert), (ikey, icert), (skey, scert)]
 
 
-def loopback_client_factory(socket):
-    client = Connection(Context(SSLv23_METHOD), socket)
+def loopback_client_factory(socket, version=SSLv23_METHOD):
+    client = Connection(Context(version), socket)
     client.set_connect_state()
     return client
 
 
-def loopback_server_factory(socket):
-    ctx = Context(SSLv23_METHOD)
+def loopback_server_factory(socket, version=SSLv23_METHOD):
+    ctx = Context(version)
     ctx.use_privatekey(load_privatekey(FILETYPE_PEM, server_key_pem))
     ctx.use_certificate(load_certificate(FILETYPE_PEM, server_cert_pem))
     server = Connection(ctx, socket)
@@ -1307,13 +1307,13 @@ class TestContext(object):
         exception, verification fails and the exception is propagated to the
         caller of `Connection.do_handshake`.
         """
-        serverContext = Context(TLSv1_METHOD)
+        serverContext = Context(TLSv1_2_METHOD)
         serverContext.use_privatekey(
             load_privatekey(FILETYPE_PEM, cleartextPrivateKeyPEM))
         serverContext.use_certificate(
             load_certificate(FILETYPE_PEM, cleartextCertificatePEM))
 
-        clientContext = Context(TLSv1_METHOD)
+        clientContext = Context(TLSv1_2_METHOD)
 
         def verify_callback(*args):
             raise Exception("silly verify failure")
@@ -2539,7 +2539,7 @@ class TestConnection(object):
         """
         key = load_privatekey(FILETYPE_PEM, server_key_pem)
         cert = load_certificate(FILETYPE_PEM, server_cert_pem)
-        ctx = Context(SSLv23_METHOD)
+        ctx = Context(TLSv1_2_METHOD)
         ctx.use_privatekey(key)
         ctx.use_certificate(cert)
         ctx.set_session_id("unity-test")
@@ -3193,7 +3193,10 @@ class TestConnectionRenegotiate(object):
         """
         Go through a complete renegotiation cycle.
         """
-        server, client = loopback()
+        server, client = loopback(
+            lambda s: loopback_server_factory(s, TLSv1_2_METHOD),
+            lambda s: loopback_client_factory(s, TLSv1_2_METHOD),
+        )
 
         server.send(b"hello world")