diff --git a/security/certverifier/CertVerifier.cpp b/security/certverifier/CertVerifier.cpp index 5b2ba85c3b99..8078b3e1c733 100644 --- a/security/certverifier/CertVerifier.cpp +++ b/security/certverifier/CertVerifier.cpp @@ -418,6 +418,29 @@ CertVerifier::VerifyCert(CERTCertificate* cert, SECCertificateUsage usage, } if (rv != Success) { + if (rv != Result::ERROR_KEY_PINNING_FAILURE) { + ScopedCERTCertificate certCopy(CERT_DupCertificate(cert)); + if (!certCopy) { + return SECFailure; + } + ScopedCERTCertList certList(CERT_NewCertList()); + if (!certList) { + return SECFailure; + } + SECStatus srv = CERT_AddCertToListTail(certList.get(), certCopy.get()); + if (srv != SECSuccess) { + return SECFailure; + } + certCopy.forget(); // now owned by certList + PRBool chainOK = false; + srv = chainValidationCallback(&callbackState, certList, &chainOK); + if (srv != SECSuccess) { + return SECFailure; + } + if (!chainOK) { + rv = Result::ERROR_KEY_PINNING_FAILURE; + } + } PR_SetError(MapResultToPRErrorCode(rv), 0); return SECFailure; } diff --git a/security/manager/ssl/tests/unit/test_pinning.js b/security/manager/ssl/tests/unit/test_pinning.js index 275cdbe07504..dc779ddaee4d 100644 --- a/security/manager/ssl/tests/unit/test_pinning.js +++ b/security/manager/ssl/tests/unit/test_pinning.js @@ -37,6 +37,12 @@ function test_strict() { run_next_test(); }); + // If a host should be pinned but other errors (particularly overridable + // errors) like 'unknown issuer' are encountered, the pinning error takes + // precedence. This prevents overrides for such hosts. + add_connection_test("unknownissuer.include-subdomains.pinning.example.com", + getXPCOMStatusFromNSS(MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE)); + // Issued by otherCA, which is not in the pinset for pinning.example.com. add_connection_test("bad.include-subdomains.pinning.example.com", getXPCOMStatusFromNSS(MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE)); @@ -68,6 +74,9 @@ function test_mitm() { add_connection_test("include-subdomains.pinning.example.com", Cr.NS_OK); add_connection_test("good.include-subdomains.pinning.example.com", Cr.NS_OK); + add_connection_test("unknownissuer.include-subdomains.pinning.example.com", + getXPCOMStatusFromNSS(SEC_ERROR_UNKNOWN_ISSUER)); + // In this case, even though otherCA is not in the pinset, it is a // user-specified trust anchor and the pinning check succeeds. add_connection_test("bad.include-subdomains.pinning.example.com", Cr.NS_OK); @@ -90,6 +99,9 @@ function test_disabled() { add_connection_test("exclude-subdomains.pinning.example.com", Cr.NS_OK); add_connection_test("sub.exclude-subdomains.pinning.example.com", Cr.NS_OK); add_connection_test("test-mode.pinning.example.com", Cr.NS_OK); + + add_connection_test("unknownissuer.include-subdomains.pinning.example.com", + getXPCOMStatusFromNSS(SEC_ERROR_UNKNOWN_ISSUER)); } function test_enforce_test_mode() { @@ -99,6 +111,9 @@ function test_enforce_test_mode() { run_next_test(); }); + add_connection_test("unknownissuer.include-subdomains.pinning.example.com", + getXPCOMStatusFromNSS(MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE)); + // Issued by otherCA, which is not in the pinset for pinning.example.com. add_connection_test("bad.include-subdomains.pinning.example.com", getXPCOMStatusFromNSS(MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE)); @@ -128,7 +143,7 @@ function check_pinning_telemetry() { .snapshot(); // Because all of our test domains are pinned to user-specified trust // anchors, effectively only strict mode and enforce test-mode get evaluated - do_check_eq(prod_histogram.counts[0], 2); // Failure count + do_check_eq(prod_histogram.counts[0], 4); // Failure count do_check_eq(prod_histogram.counts[1], 4); // Success count do_check_eq(test_histogram.counts[0], 2); // Failure count do_check_eq(test_histogram.counts[1], 0); // Success count diff --git a/security/manager/ssl/tests/unit/tlsserver/cmd/BadCertServer.cpp b/security/manager/ssl/tests/unit/tlsserver/cmd/BadCertServer.cpp index b9b7f104a76c..9e4135d91fe0 100644 --- a/security/manager/ssl/tests/unit/tlsserver/cmd/BadCertServer.cpp +++ b/security/manager/ssl/tests/unit/tlsserver/cmd/BadCertServer.cpp @@ -55,6 +55,7 @@ const BadCertHost sBadCertHosts[] = { "exclude-subdomains.pinning.example.com", "localhostAndExampleCom" }, { "sub.exclude-subdomains.pinning.example.com", "otherIssuerEE" }, { "test-mode.pinning.example.com", "otherIssuerEE" }, + { "unknownissuer.include-subdomains.pinning.example.com", "unknownissuer" }, { "nsCertTypeNotCritical.example.com", "nsCertTypeNotCritical" }, { "nsCertTypeCriticalWithExtKeyUsage.example.com", "nsCertTypeCriticalWithExtKeyUsage" }, { "nsCertTypeCritical.example.com", "nsCertTypeCritical" },