Disable the common name fallback on *any* SAN list.

This aligns with the Go crypto/x509 behavior and reduces the cases when
the SAN to CN fallback occurs. If the certificate is new enough to have
a SAN list, even if it only contains email or IP addresses, it is
reasonable to assume the certificate is new enough that the common name
is not a DNS name.

Update-Note: Our certificate verification is getting slightly stricter.
Change-Id: I9e3466d8dd8a722405c546181a589f797efa43f9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/35647
Reviewed-by: Adam Langley <agl@google.com>

crypto/x509/x509_test.cc

diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 93bb582..517635e 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -693,6 +693,40 @@
     "pFFXI2B5usgI\n"
     "-----END CERTIFICATE-----\n";
 
+// kCommonNameWithEmailSAN is a leaf certificate signed by kSANTypesRoot, with
+// *.host1.test as the common name and the email address test@host2.test in the
+// SAN list.
+static const char kCommonNameWithEmailSAN[] =
+    "-----BEGIN CERTIFICATE-----\n"
+    "MIIBvDCCASWgAwIBAgIBAjANBgkqhkiG9w0BAQsFADArMRcwFQYDVQQKEw5Cb3Jp\n"
+    "bmdTU0wgVGVzdDEQMA4GA1UEAxMHUm9vdCBDQTAgFw0wMDAxMDEwMDAwMDBaGA8y\n"
+    "MDk5MDEwMTAwMDAwMFowFzEVMBMGA1UEAwwMKi5ob3N0MS50ZXN0MFkwEwYHKoZI\n"
+    "zj0CAQYIKoZIzj0DAQcDQgAEtevOxcTjpPzlNGoUMFfZyr1k03/Hiuh+EsnuScDs\n"
+    "8XLKi6fDkvSaDClI99ycabQZRPIrvyT+dglDC6ugQd+CYqNJMEcwDAYDVR0TAQH/\n"
+    "BAIwADAbBgNVHSMEFDASgBBAN9cB+0AvuBx+VAQnjFkBMBoGA1UdEQQTMBGBD3Rl\n"
+    "c3RAaG9zdDIudGVzdDANBgkqhkiG9w0BAQsFAAOBgQCGbqb78OWJWl4zb+qw0Dz2\n"
+    "HJgZZJt6/+nNG/XJKdaYeS4eofsbwsJI4fuuOF6ZvYCJxVNtGqdfZDgycvFA9hjv\n"
+    "NGosBF1/spP17cmzTahLjxs71jDvHV/EQJbKGl/Zpta1Em1VrzSrwoOFabPXzZTJ\n"
+    "aet/mER21Z/9ZsTUoJQPJw==\n"
+    "-----END CERTIFICATE-----\n";
+
+// kCommonNameWithIPSAN is a leaf certificate signed by kSANTypesRoot, with
+// *.host1.test as the common name and the IP address 127.0.0.1 in the
+// SAN list.
+static const char kCommonNameWithIPSAN[] =
+    "-----BEGIN CERTIFICATE-----\n"
+    "MIIBsTCCARqgAwIBAgIBAjANBgkqhkiG9w0BAQsFADArMRcwFQYDVQQKEw5Cb3Jp\n"
+    "bmdTU0wgVGVzdDEQMA4GA1UEAxMHUm9vdCBDQTAgFw0wMDAxMDEwMDAwMDBaGA8y\n"
+    "MDk5MDEwMTAwMDAwMFowFzEVMBMGA1UEAwwMKi5ob3N0MS50ZXN0MFkwEwYHKoZI\n"
+    "zj0CAQYIKoZIzj0DAQcDQgAEFKrgkxm8PysXbwnHQeTD3p8YY0+sY4ssnZgmj8wX\n"
+    "KTyn893fdBHWlz71GO6t82wMTF5d+ZYwI2XU52pfl4SB2aM+MDwwDAYDVR0TAQH/\n"
+    "BAIwADAbBgNVHSMEFDASgBBAN9cB+0AvuBx+VAQnjFkBMA8GA1UdEQQIMAaHBH8A\n"
+    "AAEwDQYJKoZIhvcNAQELBQADgYEAQWZ8Oj059ZjS109V/ijMYT28xuAN5n6HHxCO\n"
+    "DopTP56Zu9+gme5wTETWEfocspZvgecoUOcedTFoKSQ7JafO09NcVLA+D6ddYpju\n"
+    "mgfuiLy9dDhqvX/NHaLBMxOBWWbOLwWE+ibyX+pOzjWRCw1L7eUXOr6PhZAOQsmU\n"
+    "D0+O6KI=\n"
+    "-----END CERTIFICATE-----\n";
+
 // CertFromPEM parses the given, NUL-terminated pem block and returns an
 // |X509*|.
 static bssl::UniquePtr<X509> CertFromPEM(const char *pem) {
@@ -1785,6 +1819,10 @@
   ASSERT_TRUE(with_sans);
   bssl::UniquePtr<X509> without_sans = CertFromPEM(kCommonNameWithoutSANs);
   ASSERT_TRUE(without_sans);
+  bssl::UniquePtr<X509> with_email = CertFromPEM(kCommonNameWithEmailSAN);
+  ASSERT_TRUE(with_email);
+  bssl::UniquePtr<X509> with_ip = CertFromPEM(kCommonNameWithIPSAN);
+  ASSERT_TRUE(with_ip);
 
   auto verify_cert = [&](X509 *leaf, unsigned flags, const char *host) {
     return Verify(
@@ -1804,6 +1842,10 @@
             verify_cert(with_sans.get(), 0 /* no flags */, "foo.host3.test"));
   EXPECT_EQ(X509_V_OK, verify_cert(without_sans.get(), 0 /* no flags */,
                                    "foo.host1.test"));
+  EXPECT_EQ(X509_V_ERR_HOSTNAME_MISMATCH,
+            verify_cert(with_email.get(), 0 /* no flags */, "foo.host1.test"));
+  EXPECT_EQ(X509_V_ERR_HOSTNAME_MISMATCH,
+            verify_cert(with_ip.get(), 0 /* no flags */, "foo.host1.test"));
 
   // X509_CHECK_FLAG_ALWAYS_CHECK_SUBJECT is ignored.
   EXPECT_EQ(X509_V_ERR_HOSTNAME_MISMATCH,
@@ -1818,6 +1860,12 @@
   EXPECT_EQ(X509_V_OK, verify_cert(without_sans.get(),
                                    X509_CHECK_FLAG_ALWAYS_CHECK_SUBJECT,
                                    "foo.host1.test"));
+  EXPECT_EQ(X509_V_ERR_HOSTNAME_MISMATCH,
+            verify_cert(with_email.get(), X509_CHECK_FLAG_ALWAYS_CHECK_SUBJECT,
+                        "foo.host1.test"));
+  EXPECT_EQ(X509_V_ERR_HOSTNAME_MISMATCH,
+            verify_cert(with_ip.get(), X509_CHECK_FLAG_ALWAYS_CHECK_SUBJECT,
+                        "foo.host1.test"));
 
   // X509_CHECK_FLAG_NEVER_CHECK_SUBJECT implements the correct behavior: the
   // common name is never checked.
@@ -1833,4 +1881,10 @@
   EXPECT_EQ(X509_V_ERR_HOSTNAME_MISMATCH,
             verify_cert(without_sans.get(), X509_CHECK_FLAG_NEVER_CHECK_SUBJECT,
                         "foo.host1.test"));
+  EXPECT_EQ(X509_V_ERR_HOSTNAME_MISMATCH,
+            verify_cert(with_email.get(), X509_CHECK_FLAG_NEVER_CHECK_SUBJECT,
+                        "foo.host1.test"));
+  EXPECT_EQ(X509_V_ERR_HOSTNAME_MISMATCH,
+            verify_cert(with_ip.get(), X509_CHECK_FLAG_NEVER_CHECK_SUBJECT,
+                        "foo.host1.test"));
 }

crypto/x509v3/v3_utl.c

diff --git a/crypto/x509v3/v3_utl.c b/crypto/x509v3/v3_utl.c
index ebda63a..51db478 100644
--- a/crypto/x509v3/v3_utl.c
+++ b/crypto/x509v3/v3_utl.c
@@ -955,7 +955,6 @@
     int j;
     int cnid = NID_undef;
     int alt_type;
-    int san_present = 0;
     int rv = 0;
     equal_fn equal;
 
@@ -988,7 +987,6 @@
             gen = sk_GENERAL_NAME_value(gens, i);
             if (gen->type != check_type)
                 continue;
-            san_present = 1;
             if (check_type == GEN_EMAIL)
                 cstr = gen->d.rfc822Name;
             else if (check_type == GEN_DNS)
@@ -1001,10 +999,7 @@
                 break;
         }
         GENERAL_NAMES_free(gens);
-        if (rv != 0)
-            return rv;
-        if (san_present)
-            return 0;
+        return rv;
     }
 
     /* We're done if CN-ID is not pertinent */