From: Max Fillinger <[email protected]> In pre-2.7 versions, this option was not available in the Mbed TLS build. This was changed in 2.7, but the option did not do anything when Mbed TLS was selected as the crypto library. Regardless of the field chosen with --x509-username-field, OpenVPN would always extract the CN as username.
This could lead to a situation where an unintended certificate gets accepted by OpenVPN: If we run with "--x509-username-field serialNumber" and "--verify-x509-name 0x05 name", OpenVPN would accept a certificate with CN=0x05 and an incorrect serial number, while a certificate with the correct serial number would be rejected. (Though note that to exploit this, an attacker needs to make the CA sign a certificate with a hexadecimal number in the CN.) This commit adds code to backend_x509_get_username to extract the correct field values from X509 certificates. It also changes the behavior of the function to match the OpenSSL version when the output buffer is too small. (With Mbed TLS, the function would silently truncate the output and return SUCCESS.) It also adds unit tests for extracting the values of different fields. Despite this commit fixing a CVE, we have decided not to keep it under embargo until the release of the next version because it is unlikely to be exploitable in practice: Someone has to run OpenVPN 2.7 with Mbed TLS, use the --x509-username-field option even though it didn't exist in Mbed TLS builds of earlier versions, not notice that the intended certificate is *not* accepted, and then an attacker has to get the CA to sign a certificate with a weird common name. CVE: 2026-63650 Github: openvpn/openvpn-private-issues#144 Reported-By: Hcamael Reported-By: 章鱼哥 (www.aipyaipy.com) Change-Id: Ic183f1f1f90561454b7b1128c95255368427cc4a Signed-off-by: Max Fillinger <[email protected]> Acked-by: Frank Lichtenheld <[email protected]> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1801 --- This change was reviewed on Gerrit and approved by at least one developer. I request to merge it to master. Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1801 This mail reflects revision 3 of this Change. Acked-by according to Gerrit (reflected above): Frank Lichtenheld <[email protected]> diff --git a/src/openvpn/ssl_verify_mbedtls.c b/src/openvpn/ssl_verify_mbedtls.c index ad5479c..2a09e86 100644 --- a/src/openvpn/ssl_verify_mbedtls.c +++ b/src/openvpn/ssl_verify_mbedtls.c @@ -137,19 +137,133 @@ return false; } +static const char * +fieldname_to_oid(const char *fieldname) +{ + if (strcmp(fieldname, "C") == 0) + { + return MBEDTLS_OID_AT_COUNTRY; + } + else if (strcmp(fieldname, "ST") == 0) + { + return MBEDTLS_OID_AT_STATE; + } + else if (strcmp(fieldname, "LOCALITY") == 0) + { + return MBEDTLS_OID_AT_LOCALITY; + } + else if (strcmp(fieldname, "O") == 0) + { + return MBEDTLS_OID_AT_ORGANIZATION; + } + else if (strcmp(fieldname, "OU") == 0) + { + return MBEDTLS_OID_AT_ORG_UNIT; + } + else if (strcmp(fieldname, "CN") == 0) + { + return MBEDTLS_OID_AT_CN; + } + else if (strcmp(fieldname, "GN") == 0) + { + return MBEDTLS_OID_AT_GIVEN_NAME; + } + else if (strcmp(fieldname, "SN") == 0) + { + return MBEDTLS_OID_AT_SUR_NAME; + } + else if (strcmp(fieldname, "initials") == 0) + { + return MBEDTLS_OID_AT_INITIALS; + } + else if (strcmp(fieldname, "pseudonym") == 0) + { + return MBEDTLS_OID_AT_PSEUDONYM; + } + else if (strcmp(fieldname, "title") == 0) + { + return MBEDTLS_OID_AT_TITLE; + } + else if (strcmp(fieldname, "generationQualifier") == 0) + { + return MBEDTLS_OID_AT_GENERATION_QUALIFIER; + } + else if (strcmp(fieldname, "postalAddress") == 0) + { + return MBEDTLS_OID_AT_POSTAL_ADDRESS; + } + else if (strcmp(fieldname, "postalCode") == 0) + { + return MBEDTLS_OID_AT_POSTAL_CODE; + } + else if (strcmp(fieldname, "emailAddress") == 0) + { + return MBEDTLS_OID_PKCS9_EMAIL; + } + else if (strcmp(fieldname, "uid") == 0) + { + return MBEDTLS_OID_AT_UNIQUE_IDENTIFIER; + } + else if (strcmp(fieldname, "dnQualifier") == 0) + { + return MBEDTLS_OID_AT_DN_QUALIFIER; + } + else + { + return NULL; + } +} + result_t backend_x509_get_username(char *cn, size_t cn_len, char *x509_username_field, mbedtls_x509_crt *cert) { - mbedtls_x509_name *name; + ASSERT(cn != NULL && cn_len > 0 && cert != NULL); - ASSERT(cn != NULL); + if (x509_username_field == NULL) + { + goto fail; + } - name = &cert->subject; + if (strcmp(x509_username_field, "serialNumber") == 0) + { + if (cn_len < 2) + { + goto fail; + } + cn[0] = '0'; + cn[1] = 'x'; + size_t cn_index = 2; + bool leading_zeros = true; + for (size_t i = 0; i < cert->serial.len; i++) + { + uint8_t serial_byte = cert->serial.p[i]; + if (leading_zeros && serial_byte == 0) + { + continue; + } + leading_zeros = false; + if (cn_index > cn_len - 3) + { + goto fail; + } + snprintf(&cn[cn_index], cn_len - cn_index, "%02X", serial_byte); + cn_index += 2; + } + return SUCCESS; + } - /* Find common name */ + const char *field_oid = fieldname_to_oid(x509_username_field); + if (field_oid == NULL) + { + goto fail; + } + + /* Find field_oid in the subject name. */ + mbedtls_x509_name *name = &cert->subject; while (name != NULL) { - if (0 == memcmp(name->oid.p, MBEDTLS_OID_AT_CN, MBEDTLS_OID_SIZE(MBEDTLS_OID_AT_CN))) + if (strlen(field_oid) == name->oid.len + && 0 == memcmp(name->oid.p, field_oid, name->oid.len)) { break; } @@ -160,22 +274,23 @@ /* Not found, return an error if this is the peer's certificate */ if (name == NULL) { - return FAILURE; + goto fail; } - /* Found, extract CN */ - if (cn_len > name->val.len) + /* Check that we have room in the buffer, including the terminating '/0' byte. */ + if (cn_len <= name->val.len) { - memcpy(cn, name->val.p, name->val.len); - cn[name->val.len] = '\0'; + goto fail; } - else - { - memcpy(cn, name->val.p, cn_len); - cn[cn_len - 1] = '\0'; - } + + memcpy(cn, name->val.p, name->val.len); + cn[name->val.len] = '\0'; return SUCCESS; + +fail: + cn[0] = '\0'; + return FAILURE; } #if MBEDTLS_VERSION_NUMBER >= 0x04000000 diff --git a/tests/unit_tests/openvpn/test_ssl.c b/tests/unit_tests/openvpn/test_ssl.c index 0e9cecf..d473d67 100644 --- a/tests/unit_tests/openvpn/test_ssl.c +++ b/tests/unit_tests/openvpn/test_ssl.c @@ -775,7 +775,7 @@ static openvpn_x509_cert_t * get_certificate(const char *cert_str) { - BIO *in = BIO_new_mem_buf((char *)cert1, -1); + BIO *in = BIO_new_mem_buf((char *)cert_str, -1); assert_non_null(in); X509 *cert = PEM_read_bio_X509(in, NULL, NULL, NULL); assert_non_null(cert); @@ -790,10 +790,37 @@ } #endif +/* Generated with: + * openssl req -x509 -newkey ec -pkeyopt ec_paramgen_curve:secp384r1 -keyout - -noenc -sha256 -days 3650 \ + * -subj '/CN=ovpn-test-secp384r1/O=OpenVPN Unit Test Example Corp./OU=Cert Details Dept.' + * -addext 'subjectAltName=DNS:unittest.example.com' -addext 'extendedKeyUsage=clientAuth' */ +static const char *cert_details_test_cert = + "-----BEGIN CERTIFICATE-----\n" + "MIICkzCCAhqgAwIBAgIUKDsZM+PApGdaD2QF9iYaxoFJAkowCgYIKoZIzj0EAwIw\n" + "ZTEcMBoGA1UEAwwTb3Zwbi10ZXN0LXNlY3AzODRyMTEoMCYGA1UECgwfT3BlblZQ\n" + "TiBVbml0IFRlc3QgRXhhbXBsZSBDb3JwLjEbMBkGA1UECwwSQ2VydCBEZXRhaWxz\n" + "IERlcHQuMB4XDTI2MDcwNzEwNTIxNloXDTM2MDcwNDEwNTIxNlowZTEcMBoGA1UE\n" + "AwwTb3Zwbi10ZXN0LXNlY3AzODRyMTEoMCYGA1UECgwfT3BlblZQTiBVbml0IFRl\n" + "c3QgRXhhbXBsZSBDb3JwLjEbMBkGA1UECwwSQ2VydCBEZXRhaWxzIERlcHQuMHYw\n" + "EAYHKoZIzj0CAQYFK4EEACIDYgAEOlKoQVk+wbBD6V/6kg+/oHfqF0Dq08LlCL+B\n" + "om4RhutG99QDrow251Ps+Ds/7LQYYRA8+hHyEFrmGM+j2o6KhS5K2uA6dIZL4zLK\n" + "vl0NeF2M61Z8tt/IjrFZd+CrEANco4GKMIGHMB0GA1UdDgQWBBSEyG6m+QdWazeg\n" + "0CHN7q0edJlqMjAfBgNVHSMEGDAWgBSEyG6m+QdWazeg0CHN7q0edJlqMjAPBgNV\n" + "HRMBAf8EBTADAQH/MB8GA1UdEQQYMBaCFHVuaXR0ZXN0LmV4YW1wbGUuY29tMBMG\n" + "A1UdJQQMMAoGCCsGAQUFBwMCMAoGCCqGSM49BAMCA2cAMGQCMCyK7aQcyKGW8BWQ\n" + "UOYqbJUJZJcviP6ACgJRzK6pgkqt9gY0E0Tb00Qh6D5dBV5i3wIwCMhSgpVJxDrc\n" + "pRfligoK8bmv4HEgnV6BDeoDYd41WVMpE9u1issQDHY0SnWC7d9q\n" + "-----END CERTIFICATE-----\n"; +const char *const cert_details_cname = "ovpn-test-secp384r1"; +const char *const cert_details_org = "OpenVPN Unit Test Example Corp."; +const char *const cert_details_org_unit = "Cert Details Dept."; +const char *const cert_details_serial_number = "229677570263950905252266749734450551528150860362"; +const char *const cert_details_serial_number_hex = "0x283B1933E3C0A4675A0F6405F6261AC68149024A"; + void crypto_test_print_cert_details(void **state) { - openvpn_x509_cert_t *cert = get_certificate(cert1); + openvpn_x509_cert_t *cert = get_certificate(cert_details_test_cert); struct gc_arena gc = gc_new(); const char *fp = backend_x509_get_serial_hex(cert, &gc); @@ -801,29 +828,57 @@ /* we messed this up between TLS libraries. But let's at least notice in * the future ...*/ #if defined(ENABLE_CRYPTO_MBEDTLS) - assert_string_equal(fp, "82:6B:DD:CC:BD:E5:5E:B7:08:F1:2D:68:00:3C:24:DE"); + assert_string_equal(fp, "28:3B:19:33:E3:C0:A4:67:5A:0F:64:05:F6:26:1A:C6:81:49:02:4A"); #else - assert_string_equal(fp, "82:6b:dd:cc:bd:e5:5e:b7:08:f1:2d:68:00:3c:24:de"); + assert_string_equal(fp, "28:3b:19:33:e3:c0:a4:67:5a:0f:64:05:f6:26:1a:c6:81:49:02:4a"); #endif const char *sn = backend_x509_get_serial(cert, &gc); - assert_string_equal(sn, "173359713849739808110610111821055272158"); + assert_string_equal(sn, cert_details_serial_number); char username[TLS_USERNAME_LEN + 1] = { 0 }; /* null-terminated */ - int ret = backend_x509_get_username(username, sizeof(username), "CN", - cert); + int ret = backend_x509_get_username(username, sizeof(username), "CN", cert); - assert_string_equal(username, "ovpn-test-ec1"); + assert_string_equal(username, cert_details_cname); assert_int_equal(ret, SUCCESS); -#ifndef ENABLE_CRYPTO_MBEDTLS - /* mbed TLS does not implement this */ - ret = backend_x509_get_username(username, sizeof(username), "serialNumber", - cert); + ret = backend_x509_get_username(username, sizeof(username), "serialNumber", cert); assert_int_equal(ret, SUCCESS); - assert_string_equal(username, "0x826BDDCCBDE55EB708F12D68003C24DE"); -#endif + assert_string_equal(username, cert_details_serial_number_hex); + + ret = backend_x509_get_username(username, sizeof(username), "O", cert); + + assert_string_equal(username, cert_details_org); + assert_int_equal(ret, SUCCESS); + + ret = backend_x509_get_username(username, sizeof(username), "OU", cert); + + assert_string_equal(username, cert_details_org_unit); + assert_int_equal(ret, SUCCESS); + + /* Check that FAILURE is returned if a field does not exist. */ + ret = backend_x509_get_username(username, sizeof(username), "SN", cert); + assert_int_equal(ret, FAILURE); + + /* Check that FAILURE is returned for invalid field names. */ + ret = backend_x509_get_username(username, sizeof(username), "invalidField", cert); + assert_int_equal(ret, FAILURE); + + /* Check that FAILURE is returned if the output buffer is too small. Do this separately + * for a subject field and for the serial number, because these are different code paths. + * + * First case: Can't fit all characters. */ + ret = backend_x509_get_username(username, strlen(cert_details_cname) / 2, "CN", cert); + assert_int_equal(ret, FAILURE); + ret = backend_x509_get_username(username, strlen(cert_details_serial_number_hex) / 2, "serialNumber", cert); + assert_int_equal(ret, FAILURE); + + /* Second case: Can fit the characters but not the terminating '\0'. */ + ret = backend_x509_get_username(username, strlen(cert_details_cname), "CN", cert); + assert_int_equal(ret, FAILURE); + ret = backend_x509_get_username(username, strlen(cert_details_serial_number_hex), "serialNumber", cert); + assert_int_equal(ret, FAILURE); gc_free(&gc); free_certificate(cert); _______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
