Remove non-standard X.509 DNS wildcard matching. Always enable X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS and never enable X509_CHECK_FLAG_MULTI_LABEL_WILDCARDS. Update-Note: BoringSSL will no longer accept wildcard patterns like *www.example.com or www*.example.com. (It already did not accept ww*w.example.com.) X509_CHECK_FLAG_MULTI_LABEL_WILDCARDS will also be ignored and can no longer be used to allow foo.bar.example.com to match *.example.com. Fixes: 462 Change-Id: I004e087bf70f4c3f249235cd864d9e19cc9a5102 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50705 Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/x509v3/v3_utl.c b/crypto/x509v3/v3_utl.c index 5d91aed..7e7c549 100644 --- a/crypto/x509v3/v3_utl.c +++ b/crypto/x509v3/v3_utl.c
@@ -821,7 +821,6 @@ const unsigned char *wildcard_start; const unsigned char *wildcard_end; const unsigned char *p; - int allow_multi = 0; int allow_idna = 0; if (subject_len < prefix_len + suffix_len) @@ -840,8 +839,6 @@ if (wildcard_start == wildcard_end) return 0; allow_idna = 1; - if (flags & X509_CHECK_FLAG_MULTI_LABEL_WILDCARDS) - allow_multi = 1; } /* IDNA labels cannot match partial wildcards */ if (!allow_idna && @@ -853,14 +850,13 @@ return 1; /* * Check that the part matched by the wildcard contains only - * permitted characters and only matches a single label unless - * allow_multi is set. + * permitted characters and only matches a single label. */ for (p = wildcard_start; p != wildcard_end; ++p) if (!(('0' <= *p && *p <= '9') || ('A' <= *p && *p <= 'Z') || ('a' <= *p && *p <= 'z') || - *p == '-' || (allow_multi && *p == '.'))) + *p == '-')) return 0; return 1; } @@ -892,12 +888,8 @@ */ if (star != NULL || (state & LABEL_IDNA) != 0 || dots) return NULL; - /* Only full-label '*.example.com' wildcards? */ - if ((flags & X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS) - && (!atstart || !atend)) - return NULL; - /* No 'foo*bar' wildcards */ - if (!atstart && !atend) + /* Only full-label '*.example.com' wildcards. */ + if (!atstart || !atend) return NULL; star = &p[i]; state &= ~LABEL_START;
diff --git a/include/openssl/x509v3.h b/include/openssl/x509v3.h index acff637..3e8cf1b 100644 --- a/include/openssl/x509v3.h +++ b/include/openssl/x509v3.h
@@ -890,10 +890,12 @@ #define X509_CHECK_FLAG_ALWAYS_CHECK_SUBJECT 0 // Disable wildcard matching for dnsName fields and common name. #define X509_CHECK_FLAG_NO_WILDCARDS 0x2 -// Wildcards must not match a partial label. -#define X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS 0x4 -// Allow (non-partial) wildcards to match multiple labels. -#define X509_CHECK_FLAG_MULTI_LABEL_WILDCARDS 0x8 +// X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS does nothing, but is necessary in +// OpenSSL to enable standard wildcard matching. In BoringSSL, this behavior is +// always enabled. +#define X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS 0 +// Deprecated: this flag does nothing +#define X509_CHECK_FLAG_MULTI_LABEL_WILDCARDS 0 // Constraint verifier subdomain patterns to match a single labels. #define X509_CHECK_FLAG_SINGLE_LABEL_SUBDOMAINS 0x10 // Skip the subject common name fallback if subjectAltNames is missing.
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index 6ef80cb..d0bd680 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc
@@ -8012,76 +8012,99 @@ } TEST(SSLTest, HostMatching) { - static const char kCertPEM[] = - "-----BEGIN CERTIFICATE-----\n" - "MIIB9jCCAZ2gAwIBAgIQeudG9R61BOxUvWkeVhU5DTAKBggqhkjOPQQDAjApMRAw\n" - "DgYDVQQKEwdBY21lIENvMRUwEwYDVQQDEwxleGFtcGxlMy5jb20wHhcNMjExMjA2\n" - "MjA1NjU2WhcNMjIxMjA2MjA1NjU2WjApMRAwDgYDVQQKEwdBY21lIENvMRUwEwYD\n" - "VQQDEwxleGFtcGxlMy5jb20wWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAAS7l2VO\n" - "Bl2TjVm9WfGk24+hMbVFUNB+RVHWbCvFvNZAoWiIJ2z34RLGInyZvCZ8xLAvsuaW\n" - "ULDDaoeDl1M0t4Hmo4GmMIGjMA4GA1UdDwEB/wQEAwIChDATBgNVHSUEDDAKBggr\n" - "BgEFBQcDATAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBTTJWurcc1t+VPQBko3\n" - "Gsw6cbcWSTBMBgNVHREERTBDggxleGFtcGxlMS5jb22CDGV4YW1wbGUyLmNvbYIP\n" - "YSouZXhhbXBsZTQuY29tgg4qLmV4YW1wbGU1LmNvbYcEAQIDBDAKBggqhkjOPQQD\n" - "AgNHADBEAiAAv0ljHJGrgyzZDkG6XvNZ5ewxRfnXcZuD0Y7E4giCZgIgNK1qjilu\n" - "5DyVbfKeeJhOCtGxqE1dWLXyJBnoRomSYBY=\n" - "-----END CERTIFICATE-----\n"; + static const char kCertPEM[] = R"( +-----BEGIN CERTIFICATE----- +MIIB9jCCAZ2gAwIBAgIQeudG9R61BOxUvWkeVhU5DTAKBggqhkjOPQQDAjApMRAw +DgYDVQQKEwdBY21lIENvMRUwEwYDVQQDEwxleGFtcGxlMy5jb20wHhcNMjExMjA2 +MjA1NjU2WhcNMjIxMjA2MjA1NjU2WjApMRAwDgYDVQQKEwdBY21lIENvMRUwEwYD +VQQDEwxleGFtcGxlMy5jb20wWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAAS7l2VO +Bl2TjVm9WfGk24+hMbVFUNB+RVHWbCvFvNZAoWiIJ2z34RLGInyZvCZ8xLAvsuaW +ULDDaoeDl1M0t4Hmo4GmMIGjMA4GA1UdDwEB/wQEAwIChDATBgNVHSUEDDAKBggr +BgEFBQcDATAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBTTJWurcc1t+VPQBko3 +Gsw6cbcWSTBMBgNVHREERTBDggxleGFtcGxlMS5jb22CDGV4YW1wbGUyLmNvbYIP +YSouZXhhbXBsZTQuY29tgg4qLmV4YW1wbGU1LmNvbYcEAQIDBDAKBggqhkjOPQQD +AgNHADBEAiAAv0ljHJGrgyzZDkG6XvNZ5ewxRfnXcZuD0Y7E4giCZgIgNK1qjilu +5DyVbfKeeJhOCtGxqE1dWLXyJBnoRomSYBY= +-----END CERTIFICATE----- +)"; bssl::UniquePtr<X509> cert(CertFromPEM(kCertPEM)); ASSERT_TRUE(cert); + static const char kCertNoSANsPEM[] = R"( +-----BEGIN CERTIFICATE----- +MIIBqzCCAVGgAwIBAgIQeudG9R61BOxUvWkeVhU5DTAKBggqhkjOPQQDAjArMRIw +EAYDVQQKEwlBY21lIENvIDIxFTATBgNVBAMTDGV4YW1wbGUzLmNvbTAeFw0yMTEy +MDYyMDU2NTZaFw0yMjEyMDYyMDU2NTZaMCsxEjAQBgNVBAoTCUFjbWUgQ28gMjEV +MBMGA1UEAxMMZXhhbXBsZTMuY29tMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE +u5dlTgZdk41ZvVnxpNuPoTG1RVDQfkVR1mwrxbzWQKFoiCds9+ESxiJ8mbwmfMSw +L7LmllCww2qHg5dTNLeB5qNXMFUwDgYDVR0PAQH/BAQDAgKEMBMGA1UdJQQMMAoG +CCsGAQUFBwMBMA8GA1UdEwEB/wQFMAMBAf8wHQYDVR0OBBYEFNMla6txzW35U9AG +SjcazDpxtxZJMAoGCCqGSM49BAMCA0gAMEUCIG3YWGWtpVhbcGV7wFKQwTfmvwHW +pw4qCFZlool4hCwsAiEA+2fc6NfSbNpFEtQkDOMJW2ANiScAVEmImNqPfb2klz4= +-----END CERTIFICATE----- +)"; + bssl::UniquePtr<X509> cert_no_sans(CertFromPEM(kCertNoSANsPEM)); + ASSERT_TRUE(cert_no_sans); - static const char kKeyPEM[] = - "-----BEGIN PRIVATE KEY-----\n" - "MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQghsaSZhUzZAcQlLyJ\n" - "MDuy7WPdyqNsAX9rmEP650LF/q2hRANCAAS7l2VOBl2TjVm9WfGk24+hMbVFUNB+\n" - "RVHWbCvFvNZAoWiIJ2z34RLGInyZvCZ8xLAvsuaWULDDaoeDl1M0t4Hm\n" - "-----END PRIVATE KEY-----\n"; + static const char kKeyPEM[] = R"( +-----BEGIN PRIVATE KEY----- +MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQghsaSZhUzZAcQlLyJ +MDuy7WPdyqNsAX9rmEP650LF/q2hRANCAAS7l2VOBl2TjVm9WfGk24+hMbVFUNB+ +RVHWbCvFvNZAoWiIJ2z34RLGInyZvCZ8xLAvsuaWULDDaoeDl1M0t4Hm +-----END PRIVATE KEY----- +)"; bssl::UniquePtr<EVP_PKEY> key(KeyFromPEM(kKeyPEM)); ASSERT_TRUE(key); - bssl::UniquePtr<SSL_CTX> server_ctx(SSL_CTX_new(TLS_method())); - ASSERT_TRUE(server_ctx); - ASSERT_TRUE(SSL_CTX_use_certificate(server_ctx.get(), cert.get())); - ASSERT_TRUE(SSL_CTX_use_PrivateKey(server_ctx.get(), key.get())); - bssl::UniquePtr<SSL_CTX> client_ctx(SSL_CTX_new(TLS_method())); ASSERT_TRUE(client_ctx); ASSERT_TRUE(X509_STORE_add_cert(SSL_CTX_get_cert_store(client_ctx.get()), cert.get())); + ASSERT_TRUE(X509_STORE_add_cert(SSL_CTX_get_cert_store(client_ctx.get()), + cert_no_sans.get())); SSL_CTX_set_verify(client_ctx.get(), SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, nullptr); struct TestCase { + X509 *cert; std::string hostname; unsigned flags; bool should_match; }; std::vector<TestCase> kTests = { // These two names are present as SANs in the certificate. - {"example1.com", 0, true}, - {"example2.com", 0, true}, + {cert.get(), "example1.com", 0, true}, + {cert.get(), "example2.com", 0, true}, // This is the CN of the certificate, but that shouldn't matter if a SAN // extension is present. - {"example3.com", 0, false}, - // a*.example4.com is a SAN, which is invalid because partial wildcards - // aren't a thing except for the OpenSSL verifier. - {"abc.example4.com", 0, true}, - // ... but they can be turned off. - {"abc.example4.com", X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS, false}, + {cert.get(), "example3.com", 0, false}, + // If the SAN is not present, we, for now, look for DNS names in the CN. + {cert_no_sans.get(), "example3.com", 0, true}, + // ... but this can be turned off. + {cert_no_sans.get(), "example3.com", X509_CHECK_FLAG_NEVER_CHECK_SUBJECT, + false}, + // a*.example4.com is a SAN, but is invalid. + {cert.get(), "abc.example4.com", 0, false}, // *.example5.com is a SAN in the certificate, which is a normal and valid // wildcard. - {"abc.example5.com", 0, true}, + {cert.get(), "abc.example5.com", 0, true}, // This name is not present. - {"notexample1.com", 0, false}, + {cert.get(), "notexample1.com", 0, false}, // The IPv4 address 1.2.3.4 is a SAN, but that shouldn't match against a // hostname that happens to be its textual representation. - {"1.2.3.4", 0, false}, + {cert.get(), "1.2.3.4", 0, false}, }; - bssl::UniquePtr<SSL> client, server; - ClientConfig config; for (const TestCase &test : kTests) { SCOPED_TRACE(test.hostname); + + bssl::UniquePtr<SSL_CTX> server_ctx(SSL_CTX_new(TLS_method())); + ASSERT_TRUE(server_ctx); + ASSERT_TRUE(SSL_CTX_use_certificate(server_ctx.get(), test.cert)); + ASSERT_TRUE(SSL_CTX_use_PrivateKey(server_ctx.get(), key.get())); + + ClientConfig config; + bssl::UniquePtr<SSL> client, server; config.verify_hostname = test.hostname; config.hostflags = test.flags; EXPECT_EQ(test.should_match,