Bug 1876840 - ignore invalid subject alternative name entries for certificates issued from non-built-in roots r=jschanck

Differential Revision: https://phabricator.services.mozilla.com/D199773

security/certverifier/CertVerifier.cpp

@@ -784,8 +784,31 @@ static bool CertIsSelfSigned(const BackCert& backCert, void* pinarg) {
   return rv == Success;
 }
 
+class SkipInvalidSANsForNonBuiltInRootsPolicy : public NameMatchingPolicy {
+ public:
+  explicit SkipInvalidSANsForNonBuiltInRootsPolicy(bool rootIsBuiltIn)
+      : mRootIsBuiltIn(rootIsBuiltIn) {}
+
+  virtual Result FallBackToCommonName(
+      Time,
+      /*out*/ FallBackToSearchWithinSubject& fallBackToCommonName) override {
+    fallBackToCommonName = FallBackToSearchWithinSubject::No;
+    return Success;
+  }
+
+  virtual HandleInvalidSubjectAlternativeNamesBy
+  HandleInvalidSubjectAlternativeNames() override {
+    return mRootIsBuiltIn ? HandleInvalidSubjectAlternativeNamesBy::Halting
+                          : HandleInvalidSubjectAlternativeNamesBy::Skipping;
+  }
+
+ private:
+  bool mRootIsBuiltIn;
+};
+
 static Result CheckCertHostnameHelper(Input peerCertInput,
-                                      const nsACString& hostname) {
+                                      const nsACString& hostname,
+                                      bool rootIsBuiltIn) {
   Input hostnameInput;
   Result rv = hostnameInput.Init(
       BitwiseCast<const uint8_t*, const char*>(hostname.BeginReading()),
@@ -794,7 +817,8 @@ static Result CheckCertHostnameHelper(Input peerCertInput,
     return Result::FATAL_ERROR_INVALID_ARGS;
   }
 
-  rv = CheckCertHostname(peerCertInput, hostnameInput);
+  SkipInvalidSANsForNonBuiltInRootsPolicy nameMatchingPolicy(rootIsBuiltIn);
+  rv = CheckCertHostname(peerCertInput, hostnameInput, nameMatchingPolicy);
   // Treat malformed name information as a domain mismatch.
   if (rv == Result::ERROR_BAD_DER) {
     return Result::ERROR_BAD_CERT_DOMAIN;
@@ -897,7 +921,8 @@ Result CertVerifier::VerifySSLServerCert(
     if (rv == Result::ERROR_EXPIRED_CERTIFICATE ||
         rv == Result::ERROR_NOT_YET_VALID_CERTIFICATE ||
         rv == Result::ERROR_INVALID_DER_TIME) {
-      Result hostnameResult = CheckCertHostnameHelper(peerCertInput, hostname);
+      Result hostnameResult =
+          CheckCertHostnameHelper(peerCertInput, hostname, false);
       if (hostnameResult != Success) {
         return hostnameResult;
       }
@@ -931,7 +956,8 @@ Result CertVerifier::VerifySSLServerCert(
     }
   }
 
-  rv = CheckCertHostnameHelper(peerCertInput, hostname);
+  rv = CheckCertHostnameHelper(peerCertInput, hostname,
+                               isBuiltChainRootBuiltInRootLocal);
   if (rv != Success) {
     return rv;
   }

security/manager/ssl/tests/unit/bad_certs/ipAddressAsDNSNameInSAN.pem

@@ -1,18 +1,19 @@
 -----BEGIN CERTIFICATE-----
-MIIC0DCCAbigAwIBAgIUN3Rt99rZ7xTMyrgG9lS5nzK4r04wDQYJKoZIhvcNAQEL
+MIIDHTCCAgWgAwIBAgIUeTOq+w44V4g+/ZK2O5Xfytj12egwDQYJKoZIhvcNAQEL
 BQAwEjEQMA4GA1UEAwwHVGVzdCBDQTAiGA8yMDIyMTEyNzAwMDAwMFoYDzIwMjUw
-MjA0MDAwMDAwWjAUMRIwEAYDVQQDDAkxMjcuMC4wLjEwggEiMA0GCSqGSIb3DQEB
+MjA0MDAwMDAwWjA8MTowOAYDVQQDDDFJUCBhZGRyZXNzIGFzIGROU05hbWUgaW4g
-AQUAA4IBDwAwggEKAoIBAQC6iFGoRI4W1kH9braIBjYQPTwT2erkNUq07PVoV2wk
+c3ViamVjdCBhbHRlcm5hdGl2ZSBuYW1lMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8A
-e8HHJajg2B+9sZwGm24ahvJr4q9adWtqZHEIeqVap0WH9xzVJJwCfs1D/B5p0Dgg
+MIIBCgKCAQEAuohRqESOFtZB/W62iAY2ED08E9nq5DVKtOz1aFdsJHvBxyWo4Ngf
-KZOrIMNJ5Nu5TMJrbA7tFYIP8X6taRqx0wI6iypB7qdw4A8Njf1mCyuwJJKkfbmI
+vbGcBptuGobya+KvWnVramRxCHqlWqdFh/cc1SScAn7NQ/weadA4ICmTqyDDSeTb
-YXmQsVeQPdI7xeC4SB+oN9OIQ+8nFthVt2Zaqn4CkC86exCABiTMHGyXrZZhW7fi
+uUzCa2wO7RWCD/F+rWkasdMCOosqQe6ncOAPDY39ZgsrsCSSpH25iGF5kLFXkD3S
-lhLAdTGjDJHdtMr3/K0dJdMJ77kXDqdo4bN7LyJvaeO0ipVhHe4m1iWdq5EITjbL
+O8XguEgfqDfTiEPvJxbYVbdmWqp+ApAvOnsQgAYkzBxsl62WYVu34pYSwHUxowyR
-HCQELL8Wiy/l8Y+ZFzG4s/5JI/pyUcQx1QOs2hgKNe2NAgMBAAGjGDAWMBQGA1Ud
+3bTK9/ytHSXTCe+5Fw6naOGzey8ib2njtIqVYR3uJtYlnauRCE42yxwkBCy/Fosv
-EQQNMAuCCTEyNy4wLjAuMTANBgkqhkiG9w0BAQsFAAOCAQEArpqxJjitFHxHH0Ye
+5fGPmRcxuLP+SSP6clHEMdUDrNoYCjXtjQIDAQABoz0wOzA5BgNVHREEMjAwggkx
-hxmuJ3fs3mzqoI/iQvGYu18k18IqzuZLtfJ6kfQpdqA7qGzxEcgmknaRiL0tuADr
+MjcuMC4wLjGCI2lwQWRkcmVzc0FzRE5TTmFtZUluU0FOLmV4YW1wbGUuY29tMA0G
-l/IUKojIEzXuCunsKN5ZPCB6HSR3TUtO0NmMNSS7kHKe20AtqrNj6asNnxxY2eSv
+CSqGSIb3DQEBCwUAA4IBAQB9zxe9n2/b6xqibQH/LdIgczeL+xxvdAnuq0dkjEgO
-lJ3v8s/FQMirdWUTsrNc4f3P6VErI9IOEq0DJJBr4m/aczhAr2je4/sTDHoC5U44
+UcZx3+qdQXL//Iq+dG3nZoaoSqnQMx6KWvlsoVYIyMHlcFyv5EBf6B8feps9i0J+
-YGbEzJlxmjGVU/mPWwvBTXnTe7wUrSnu4IIUCcKa1yUfjCQUUVJlfNItIFDNIBI2
+YqpuCBp2dGtR4MolxDTKZnk5EopQ/kBckn+qTrOvLCnSy3tfBUvAM67qFW2g1vMG
-ipLYD/KLEjf3CZJyCYOZpI9pIg30KIKAOB3Q3o+vKINz7K5yk6okeZX+kbJ6EKbF
+9kqbZ5cd/ozv3dAW8LYeIKtM2kqDkCQgx7PbbgY2dixqWSyIPEtqOsrAKceJ5Nga
-Pu/77g==
+s1sWdlh0o8b9fpl9O9AzkojqqyX5hcdt5XjpntCQCAwsgp2GOqOkkLx7G9cLrLDk
+QGUd8FuFAwEe1BQVS8uzUYY0vW8LrOYdqhtDq1a9f5cN
 -----END CERTIFICATE-----

security/manager/ssl/tests/unit/bad_certs/ipAddressAsDNSNameInSAN.pem.certspec

@@ -1,3 +1,3 @@
 issuer:Test CA
-subject:127.0.0.1
+subject:IP address as dNSName in subject alternative name
-extension:subjectAlternativeName:127.0.0.1
+extension:subjectAlternativeName:127.0.0.1,ipAddressAsDNSNameInSAN.example.com

security/manager/ssl/tests/unit/head_psm.js

@@ -30,12 +30,12 @@ const { X509 } = ChromeUtils.importESModule(
   "resource://gre/modules/psm/X509.sys.mjs"
 );
 
-const isDebugBuild = Cc["@mozilla.org/xpcom/debug;1"].getService(
+const gIsDebugBuild = Cc["@mozilla.org/xpcom/debug;1"].getService(
   Ci.nsIDebug2
 ).isDebugBuild;
 
 // The test EV roots are only enabled in debug builds as a security measure.
-const gEVExpected = isDebugBuild;
+const gEVExpected = gIsDebugBuild;
 
 const CLIENT_AUTH_FILE_NAME = "ClientAuthRememberList.bin";
 const SSS_STATE_FILE_NAME = "SiteSecurityServiceState.bin";

security/manager/ssl/tests/unit/test_cert_overrides.js

@@ -62,7 +62,7 @@ function check_telemetry() {
   );
   equal(
     histogram.values[9],
-    9,
+    gIsDebugBuild ? 9 : 8,
     "Actual and expected SSL_ERROR_BAD_CERT_DOMAIN values should match"
   );
   equal(
@@ -126,7 +126,7 @@ function check_telemetry() {
   );
   equal(
     keySizeHistogram.values[1],
-    16,
+    gIsDebugBuild ? 17 : 15,
     "Actual and expected successful verifications of 2048-bit keys should match"
   );
   equal(
@@ -400,10 +400,36 @@ function add_simple_tests() {
     MOZILLA_PKIX_ERROR_INADEQUATE_KEY_SIZE
   );
 
-  add_cert_override_test(
+  // The test root is not a built-in (by default), so the invalid dNSName entry
+  // in the subject alternative name extension is skipped.
+  add_connection_test(
     "ipAddressAsDNSNameInSAN.example.com",
-    SSL_ERROR_BAD_CERT_DOMAIN
+    PRErrorCodeSuccess
   );
+
+  if (gIsDebugBuild) {
+    // Treat the test root like a built-in.
+    add_test(function () {
+      let rootCert = constructCertFromFile("bad_certs/test-ca.pem");
+      Services.prefs.setCharPref(
+        "security.test.built_in_root_hash",
+        rootCert.sha256Fingerprint
+      );
+      run_next_test();
+    });
+    // If the root is a built-in, the invalid dNSName entry in the subject
+    // alternative name extension is not skipped, and this result in an error.
+    add_cert_override_test(
+      "ipAddressAsDNSNameInSAN.example.com",
+      SSL_ERROR_BAD_CERT_DOMAIN
+    );
+    // Reset the test root's built-in status.
+    add_test(function () {
+      Services.prefs.clearUserPref("security.test.built_in_root_hash");
+      run_next_test();
+    });
+  }
+
   add_cert_override_test("noValidNames.example.com", SSL_ERROR_BAD_CERT_DOMAIN);
   add_cert_override_test(
     "badSubjectAltNames.example.com",