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>
3 years ago · c3c540b9a4
parent 2042972e84
commit c3c540b9a4
3 changed files with 71 additions and 54 deletions
--- a/crypto/x509v3/v3_utl.c
+++ b/crypto/x509v3/v3_utl.c
@ -821,7 +821,6 @@ static int wildcard_match(const unsigned char *prefix, size_t prefix_len,
    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 @@ static int wildcard_match(const unsigned char *prefix, size_t prefix_len,
        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 @@ static int wildcard_match(const unsigned char *prefix, size_t prefix_len,
        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 @@ static const unsigned char *valid_star(const unsigned char *p, size_t len,
             */
            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;
--- a/include/openssl/x509v3.h
+++ b/include/openssl/x509v3.h
@ -890,10 +890,12 @@ OPENSSL_EXPORT STACK_OF(OPENSSL_STRING) *X509_get1_ocsp(X509 *x);
 #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.
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@ -8012,76 +8012,99 @@ TEST(SSLTest, PermuteExtensions) {
 }

 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,