From ff83cddd17d5c4b657a9b1bf32f0ce9c1847001f Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Mon, 12 Aug 2013 18:05:51 -0400 Subject: Ghetto rebase Heimes' changes onto the 0.13 release branch --- ChangeLog | 6 ++++ OpenSSL/crypto/x509.c | 1 + OpenSSL/crypto/x509ext.c | 78 ++++++++++++++++++++++++++++++++++++++++++++- OpenSSL/test/test_crypto.py | 59 ++++++++++++++++++++++++++++++++++ 4 files changed, 143 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index 068a5f2..94d852e 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +2013-08-11 Christian Heimes + + * OpenSSL/crypto/x509ext.c: Fix handling of NULL bytes inside + subjectAltName general names, CVE-2013-4073. + * OpenSSL/crypto/x509.c: Fix memory leak in get_extension(). + 2011-08-14 Jean-Paul Calderone * Release 0.13 diff --git a/OpenSSL/crypto/x509.c b/OpenSSL/crypto/x509.c index 0754dec..c086ce8 100644 --- a/OpenSSL/crypto/x509.c +++ b/OpenSSL/crypto/x509.c @@ -756,6 +756,7 @@ crypto_X509_get_extension(crypto_X509Obj *self, PyObject *args) { extobj = PyObject_New(crypto_X509ExtensionObj, &crypto_X509Extension_Type); extobj->x509_extension = X509_EXTENSION_dup(ext); + extobj->dealloc = 1; return (PyObject*)extobj; } diff --git a/OpenSSL/crypto/x509ext.c b/OpenSSL/crypto/x509ext.c index adbe084..caa78b0 100644 --- a/OpenSSL/crypto/x509ext.c +++ b/OpenSSL/crypto/x509ext.c @@ -236,6 +236,75 @@ crypto_X509Extension_dealloc(crypto_X509ExtensionObj *self) PyObject_Del(self); } + +/* Special handling of subjectAltName, see CVE-2013-4073 */ + +int +crypto_X509Extension_str_san(crypto_X509ExtensionObj *self, BIO *bio) +{ + GENERAL_NAMES *names; + const X509V3_EXT_METHOD *method = NULL; + long i, length, num; + const unsigned char *p; + + method = X509V3_EXT_get(self->x509_extension); + if (method == NULL) { + return -1; + } + + p = self->x509_extension->value->data; + length = self->x509_extension->value->length; + if (method->it) { + names = (GENERAL_NAMES*)(ASN1_item_d2i(NULL, &p, length, + ASN1_ITEM_ptr(method->it))); + } + else { + names = (GENERAL_NAMES*)(method->d2i(NULL, &p, length)); + } + if (names == NULL) { + return -1; + } + + num = sk_GENERAL_NAME_num(names); + for (i = 0; i < num; i++) { + GENERAL_NAME *name; + ASN1_STRING *as; + name = sk_GENERAL_NAME_value(names, i); + switch (name->type) { + case GEN_EMAIL: + BIO_puts(bio, "email:"); + as = name->d.rfc822Name; + BIO_write(bio, ASN1_STRING_data(as), + ASN1_STRING_length(as)); + break; + case GEN_DNS: + BIO_puts(bio, "DNS:"); + as = name->d.dNSName; + BIO_write(bio, ASN1_STRING_data(as), + ASN1_STRING_length(as)); + break; + case GEN_URI: + BIO_puts(bio, "URI:"); + as = name->d.uniformResourceIdentifier; + BIO_write(bio, ASN1_STRING_data(as), + ASN1_STRING_length(as)); + break; + default: + /* use builtin print for GEN_OTHERNAME, GEN_X400, + * GEN_EDIPARTY, GEN_DIRNAME, GEN_IPADD and GEN_RID + */ + GENERAL_NAME_print(bio, name); + } + /* trailing ', ' except for last element */ + if (i < (num - 1)) { + BIO_puts(bio, ", "); + } + } + sk_GENERAL_NAME_pop_free(names, GENERAL_NAME_free); + + return 0; +} + /* * Print a nice text representation of the certificate request. */ @@ -247,7 +316,14 @@ crypto_X509Extension_str(crypto_X509ExtensionObj *self) PyObject *str; BIO *bio = BIO_new(BIO_s_mem()); - if (!X509V3_EXT_print(bio, self->x509_extension, 0, 0)) + if (OBJ_obj2nid(self->x509_extension->object) == NID_subject_alt_name) { + if (crypto_X509Extension_str_san(self, bio) == -1) { + BIO_free(bio); + exception_from_error_queue(crypto_Error); + return NULL; + } + } + else if (!X509V3_EXT_print(bio, self->x509_extension, 0, 0)) { BIO_free(bio); exception_from_error_queue(crypto_Error); diff --git a/OpenSSL/test/test_crypto.py b/OpenSSL/test/test_crypto.py index 5cdcefb..e65917e 100644 --- a/OpenSSL/test/test_crypto.py +++ b/OpenSSL/test/test_crypto.py @@ -265,6 +265,37 @@ oolb6NMg/R3enNPvS1O4UU1H8wpaF77L4yiSWlE0p4w= -----END RSA PRIVATE KEY----- """) +# certificate with NULL bytes in subjectAltName and common name + +nullbyte_san_PEM = b("""-----BEGIN CERTIFICATE----- +MIIE2DCCA8CgAwIBAgIBADANBgkqhkiG9w0BAQUFADCBxTELMAkGA1UEBhMCVVMx +DzANBgNVBAgMBk9yZWdvbjESMBAGA1UEBwwJQmVhdmVydG9uMSMwIQYDVQQKDBpQ +eXRob24gU29mdHdhcmUgRm91bmRhdGlvbjEgMB4GA1UECwwXUHl0aG9uIENvcmUg +RGV2ZWxvcG1lbnQxJDAiBgNVBAMMG251bGwucHl0aG9uLm9yZwBleGFtcGxlLm9y +ZzEkMCIGCSqGSIb3DQEJARYVcHl0aG9uLWRldkBweXRob24ub3JnMB4XDTEzMDgw +NzEzMTE1MloXDTEzMDgwNzEzMTI1MlowgcUxCzAJBgNVBAYTAlVTMQ8wDQYDVQQI +DAZPcmVnb24xEjAQBgNVBAcMCUJlYXZlcnRvbjEjMCEGA1UECgwaUHl0aG9uIFNv +ZnR3YXJlIEZvdW5kYXRpb24xIDAeBgNVBAsMF1B5dGhvbiBDb3JlIERldmVsb3Bt +ZW50MSQwIgYDVQQDDBtudWxsLnB5dGhvbi5vcmcAZXhhbXBsZS5vcmcxJDAiBgkq +hkiG9w0BCQEWFXB5dGhvbi1kZXZAcHl0aG9uLm9yZzCCASIwDQYJKoZIhvcNAQEB +BQADggEPADCCAQoCggEBALXq7cn7Rn1vO3aA3TrzA5QLp6bb7B3f/yN0CJ2XFj+j +pHs+Gw6WWSUDpybiiKnPec33BFawq3kyblnBMjBU61ioy5HwQqVkJ8vUVjGIUq3P +vX/wBmQfzCe4o4uM89gpHyUL9UYGG8oCRa17dgqcv7u5rg0Wq2B1rgY+nHwx3JIv +KRrgSwyRkGzpN8WQ1yrXlxWjgI9de0mPVDDUlywcWze1q2kwaEPTM3hLAmD1PESA +oY/n8A/RXoeeRs9i/Pm/DGUS8ZPINXk/yOzsR/XvvkTVroIeLZqfmFpnZeF0cHzL +08LODkVJJ9zjLdT7SA4vnne4FEbAxDbKAq5qkYzaL4UCAwEAAaOB0DCBzTAMBgNV +HRMBAf8EAjAAMB0GA1UdDgQWBBSIWlXAUv9hzVKjNQ/qWpwkOCL3XDALBgNVHQ8E +BAMCBeAwgZAGA1UdEQSBiDCBhYIeYWx0bnVsbC5weXRob24ub3JnAGV4YW1wbGUu +Y29tgSBudWxsQHB5dGhvbi5vcmcAdXNlckBleGFtcGxlLm9yZ4YpaHR0cDovL251 +bGwucHl0aG9uLm9yZwBodHRwOi8vZXhhbXBsZS5vcmeHBMAAAgGHECABDbgAAAAA +AAAAAAAAAAEwDQYJKoZIhvcNAQEFBQADggEBAKxPRe99SaghcI6IWT7UNkJw9aO9 +i9eo0Fj2MUqxpKbdb9noRDy2CnHWf7EIYZ1gznXPdwzSN4YCjV5d+Q9xtBaowT0j +HPERs1ZuytCNNJTmhyqZ8q6uzMLoht4IqH/FBfpvgaeC5tBTnTT0rD5A/olXeimk +kX4LxlEx5RAvpGB2zZVRGr6LobD9rVK91xuHYNIxxxfEGE8tCCWjp0+3ksri9SXx +VHWBnbM9YaL32u3hxm8sYB/Yb8WSBavJCWJJqRStVRHM1koZlJmXNx2BX4vPo6iW +RFEIPQsFZRLrtnCAiEhyT8bC2s/Njlu6ly9gtJZWSV46Q3ZjBL4q9sHKqZQ= +-----END CERTIFICATE-----""") + class X509ExtTests(TestCase): """ @@ -1381,6 +1412,34 @@ WpOdIpB8KksUTCzV591Nr1wd self.assertRaises(IndexError, cert.get_extension, 4) self.assertRaises(TypeError, cert.get_extension, "hello") + def test_nullbyte_san(self): + """ + Test correct handling of CN and SAN with NULL bytes + + see CVE-2013-4073 + """ + cert = load_certificate(FILETYPE_PEM, nullbyte_san_PEM) + subject = cert.get_subject() + self.assertEqual(subject.CN, 'null.python.org\x00example.org') + issuer = cert.get_issuer() + self.assertEqual(issuer.CN, 'null.python.org\x00example.org') + + ext = cert.get_extension(0) + self.assertEqual(ext.get_short_name(), b('basicConstraints')) + + ext = cert.get_extension(1) + self.assertEqual(ext.get_short_name(), b('subjectKeyIdentifier')) + + ext = cert.get_extension(2) + self.assertEqual(ext.get_short_name(), b('keyUsage')) + + ext = cert.get_extension(3) + self.assertEqual(ext.get_short_name(), b('subjectAltName')) + self.assertEqual(str(ext), + 'DNS:altnull.python.org\x00example.com, ' + 'email:null@python.org\x00user@example.org, ' + 'URI:http://null.python.org\x00http://example.org, ' + 'IP Address:192.0.2.1, IP Address:2001:DB8:0:0:0:0:0:1\n') def test_invalid_digest_algorithm(self): """ -- cgit v1.2.1 From de1a6825575f89b896b71156d6dfe14964184326 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 23 Aug 2013 15:21:30 -0400 Subject: CVE-2013-4073 is sort of related, but it doesn\t really explain what's being fixed here. --- ChangeLog | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index 94d852e..6d71417 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,7 +1,8 @@ 2013-08-11 Christian Heimes * OpenSSL/crypto/x509ext.c: Fix handling of NULL bytes inside - subjectAltName general names, CVE-2013-4073. + subjectAltName general names when formatting an X509 extension + as a string. * OpenSSL/crypto/x509.c: Fix memory leak in get_extension(). 2011-08-14 Jean-Paul Calderone -- cgit v1.2.1 From 9ceb2f15c20ad49179b44997ed188db0e0879c98 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 23 Aug 2013 15:23:21 -0400 Subject: again --- OpenSSL/crypto/x509ext.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/OpenSSL/crypto/x509ext.c b/OpenSSL/crypto/x509ext.c index caa78b0..e075ae7 100644 --- a/OpenSSL/crypto/x509ext.c +++ b/OpenSSL/crypto/x509ext.c @@ -237,8 +237,10 @@ crypto_X509Extension_dealloc(crypto_X509ExtensionObj *self) } -/* Special handling of subjectAltName, see CVE-2013-4073 */ - +/* Special handling of subjectAltName. OpenSSL's builtin formatter, + * X509V3_EXT_print, mishandles NUL bytes allowing a truncated display that + * does not accurately reflect what's in the extension. + */ int crypto_X509Extension_str_san(crypto_X509ExtensionObj *self, BIO *bio) { -- cgit v1.2.1 From 4bf75c670d73568f71c7399583824da5c0f225d7 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 23 Aug 2013 15:39:53 -0400 Subject: Split this big test up into the two important pieces and make a test for each --- OpenSSL/test/test_crypto.py | 50 +++++++++++++++++++++++---------------------- 1 file changed, 26 insertions(+), 24 deletions(-) diff --git a/OpenSSL/test/test_crypto.py b/OpenSSL/test/test_crypto.py index e65917e..8ac80c0 100644 --- a/OpenSSL/test/test_crypto.py +++ b/OpenSSL/test/test_crypto.py @@ -267,7 +267,7 @@ oolb6NMg/R3enNPvS1O4UU1H8wpaF77L4yiSWlE0p4w= # certificate with NULL bytes in subjectAltName and common name -nullbyte_san_PEM = b("""-----BEGIN CERTIFICATE----- +nulbyteSubjectAltNamePEM = b("""-----BEGIN CERTIFICATE----- MIIE2DCCA8CgAwIBAgIBADANBgkqhkiG9w0BAQUFADCBxTELMAkGA1UEBhMCVVMx DzANBgNVBAgMBk9yZWdvbjESMBAGA1UEBwwJQmVhdmVydG9uMSMwIQYDVQQKDBpQ eXRob24gU29mdHdhcmUgRm91bmRhdGlvbjEgMB4GA1UECwwXUHl0aG9uIENvcmUg @@ -887,6 +887,18 @@ class X509NameTests(TestCase): [(b("CN"), b("foo")), (b("OU"), b("bar"))]) + def test_load_nul_byte_attribute(self): + """ + An L{X509Name} from an L{X509} instance loaded from a file can have a + NUL byte in the value of one of its attributes. + """ + cert = load_certificate(FILETYPE_PEM, nulbyteSubjectAltNamePEM) + subject = cert.get_subject() + self.assertEqual( + b("null.python.org\x00example.org"), subject.commonName) + + + class _PKeyInteractionTestsMixin: """ Tests which involve another thing and a PKey. @@ -1412,34 +1424,24 @@ WpOdIpB8KksUTCzV591Nr1wd self.assertRaises(IndexError, cert.get_extension, 4) self.assertRaises(TypeError, cert.get_extension, "hello") - def test_nullbyte_san(self): - """ - Test correct handling of CN and SAN with NULL bytes - see CVE-2013-4073 + def test_nullbyte_subjectAltName(self): """ - cert = load_certificate(FILETYPE_PEM, nullbyte_san_PEM) - subject = cert.get_subject() - self.assertEqual(subject.CN, 'null.python.org\x00example.org') - issuer = cert.get_issuer() - self.assertEqual(issuer.CN, 'null.python.org\x00example.org') - - ext = cert.get_extension(0) - self.assertEqual(ext.get_short_name(), b('basicConstraints')) - - ext = cert.get_extension(1) - self.assertEqual(ext.get_short_name(), b('subjectKeyIdentifier')) - - ext = cert.get_extension(2) - self.assertEqual(ext.get_short_name(), b('keyUsage')) + The fields of a I{subjectAltName} extension on an X509 may contain NUL + bytes and this value is reflected in the string representation of the + extension object. + """ + cert = load_certificate(FILETYPE_PEM, nulbyteSubjectAltNamePEM) ext = cert.get_extension(3) self.assertEqual(ext.get_short_name(), b('subjectAltName')) - self.assertEqual(str(ext), - 'DNS:altnull.python.org\x00example.com, ' - 'email:null@python.org\x00user@example.org, ' - 'URI:http://null.python.org\x00http://example.org, ' - 'IP Address:192.0.2.1, IP Address:2001:DB8:0:0:0:0:0:1\n') + self.assertEqual( + b("DNS:altnull.python.org\x00example.com, " + "email:null@python.org\x00user@example.org, " + "URI:http://null.python.org\x00http://example.org, " + "IP Address:192.0.2.1, IP Address:2001:DB8:0:0:0:0:0:1\n"), + b(str(ext))) + def test_invalid_digest_algorithm(self): """ -- cgit v1.2.1 From 30f80c95a322dd48479507519608b473618f4972 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 23 Aug 2013 15:40:59 -0400 Subject: Some formatting and indentation fixes; expand "san" to "subjectAltName". --- OpenSSL/crypto/x509ext.c | 72 +++++++++++++++++++++++------------------------- 1 file changed, 35 insertions(+), 37 deletions(-) diff --git a/OpenSSL/crypto/x509ext.c b/OpenSSL/crypto/x509ext.c index e075ae7..1eedf39 100644 --- a/OpenSSL/crypto/x509ext.c +++ b/OpenSSL/crypto/x509ext.c @@ -242,8 +242,7 @@ crypto_X509Extension_dealloc(crypto_X509ExtensionObj *self) * does not accurately reflect what's in the extension. */ int -crypto_X509Extension_str_san(crypto_X509ExtensionObj *self, BIO *bio) -{ +crypto_X509Extension_str_subjectAltName(crypto_X509ExtensionObj *self, BIO *bio) { GENERAL_NAMES *names; const X509V3_EXT_METHOD *method = NULL; long i, length, num; @@ -269,38 +268,38 @@ crypto_X509Extension_str_san(crypto_X509ExtensionObj *self, BIO *bio) num = sk_GENERAL_NAME_num(names); for (i = 0; i < num; i++) { - GENERAL_NAME *name; - ASN1_STRING *as; - name = sk_GENERAL_NAME_value(names, i); - switch (name->type) { - case GEN_EMAIL: - BIO_puts(bio, "email:"); - as = name->d.rfc822Name; - BIO_write(bio, ASN1_STRING_data(as), - ASN1_STRING_length(as)); - break; - case GEN_DNS: - BIO_puts(bio, "DNS:"); - as = name->d.dNSName; - BIO_write(bio, ASN1_STRING_data(as), - ASN1_STRING_length(as)); - break; - case GEN_URI: - BIO_puts(bio, "URI:"); - as = name->d.uniformResourceIdentifier; - BIO_write(bio, ASN1_STRING_data(as), - ASN1_STRING_length(as)); - break; - default: - /* use builtin print for GEN_OTHERNAME, GEN_X400, - * GEN_EDIPARTY, GEN_DIRNAME, GEN_IPADD and GEN_RID - */ - GENERAL_NAME_print(bio, name); - } - /* trailing ', ' except for last element */ - if (i < (num - 1)) { - BIO_puts(bio, ", "); - } + GENERAL_NAME *name; + ASN1_STRING *as; + name = sk_GENERAL_NAME_value(names, i); + switch (name->type) { + case GEN_EMAIL: + BIO_puts(bio, "email:"); + as = name->d.rfc822Name; + BIO_write(bio, ASN1_STRING_data(as), + ASN1_STRING_length(as)); + break; + case GEN_DNS: + BIO_puts(bio, "DNS:"); + as = name->d.dNSName; + BIO_write(bio, ASN1_STRING_data(as), + ASN1_STRING_length(as)); + break; + case GEN_URI: + BIO_puts(bio, "URI:"); + as = name->d.uniformResourceIdentifier; + BIO_write(bio, ASN1_STRING_data(as), + ASN1_STRING_length(as)); + break; + default: + /* use builtin print for GEN_OTHERNAME, GEN_X400, + * GEN_EDIPARTY, GEN_DIRNAME, GEN_IPADD and GEN_RID + */ + GENERAL_NAME_print(bio, name); + } + /* trailing ', ' except for last element */ + if (i < (num - 1)) { + BIO_puts(bio, ", "); + } } sk_GENERAL_NAME_pop_free(names, GENERAL_NAME_free); @@ -311,15 +310,14 @@ crypto_X509Extension_str_san(crypto_X509ExtensionObj *self, BIO *bio) * Print a nice text representation of the certificate request. */ static PyObject * -crypto_X509Extension_str(crypto_X509ExtensionObj *self) -{ +crypto_X509Extension_str(crypto_X509ExtensionObj *self) { int str_len; char *tmp_str; PyObject *str; BIO *bio = BIO_new(BIO_s_mem()); if (OBJ_obj2nid(self->x509_extension->object) == NID_subject_alt_name) { - if (crypto_X509Extension_str_san(self, bio) == -1) { + if (crypto_X509Extension_str_subjectAltName(self, bio) == -1) { BIO_free(bio); exception_from_error_queue(crypto_Error); return NULL; -- cgit v1.2.1 From bb04db80e51283ff7f2a9619dd9015c48d8f1af2 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 23 Aug 2013 15:41:38 -0400 Subject: some more formatting fixes --- OpenSSL/crypto/x509ext.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/OpenSSL/crypto/x509ext.c b/OpenSSL/crypto/x509ext.c index 1eedf39..9e0b7d3 100644 --- a/OpenSSL/crypto/x509ext.c +++ b/OpenSSL/crypto/x509ext.c @@ -258,8 +258,7 @@ crypto_X509Extension_str_subjectAltName(crypto_X509ExtensionObj *self, BIO *bio) if (method->it) { names = (GENERAL_NAMES*)(ASN1_item_d2i(NULL, &p, length, ASN1_ITEM_ptr(method->it))); - } - else { + } else { names = (GENERAL_NAMES*)(method->d2i(NULL, &p, length)); } if (names == NULL) { @@ -322,9 +321,7 @@ crypto_X509Extension_str(crypto_X509ExtensionObj *self) { exception_from_error_queue(crypto_Error); return NULL; } - } - else if (!X509V3_EXT_print(bio, self->x509_extension, 0, 0)) - { + } else if (!X509V3_EXT_print(bio, self->x509_extension, 0, 0)) { BIO_free(bio); exception_from_error_queue(crypto_Error); return NULL; @@ -343,7 +340,7 @@ PyTypeObject crypto_X509Extension_Type = { "X509Extension", sizeof(crypto_X509ExtensionObj), 0, - (destructor)crypto_X509Extension_dealloc, + (destructor)crypto_X509Extension_dealloc, NULL, /* print */ NULL, /* getattr */ NULL, /* setattr (setattrfunc)crypto_X509Name_setattr, */ -- cgit v1.2.1 From 06754fc64b15c348a0f433e695588cb40e1f3bd4 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 23 Aug 2013 15:47:47 -0400 Subject: in fact, commonName is a native string --- OpenSSL/test/test_crypto.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/OpenSSL/test/test_crypto.py b/OpenSSL/test/test_crypto.py index 8ac80c0..9b49213 100644 --- a/OpenSSL/test/test_crypto.py +++ b/OpenSSL/test/test_crypto.py @@ -895,7 +895,7 @@ class X509NameTests(TestCase): cert = load_certificate(FILETYPE_PEM, nulbyteSubjectAltNamePEM) subject = cert.get_subject() self.assertEqual( - b("null.python.org\x00example.org"), subject.commonName) + "null.python.org\x00example.org", subject.commonName) -- cgit v1.2.1