diff --git a/browser/base/content/test/about/browser.ini b/browser/base/content/test/about/browser.ini index f1d6dfb2b413..4326ba1748c5 100644 --- a/browser/base/content/test/about/browser.ini +++ b/browser/base/content/test/about/browser.ini @@ -12,7 +12,6 @@ support-files = [browser_aboutCertError_clockSkew.js] [browser_aboutCertError_exception.js] [browser_aboutCertError_mitm.js] -[browser_aboutCertError_multiple_errors.js] [browser_aboutCertError_noSubjectAltName.js] [browser_aboutCertError_offlineSupport.js] [browser_aboutCertError_telemetry.js] diff --git a/browser/base/content/test/about/browser_aboutCertError_exception.js b/browser/base/content/test/about/browser_aboutCertError_exception.js index c891c5ea3460..a0a2de401d67 100644 --- a/browser/base/content/test/about/browser_aboutCertError_exception.js +++ b/browser/base/content/test/about/browser_aboutCertError_exception.js @@ -91,7 +91,6 @@ add_task(async function checkPermanentExceptionPref() { -1, {}, cert, - {}, isTemporary ); ok(hasException, "Has stored an exception for the page."); diff --git a/browser/base/content/test/about/browser_aboutCertError_multiple_errors.js b/browser/base/content/test/about/browser_aboutCertError_multiple_errors.js deleted file mode 100644 index 4460b52b3e37..000000000000 --- a/browser/base/content/test/about/browser_aboutCertError_multiple_errors.js +++ /dev/null @@ -1,153 +0,0 @@ -/* Any copyright is dedicated to the Public Domain. - http://creativecommons.org/publicdomain/zero/1.0/ */ - -"use strict"; - -const EXPIRED_CERT = "https://expired.example.com/"; -const BAD_CERT = "https://mismatch.badcertdomain.example.com/"; - -const kErrors = [ - "MOZILLA_PKIX_ERROR_MITM_DETECTED", - "SEC_ERROR_UNKNOWN_ISSUER", - "SEC_ERROR_CA_CERT_INVALID", - "SEC_ERROR_UNTRUSTED_ISSUER", - "SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED", - "SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE", - "MOZILLA_PKIX_ERROR_SELF_SIGNED_CERT", - "MOZILLA_PKIX_ERROR_ADDITIONAL_POLICY_CONSTRAINT_FAILED", -]; - -/** - * This is a quasi-unit test style test to check what happens - * when we encounter certificates with multiple problems. - * - * We should consistently display the most urgent message first. - */ -add_task(async function test_expired_bad_cert() { - let tab = await openErrorPage(EXPIRED_CERT); - const kExpiryLabel = "cert-error-expired-now"; - let browser = tab.linkedBrowser; - await SpecialPowers.spawn(browser, [kExpiryLabel, kErrors], async function( - knownExpiryLabel, - errorCodes - ) { - await ContentTaskUtils.waitForCondition( - () => - !!content.document.querySelectorAll(`#badCertTechnicalInfo label`) - .length - ); - // First check that for this real cert, which is simply expired and nothing else, - // we show the expiry info: - let rightLabel = content.document.querySelector( - `#badCertTechnicalInfo label[data-l10n-id="${knownExpiryLabel}"]` - ); - ok(rightLabel, "Expected a label with the right contents."); - info(content.document.querySelector("#badCertTechnicalInfo").innerHTML); - - const kExpiredErrors = errorCodes.map(err => { - return Cu.cloneInto( - { - errorCodeString: err, - isUntrusted: true, - isNotValidAtThisTime: true, - validNotBefore: 0, - validNotAfter: 86400000, - }, - content.window - ); - }); - for (let err of kExpiredErrors) { - // Test hack: invoke the content-privileged helper method with the fake cert info. - await Cu.waiveXrays(content.window).setTechnicalDetailsOnCertError(err); - let allLabels = content.document.querySelectorAll( - "#badCertTechnicalInfo label" - ); - ok( - allLabels.length, - "There should be an advanced technical description for " + - err.errorCodeString - ); - for (let label of allLabels) { - let id = label.getAttribute("data-l10n-id"); - ok( - id, - `Label for ${err.errorCodeString} should have data-l10n-id (was: ${id}).` - ); - isnot( - id, - knownExpiryLabel, - `Label for ${err.errorCodeString} should not be about expiry.` - ); - } - } - }); - await BrowserTestUtils.removeTab(tab); -}); - -/** - * The same as above, but now for alt-svc domain mismatch certs. - */ -add_task(async function test_alt_svc_bad_cert() { - let tab = await openErrorPage(BAD_CERT); - const kErrKnownPrefix = "cert-error-domain-mismatch"; - const kErrKnownAlt = "cert-error-domain-mismatch-single-nolink"; - let browser = tab.linkedBrowser; - await SpecialPowers.spawn( - browser, - [kErrKnownAlt, kErrKnownPrefix, kErrors], - async function(knownAlt, knownAltPrefix, errorCodes) { - await ContentTaskUtils.waitForCondition( - () => - !!content.document.querySelectorAll(`#badCertTechnicalInfo label`) - .length - ); - // First check that for this real cert, which is simply for the wrong domain and nothing else, - // we show the alt-svc info: - let rightLabel = content.document.querySelector( - `#badCertTechnicalInfo label[data-l10n-id="${knownAlt}"]` - ); - ok(rightLabel, "Expected a label with the right contents."); - info(content.document.querySelector("#badCertTechnicalInfo").innerHTML); - - const kAltSvcErrors = errorCodes.map(err => { - return Cu.cloneInto( - { - errorCodeString: err, - isUntrusted: true, - isDomainMismatch: true, - }, - content.window - ); - }); - for (let err of kAltSvcErrors) { - // Test hack: invoke the content-privileged helper method with the fake cert info. - await Cu.waiveXrays(content.window).setTechnicalDetailsOnCertError(err); - let allLabels = content.document.querySelectorAll( - "#badCertTechnicalInfo label" - ); - ok( - allLabels.length, - "There should be an advanced technical description for " + - err.errorCodeString - ); - for (let label of allLabels) { - let id = label.getAttribute("data-l10n-id"); - ok( - id, - `Label for ${err.errorCodeString} should have data-l10n-id (was: ${id}).` - ); - isnot( - id, - knownAlt, - `Label for ${err.errorCodeString} should not mention other domains.` - ); - ok( - !id.startsWith(knownAltPrefix), - `Label shouldn't start with ${knownAltPrefix}` - ); - } - } - } - ); - await BrowserTestUtils.removeTab(tab); -}); diff --git a/browser/base/content/test/siteIdentity/browser_navigation_failures.js b/browser/base/content/test/siteIdentity/browser_navigation_failures.js index c9a78861fd15..2d0af8258a6c 100644 --- a/browser/base/content/test/siteIdentity/browser_navigation_failures.js +++ b/browser/base/content/test/siteIdentity/browser_navigation_failures.js @@ -129,15 +129,11 @@ add_task(async function() { let cert = getTestServerCertificate(); // Start a server and trust its certificate. let server = startServer(cert); - let overrideBits = - Ci.nsICertOverrideService.ERROR_UNTRUSTED | - Ci.nsICertOverrideService.ERROR_MISMATCH; certOverrideService.rememberValidityOverride( "localhost", server.port, {}, cert, - overrideBits, true ); diff --git a/browser/base/content/test/siteIdentity/browser_secure_transport_insecure_scheme.js b/browser/base/content/test/siteIdentity/browser_secure_transport_insecure_scheme.js index 4a1d5c3a857a..8899f7355c6c 100644 --- a/browser/base/content/test/siteIdentity/browser_secure_transport_insecure_scheme.js +++ b/browser/base/content/test/siteIdentity/browser_secure_transport_insecure_scheme.js @@ -134,15 +134,11 @@ add_task(async function() { let cert = getTestServerCertificate(); // Start the proxy and configure Firefox to trust its certificate. let server = startServer(cert); - let overrideBits = - Ci.nsICertOverrideService.ERROR_UNTRUSTED | - Ci.nsICertOverrideService.ERROR_MISMATCH; certOverrideService.rememberValidityOverride( "localhost", server.port, {}, cert, - overrideBits, true ); // Configure Firefox to use the proxy. diff --git a/browser/components/urlbar/tests/browser/browser_speculative_connect_not_with_client_cert.js b/browser/components/urlbar/tests/browser/browser_speculative_connect_not_with_client_cert.js index 1906fd228c58..5302fb72b7fc 100644 --- a/browser/components/urlbar/tests/browser/browser_speculative_connect_not_with_client_cert.js +++ b/browser/components/urlbar/tests/browser/browser_speculative_connect_not_with_client_cert.js @@ -160,15 +160,11 @@ add_setup(async function() { }, ]); - let overrideBits = - Ci.nsICertOverrideService.ERROR_UNTRUSTED | - Ci.nsICertOverrideService.ERROR_MISMATCH; certOverrideService.rememberValidityOverride( "localhost", server.port, {}, cert, - overrideBits, true ); diff --git a/dom/base/Document.cpp b/dom/base/Document.cpp index e0e4f4110888..9ec50d9051fb 100644 --- a/dom/base/Document.cpp +++ b/dom/base/Document.cpp @@ -1572,27 +1572,6 @@ already_AddRefed Document::AddCertException( return promise.forget(); } - bool isUntrusted = true; - rv = tsi->GetIsUntrusted(&isUntrusted); - if (NS_WARN_IF(NS_FAILED(rv))) { - promise->MaybeReject(rv); - return promise.forget(); - } - - bool isDomainMismatch = true; - rv = tsi->GetIsDomainMismatch(&isDomainMismatch); - if (NS_WARN_IF(NS_FAILED(rv))) { - promise->MaybeReject(rv); - return promise.forget(); - } - - bool isNotValidAtThisTime = true; - rv = tsi->GetIsNotValidAtThisTime(&isNotValidAtThisTime); - if (NS_WARN_IF(NS_FAILED(rv))) { - promise->MaybeReject(rv); - return promise.forget(); - } - nsCOMPtr cert; rv = tsi->GetServerCert(getter_AddRefs(cert)); if (NS_WARN_IF(NS_FAILED(rv))) { @@ -1604,17 +1583,6 @@ already_AddRefed Document::AddCertException( return promise.forget(); } - uint32_t flags = 0; - if (isUntrusted) { - flags |= nsICertOverrideService::ERROR_UNTRUSTED; - } - if (isDomainMismatch) { - flags |= nsICertOverrideService::ERROR_MISMATCH; - } - if (isNotValidAtThisTime) { - flags |= nsICertOverrideService::ERROR_TIME; - } - if (XRE_IsContentProcess()) { nsCOMPtr certSer = do_QueryInterface(cert); nsCString certSerialized; @@ -1623,8 +1591,7 @@ already_AddRefed Document::AddCertException( ContentChild* cc = ContentChild::GetSingleton(); MOZ_ASSERT(cc); OriginAttributes const& attrs = NodePrincipal()->OriginAttributesRef(); - cc->SendAddCertException(certSerialized, flags, host, port, attrs, - aIsTemporary) + cc->SendAddCertException(certSerialized, host, port, attrs, aIsTemporary) ->Then(GetCurrentSerialEventTarget(), __func__, [promise](const mozilla::MozPromise< nsresult, mozilla::ipc::ResponseRejectReason, @@ -1648,7 +1615,7 @@ already_AddRefed Document::AddCertException( OriginAttributes const& attrs = NodePrincipal()->OriginAttributesRef(); rv = overrideService->RememberValidityOverride(host, port, attrs, cert, - flags, aIsTemporary); + aIsTemporary); if (NS_WARN_IF(NS_FAILED(rv))) { promise->MaybeReject(rv); return promise.forget(); diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp index e8a941139198..31ae9c7d712d 100644 --- a/dom/ipc/ContentParent.cpp +++ b/dom/ipc/ContentParent.cpp @@ -6547,9 +6547,8 @@ mozilla::ipc::IPCResult ContentParent::RecvBHRThreadHang( } mozilla::ipc::IPCResult ContentParent::RecvAddCertException( - const nsACString& aSerializedCert, uint32_t aFlags, - const nsACString& aHostName, int32_t aPort, - const OriginAttributes& aOriginAttributes, bool aIsTemporary, + const nsACString& aSerializedCert, const nsACString& aHostName, + int32_t aPort, const OriginAttributes& aOriginAttributes, bool aIsTemporary, AddCertExceptionResolver&& aResolver) { nsCOMPtr certObj; nsresult rv = NS_DeserializeObject(aSerializedCert, getter_AddRefs(certObj)); @@ -6564,7 +6563,7 @@ mozilla::ipc::IPCResult ContentParent::RecvAddCertException( rv = NS_ERROR_FAILURE; } else { rv = overrideService->RememberValidityOverride( - aHostName, aPort, aOriginAttributes, cert, aFlags, aIsTemporary); + aHostName, aPort, aOriginAttributes, cert, aIsTemporary); } } } diff --git a/dom/ipc/ContentParent.h b/dom/ipc/ContentParent.h index 0d77591a72e3..d7ce54488488 100644 --- a/dom/ipc/ContentParent.h +++ b/dom/ipc/ContentParent.h @@ -1236,10 +1236,9 @@ class ContentParent final : public PContentParent, mozilla::ipc::IPCResult RecvBHRThreadHang(const HangDetails& aHangDetails); mozilla::ipc::IPCResult RecvAddCertException( - const nsACString& aSerializedCert, uint32_t aFlags, - const nsACString& aHostName, int32_t aPort, - const OriginAttributes& aOriginAttributes, bool aIsTemporary, - AddCertExceptionResolver&& aResolver); + const nsACString& aSerializedCert, const nsACString& aHostName, + int32_t aPort, const OriginAttributes& aOriginAttributes, + bool aIsTemporary, AddCertExceptionResolver&& aResolver); mozilla::ipc::IPCResult RecvAutomaticStorageAccessPermissionCanBeGranted( nsIPrincipal* aPrincipal, diff --git a/dom/ipc/PContent.ipdl b/dom/ipc/PContent.ipdl index 5be2d7ec772f..c6e689e80304 100644 --- a/dom/ipc/PContent.ipdl +++ b/dom/ipc/PContent.ipdl @@ -1626,8 +1626,8 @@ parent: /* * Adds a certificate exception for the given hostname and port. */ - async AddCertException(nsCString aSerializedCert, uint32_t aFlags, - nsCString aHostName, int32_t aPort, OriginAttributes aOriginAttributes, + async AddCertException(nsCString aSerializedCert, nsCString aHostName, + int32_t aPort, OriginAttributes aOriginAttributes, bool aIsTemporary) returns (nsresult success); diff --git a/dom/media/webrtc/tests/mochitests/addTurnsSelfsignedCert.js b/dom/media/webrtc/tests/mochitests/addTurnsSelfsignedCert.js index 7716ea59ce49..1e8be3a397dd 100644 --- a/dom/media/webrtc/tests/mochitests/addTurnsSelfsignedCert.js +++ b/dom/media/webrtc/tests/mochitests/addTurnsSelfsignedCert.js @@ -25,7 +25,6 @@ addMessageListener("add-turns-certs", certs => { port, {}, cert, - Ci.nsICertOverrideService.ERROR_UNTRUSTED, false ); }); diff --git a/mobile/android/modules/geckoview/GeckoViewProgress.jsm b/mobile/android/modules/geckoview/GeckoViewProgress.jsm index 80b6da81d1c8..71f032fc8e57 100644 --- a/mobile/android/modules/geckoview/GeckoViewProgress.jsm +++ b/mobile/android/modules/geckoview/GeckoViewProgress.jsm @@ -156,7 +156,6 @@ var IdentityHandler = { uri.port, {}, cert, - {}, {} ); diff --git a/netwerk/test/unit/test_be_conservative.js b/netwerk/test/unit/test_be_conservative.js index f9d46f8c2ac0..f7c7d026e0e6 100644 --- a/netwerk/test/unit/test_be_conservative.js +++ b/netwerk/test/unit/test_be_conservative.js @@ -150,18 +150,7 @@ function storeCertOverride(port, cert) { let certOverrideService = Cc[ "@mozilla.org/security/certoverride;1" ].getService(Ci.nsICertOverrideService); - let overrideBits = - Ci.nsICertOverrideService.ERROR_UNTRUSTED | - Ci.nsICertOverrideService.ERROR_TIME | - Ci.nsICertOverrideService.ERROR_MISMATCH; - certOverrideService.rememberValidityOverride( - hostname, - port, - {}, - cert, - overrideBits, - true - ); + certOverrideService.rememberValidityOverride(hostname, port, {}, cert, true); } function startClient(port, beConservative, expectSuccess) { diff --git a/netwerk/test/unit/test_be_conservative_error_handling.js b/netwerk/test/unit/test_be_conservative_error_handling.js index 91e491ad9bc7..d625f9b46bbb 100644 --- a/netwerk/test/unit/test_be_conservative_error_handling.js +++ b/netwerk/test/unit/test_be_conservative_error_handling.js @@ -143,18 +143,7 @@ function storeCertOverride(port, cert) { let certOverrideService = Cc[ "@mozilla.org/security/certoverride;1" ].getService(Ci.nsICertOverrideService); - let overrideBits = - Ci.nsICertOverrideService.ERROR_UNTRUSTED | - Ci.nsICertOverrideService.ERROR_TIME | - Ci.nsICertOverrideService.ERROR_MISMATCH; - certOverrideService.rememberValidityOverride( - hostname, - port, - {}, - cert, - overrideBits, - true - ); + certOverrideService.rememberValidityOverride(hostname, port, {}, cert, true); } function startClient(port, beConservative, expectSuccess) { diff --git a/netwerk/test/unit/test_tls_flags.js b/netwerk/test/unit/test_tls_flags.js index 5b5679b10c46..9e7075aadb7e 100644 --- a/netwerk/test/unit/test_tls_flags.js +++ b/netwerk/test/unit/test_tls_flags.js @@ -158,18 +158,7 @@ function storeCertOverride(port, cert) { let certOverrideService = Cc[ "@mozilla.org/security/certoverride;1" ].getService(Ci.nsICertOverrideService); - let overrideBits = - Ci.nsICertOverrideService.ERROR_UNTRUSTED | - Ci.nsICertOverrideService.ERROR_TIME | - Ci.nsICertOverrideService.ERROR_MISMATCH; - certOverrideService.rememberValidityOverride( - hostname, - port, - {}, - cert, - overrideBits, - true - ); + certOverrideService.rememberValidityOverride(hostname, port, {}, cert, true); } function startClient(port, tlsFlags, expectSuccess) { diff --git a/netwerk/test/unit/test_tls_server.js b/netwerk/test/unit/test_tls_server.js index 00334682450b..6973e6cc9be9 100644 --- a/netwerk/test/unit/test_tls_server.js +++ b/netwerk/test/unit/test_tls_server.js @@ -116,16 +116,11 @@ function startServer( } function storeCertOverride(port, cert) { - let overrideBits = - Ci.nsICertOverrideService.ERROR_UNTRUSTED | - Ci.nsICertOverrideService.ERROR_TIME | - Ci.nsICertOverrideService.ERROR_MISMATCH; certOverrideService.rememberValidityOverride( "127.0.0.1", port, {}, cert, - overrideBits, true ); } diff --git a/netwerk/test/unit/test_tls_server_multiple_clients.js b/netwerk/test/unit/test_tls_server_multiple_clients.js index 066e64594ceb..eb29be9983e1 100644 --- a/netwerk/test/unit/test_tls_server_multiple_clients.js +++ b/netwerk/test/unit/test_tls_server_multiple_clients.js @@ -62,16 +62,11 @@ function startServer(cert) { } function storeCertOverride(port, cert) { - let overrideBits = - Ci.nsICertOverrideService.ERROR_UNTRUSTED | - Ci.nsICertOverrideService.ERROR_TIME | - Ci.nsICertOverrideService.ERROR_MISMATCH; certOverrideService.rememberValidityOverride( "127.0.0.1", port, {}, cert, - overrideBits, true ); } diff --git a/security/certverifier/CertVerifier.cpp b/security/certverifier/CertVerifier.cpp index 1d5a3844c3ed..eb1c07ead75f 100644 --- a/security/certverifier/CertVerifier.cpp +++ b/security/certverifier/CertVerifier.cpp @@ -752,6 +752,24 @@ static bool CertIsSelfSigned(const BackCert& backCert, void* pinarg) { return rv == Success; } +static Result CheckCertHostnameHelper(Input peerCertInput, + const nsACString& hostname) { + Input hostnameInput; + Result rv = hostnameInput.Init( + BitwiseCast(hostname.BeginReading()), + hostname.Length()); + if (rv != Success) { + return Result::FATAL_ERROR_INVALID_ARGS; + } + + rv = CheckCertHostname(peerCertInput, hostnameInput); + // Treat malformed name information as a domain mismatch. + if (rv == Result::ERROR_BAD_DER) { + return Result::ERROR_BAD_CERT_DOMAIN; + } + return rv; +} + Result CertVerifier::VerifySSLServerCert( const nsTArray& peerCertBytes, Time time, /*optional*/ void* pinarg, const nsACString& hostname, @@ -836,6 +854,17 @@ Result CertVerifier::VerifySSLServerCert( return Result::ERROR_MITM_DETECTED; } } + // If the certificate is expired or not yet valid, first check whether or + // not it is valid for the indicated hostname, because that would be a more + // serious error. + if (rv == Result::ERROR_EXPIRED_CERTIFICATE || + rv == Result::ERROR_NOT_YET_VALID_CERTIFICATE || + rv == Result::ERROR_INVALID_DER_TIME) { + Result hostnameResult = CheckCertHostnameHelper(peerCertInput, hostname); + if (hostnameResult != Success) { + return hostnameResult; + } + } return rv; } @@ -865,21 +894,8 @@ Result CertVerifier::VerifySSLServerCert( } } - Input hostnameInput; - rv = hostnameInput.Init( - BitwiseCast(hostname.BeginReading()), - hostname.Length()); + rv = CheckCertHostnameHelper(peerCertInput, hostname); if (rv != Success) { - return Result::FATAL_ERROR_INVALID_ARGS; - } - - rv = CheckCertHostname(peerCertInput, hostnameInput); - if (rv != Success) { - // Treat malformed name information as a domain mismatch. - if (rv == Result::ERROR_BAD_DER) { - return Result::ERROR_BAD_CERT_DOMAIN; - } - return rv; } diff --git a/security/manager/ssl/PVerifySSLServerCert.ipdl b/security/manager/ssl/PVerifySSLServerCert.ipdl index 3b2dc50b1118..14f56ac28ece 100644 --- a/security/manager/ssl/PVerifySSLServerCert.ipdl +++ b/security/manager/ssl/PVerifySSLServerCert.ipdl @@ -21,8 +21,8 @@ child: uint8_t aEVStatus, bool isBuiltCertChainRootBuiltInRoot); - async OnVerifiedSSLServerCertFailure(uint32_t aFinalError, - uint32_t aCollectedErrors); + async OnVerifiedSSLServerCertFailure(int32_t aFinalError, + uint32_t aOverridableErrorCategory); async __delete__(); }; diff --git a/security/manager/ssl/SSLServerCertVerification.cpp b/security/manager/ssl/SSLServerCertVerification.cpp index a0d0340360e7..3665a8f6389b 100644 --- a/security/manager/ssl/SSLServerCertVerification.cpp +++ b/security/manager/ssl/SSLServerCertVerification.cpp @@ -145,13 +145,9 @@ using namespace mozilla::pkix; namespace mozilla { namespace psm { -namespace { - // do not use a nsCOMPtr to avoid static initializer/destructor nsIThreadPool* gCertVerificationThreadPool = nullptr; -} // unnamed namespace - // Called when the socket transport thread starts, to initialize the SSL cert // verification thread pool. By tying the thread pool startup/shutdown directly // to the STS thread's lifetime, we ensure that they are *always* available for @@ -190,8 +186,6 @@ void StopSSLServerCertVerificationThreads() { } } -namespace { - // A probe value of 1 means "no error". uint32_t MapOverridableErrorToProbeValue(PRErrorCode errorCode) { switch (errorCode) { @@ -236,7 +230,7 @@ uint32_t MapOverridableErrorToProbeValue(PRErrorCode errorCode) { } NS_WARNING( "Unknown certificate error code. Does MapOverridableErrorToProbeValue " - "handle everything in DetermineCertOverrideErrors?"); + "handle everything in CategorizeCertificateError?"); return 0; } @@ -270,35 +264,19 @@ static uint32_t MapCertErrorToProbeValue(PRErrorCode errorCode) { return probeValue; } -SECStatus DetermineCertOverrideErrors(const nsCOMPtr& cert, - const nsACString& hostName, - mozilla::pkix::Time now, - PRErrorCode defaultErrorCodeToReport, - /*out*/ uint32_t& collectedErrors, - /*out*/ PRErrorCode& errorCodeTrust, - /*out*/ PRErrorCode& errorCodeMismatch, - /*out*/ PRErrorCode& errorCodeTime) { - MOZ_ASSERT(cert); - MOZ_ASSERT(collectedErrors == 0); - MOZ_ASSERT(errorCodeTrust == 0); - MOZ_ASSERT(errorCodeMismatch == 0); - MOZ_ASSERT(errorCodeTime == 0); +enum class OverridableErrorCategory : uint32_t { + Unset = 0, + Trust = 1, + Domain = 2, + Time = 3, +}; - nsTArray certDER; - if (NS_FAILED(cert->GetRawDER(certDER))) { - PR_SetError(SEC_ERROR_LIBRARY_FAILURE, 0); - return SECFailure; - } - mozilla::pkix::Input certInput; - if (certInput.Init(certDER.Elements(), certDER.Length()) != Success) { - PR_SetError(SEC_ERROR_BAD_DER, 0); - return SECFailure; - } - - // Assumes the error prioritization described in mozilla::pkix's - // BuildForward function. Also assumes that CheckCertHostname was only - // called if CertVerifier::VerifyCert succeeded. - switch (defaultErrorCodeToReport) { +// If the given PRErrorCode is an overridable certificate error, return which +// category (trust, time, domain mismatch) it falls in. If it is not +// overridable, return Nothing. +Maybe CategorizeCertificateError( + PRErrorCode certificateError) { + switch (certificateError) { case SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED: case SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE: case SEC_ERROR_UNKNOWN_ISSUER: @@ -310,95 +288,21 @@ SECStatus DetermineCertOverrideErrors(const nsCOMPtr& cert, case mozilla::pkix::MOZILLA_PKIX_ERROR_MITM_DETECTED: case mozilla::pkix::MOZILLA_PKIX_ERROR_NOT_YET_VALID_ISSUER_CERTIFICATE: case mozilla::pkix::MOZILLA_PKIX_ERROR_SELF_SIGNED_CERT: - case mozilla::pkix::MOZILLA_PKIX_ERROR_V1_CERT_USED_AS_CA: { - collectedErrors = nsICertOverrideService::ERROR_UNTRUSTED; - errorCodeTrust = defaultErrorCodeToReport; - - mozilla::pkix::BackCert backCert( - certInput, mozilla::pkix::EndEntityOrCA::MustBeEndEntity, nullptr); - Result rv = backCert.Init(); - if (rv != Success) { - PR_SetError(MapResultToPRErrorCode(rv), 0); - return SECFailure; - } - mozilla::pkix::Time notBefore(mozilla::pkix::Time::uninitialized); - mozilla::pkix::Time notAfter(mozilla::pkix::Time::uninitialized); - // If the validity can't be parsed, ParseValidity will return - // Result::ERROR_INVALID_DER_TIME. - rv = mozilla::pkix::ParseValidity(backCert.GetValidity(), ¬Before, - ¬After); - if (rv != Success) { - collectedErrors |= nsICertOverrideService::ERROR_TIME; - errorCodeTime = MapResultToPRErrorCode(rv); - break; - } - // If `now` is outside of the certificate's validity period, - // CheckValidity will return Result::ERROR_NOT_YET_VALID_CERTIFICATE or - // Result::ERROR_EXPIRED_CERTIFICATE, as appropriate, and Success - // otherwise. - rv = mozilla::pkix::CheckValidity(now, notBefore, notAfter); - if (rv != Success) { - collectedErrors |= nsICertOverrideService::ERROR_TIME; - errorCodeTime = MapResultToPRErrorCode(rv); - } - break; - } + case mozilla::pkix::MOZILLA_PKIX_ERROR_V1_CERT_USED_AS_CA: + return Some(OverridableErrorCategory::Trust); case SEC_ERROR_INVALID_TIME: case SEC_ERROR_EXPIRED_CERTIFICATE: case mozilla::pkix::MOZILLA_PKIX_ERROR_NOT_YET_VALID_CERTIFICATE: - collectedErrors = nsICertOverrideService::ERROR_TIME; - errorCodeTime = defaultErrorCodeToReport; - break; + return Some(OverridableErrorCategory::Time); case SSL_ERROR_BAD_CERT_DOMAIN: - collectedErrors = nsICertOverrideService::ERROR_MISMATCH; - errorCodeMismatch = SSL_ERROR_BAD_CERT_DOMAIN; - break; - - case 0: - NS_ERROR("No error code set during certificate validation failure."); - PR_SetError(PR_INVALID_STATE_ERROR, 0); - return SECFailure; + return Some(OverridableErrorCategory::Domain); default: - PR_SetError(defaultErrorCodeToReport, 0); - return SECFailure; + break; } - - if (defaultErrorCodeToReport != SSL_ERROR_BAD_CERT_DOMAIN) { - Input hostnameInput; - Result result = hostnameInput.Init( - BitwiseCast(hostName.BeginReading()), - hostName.Length()); - if (result != Success) { - PR_SetError(SEC_ERROR_INVALID_ARGS, 0); - return SECFailure; - } - // CheckCertHostname expects that its input represents a certificate that - // has already been successfully validated by BuildCertChain. This is - // obviously not the case, however, because we're in the error path of - // certificate verification. Thus, this is problematic. In the future, it - // would be nice to remove this optimistic additional error checking and - // simply punt to the front-end, which can more easily (and safely) perform - // extra checks to give the user hints as to why verification failed. - result = CheckCertHostname(certInput, hostnameInput); - // Treat malformed name information as a domain mismatch. - if (result == Result::ERROR_BAD_DER || - result == Result::ERROR_BAD_CERT_DOMAIN) { - collectedErrors |= nsICertOverrideService::ERROR_MISMATCH; - errorCodeMismatch = SSL_ERROR_BAD_CERT_DOMAIN; - } else if (IsFatalError(result)) { - // Because its input has not been validated by BuildCertChain, - // CheckCertHostname can return an error that is less important than the - // original certificate verification error. Only return an error result - // from this function if we've encountered a fatal error. - PR_SetError(MapResultToPRErrorCode(result), 0); - return SECFailure; - } - } - - return SECSuccess; + return Nothing(); } // Helper function to determine if overrides are allowed for this host. @@ -727,112 +631,60 @@ PRErrorCode AuthCertificateParseResults( uint64_t aPtrForLog, const nsACString& aHostName, int32_t aPort, const OriginAttributes& aOriginAttributes, const nsCOMPtr& aCert, mozilla::pkix::Time aTime, - PRErrorCode aDefaultErrorCodeToReport, - /* out */ uint32_t& aCollectedErrors) { - if (aDefaultErrorCodeToReport == 0) { - MOZ_ASSERT_UNREACHABLE( - "No error set during certificate validation failure"); - return SEC_ERROR_LIBRARY_FAILURE; - } - - uint32_t probeValue = MapCertErrorToProbeValue(aDefaultErrorCodeToReport); + PRErrorCode aCertVerificationError, + /* out */ OverridableErrorCategory& aOverridableErrorCategory) { + uint32_t probeValue = MapCertErrorToProbeValue(aCertVerificationError); Telemetry::Accumulate(Telemetry::SSL_CERT_VERIFICATION_ERRORS, probeValue); - aCollectedErrors = 0; - PRErrorCode errorCodeTrust = 0; - PRErrorCode errorCodeMismatch = 0; - PRErrorCode errorCodeTime = 0; - if (DetermineCertOverrideErrors( - aCert, aHostName, aTime, aDefaultErrorCodeToReport, aCollectedErrors, - errorCodeTrust, errorCodeMismatch, errorCodeTime) != SECSuccess) { - PRErrorCode errorCode = PR_GetError(); - MOZ_ASSERT(!ErrorIsOverridable(errorCode)); - if (errorCode == 0) { - MOZ_ASSERT_UNREACHABLE( - "No error set during DetermineCertOverrideErrors failure"); - return SEC_ERROR_LIBRARY_FAILURE; - } - return errorCode; - } - - if (!aCollectedErrors) { - MOZ_ASSERT_UNREACHABLE("aCollectedErrors should not be 0"); - return SEC_ERROR_LIBRARY_FAILURE; + Maybe maybeOverridableErrorCategory = + CategorizeCertificateError(aCertVerificationError); + // If this isn't an overridable error, return it now. This will stop the + // connection and report the given error. + if (!maybeOverridableErrorCategory.isSome()) { + return aCertVerificationError; } + aOverridableErrorCategory = *maybeOverridableErrorCategory; bool overrideAllowed = false; - if (NS_FAILED(OverrideAllowedForHost(aPtrForLog, aHostName, aOriginAttributes, - overrideAllowed))) { - MOZ_LOG(gPIPNSSLog, LogLevel::Debug, - ("[0x%" PRIx64 "] AuthCertificateParseResults - " - "OverrideAllowedForHost failed\n", - aPtrForLog)); - return aDefaultErrorCodeToReport; + nsresult rv = OverrideAllowedForHost(aPtrForLog, aHostName, aOriginAttributes, + overrideAllowed); + if (NS_FAILED(rv)) { + return aCertVerificationError; } - if (overrideAllowed) { - nsCOMPtr overrideService = - do_GetService(NS_CERTOVERRIDE_CONTRACTID); - - uint32_t overrideBits = 0; - uint32_t remainingDisplayErrors = aCollectedErrors; - - // it is fine to continue without the nsICertOverrideService - if (overrideService) { - bool haveOverride; - bool isTemporaryOverride; // we don't care - nsresult rv = overrideService->HasMatchingOverride( - aHostName, aPort, aOriginAttributes, aCert, &overrideBits, - &isTemporaryOverride, &haveOverride); - if (NS_SUCCEEDED(rv) && haveOverride) { - // remove the errors that are already overriden - remainingDisplayErrors &= ~overrideBits; - } - } - - if (!remainingDisplayErrors) { - // This can double- or triple-count one certificate with multiple - // different types of errors. Since this is telemetry and we just - // want a ballpark answer, we don't care. - if (errorCodeTrust != 0) { - uint32_t probeValue = MapOverridableErrorToProbeValue(errorCodeTrust); - Telemetry::Accumulate(Telemetry::SSL_CERT_ERROR_OVERRIDES, probeValue); - } - if (errorCodeMismatch != 0) { - uint32_t probeValue = - MapOverridableErrorToProbeValue(errorCodeMismatch); - Telemetry::Accumulate(Telemetry::SSL_CERT_ERROR_OVERRIDES, probeValue); - } - if (errorCodeTime != 0) { - uint32_t probeValue = MapOverridableErrorToProbeValue(errorCodeTime); - Telemetry::Accumulate(Telemetry::SSL_CERT_ERROR_OVERRIDES, probeValue); - } - - // all errors are covered by override rules, so let's accept the cert - MOZ_LOG( - gPIPNSSLog, LogLevel::Debug, - ("[0x%" PRIx64 "] All errors covered by override rules", aPtrForLog)); - return 0; - } - } else { + if (!overrideAllowed) { MOZ_LOG(gPIPNSSLog, LogLevel::Debug, - ("[0x%" PRIx64 "] HSTS or pinned host - no overrides allowed\n", + ("[0x%" PRIx64 "] HSTS or pinned host - no overrides allowed", aPtrForLog)); + return aCertVerificationError; } - MOZ_LOG( - gPIPNSSLog, LogLevel::Debug, - ("[0x%" PRIx64 "] Certificate error was not overridden\n", aPtrForLog)); + nsCOMPtr overrideService = + do_GetService(NS_CERTOVERRIDE_CONTRACTID); + if (!overrideService) { + return aCertVerificationError; + } + bool haveOverride; + bool isTemporaryOverride; + rv = overrideService->HasMatchingOverride(aHostName, aPort, aOriginAttributes, + aCert, &isTemporaryOverride, + &haveOverride); + if (NS_FAILED(rv)) { + return aCertVerificationError; + } + Unused << isTemporaryOverride; + if (haveOverride) { + uint32_t probeValue = + MapOverridableErrorToProbeValue(aCertVerificationError); + Telemetry::Accumulate(Telemetry::SSL_CERT_ERROR_OVERRIDES, probeValue); + MOZ_LOG(gPIPNSSLog, LogLevel::Debug, + ("[0x%" PRIx64 "] certificate error overridden", aPtrForLog)); + return 0; + } - // pick the error code to report by priority - return errorCodeTrust ? errorCodeTrust - : errorCodeMismatch ? errorCodeMismatch - : errorCodeTime ? errorCodeTime - : aDefaultErrorCodeToReport; + return aCertVerificationError; } -} // unnamed namespace - static nsTArray> CreateCertBytesArray( const UniqueCERTCertList& aCertChain) { nsTArray> certsBytes; @@ -928,7 +780,8 @@ SSLServerCertVerificationJob::Run() { std::move(builtChainBytesArray), std::move(mPeerCertChain), TransportSecurityInfo::ConvertCertificateTransparencyInfoToStatus( certificateTransparencyInfo), - evStatus, true, 0, 0, isCertChainRootBuiltInRoot, mProviderFlags); + evStatus, true, 0, OverridableErrorCategory::Unset, + isCertChainRootBuiltInRoot, mProviderFlags); return NS_OK; } @@ -937,17 +790,18 @@ SSLServerCertVerificationJob::Run() { jobStartTime, TimeStamp::Now()); PRErrorCode error = MapResultToPRErrorCode(rv); - uint32_t collectedErrors = 0; + OverridableErrorCategory overridableErrorCategory = + OverridableErrorCategory::Unset; nsCOMPtr cert(new nsNSSCertificate(std::move(certBytes))); PRErrorCode finalError = AuthCertificateParseResults( mAddrForLogging, mHostName, mPort, mOriginAttributes, cert, mTime, error, - collectedErrors); + overridableErrorCategory); // NB: finalError may be 0 here, in which the connection will continue. mResultTask->Dispatch( std::move(builtChainBytesArray), std::move(mPeerCertChain), nsITransportSecurityInfo::CERTIFICATE_TRANSPARENCY_NOT_APPLICABLE, - EVStatus::NotEV, false, finalError, collectedErrors, false, + EVStatus::NotEV, false, finalError, overridableErrorCategory, false, mProviderFlags); return NS_OK; } @@ -1168,14 +1022,15 @@ SSLServerCertVerificationResult::SSLServerCertVerificationResult( mEVStatus(EVStatus::NotEV), mSucceeded(false), mFinalError(0), - mCollectedErrors(0), + mOverridableErrorCategory(OverridableErrorCategory::Unset), mProviderFlags(0) {} void SSLServerCertVerificationResult::Dispatch( nsTArray>&& aBuiltChain, nsTArray>&& aPeerCertChain, uint16_t aCertificateTransparencyStatus, EVStatus aEVStatus, - bool aSucceeded, PRErrorCode aFinalError, uint32_t aCollectedErrors, + bool aSucceeded, PRErrorCode aFinalError, + OverridableErrorCategory aOverridableErrorCategory, bool aIsBuiltCertChainRootBuiltInRoot, uint32_t aProviderFlags) { mBuiltChain = std::move(aBuiltChain); mPeerCertChain = std::move(aPeerCertChain); @@ -1183,7 +1038,7 @@ void SSLServerCertVerificationResult::Dispatch( mEVStatus = aEVStatus; mSucceeded = aSucceeded; mFinalError = aFinalError; - mCollectedErrors = aCollectedErrors; + mOverridableErrorCategory = aOverridableErrorCategory; mIsBuiltCertChainRootBuiltInRoot = aIsBuiltCertChainRootBuiltInRoot; mProviderFlags = aProviderFlags; @@ -1251,8 +1106,8 @@ SSLServerCertVerificationResult::Run() { // Certificate validation failed; store the peer certificate chain on // infoObject so it can be used for error reporting. mInfoObject->SetFailedCertChain(std::move(mPeerCertChain)); - if (mCollectedErrors != 0) { - mInfoObject->SetStatusErrorBits(cert, mCollectedErrors); + if (mOverridableErrorCategory != OverridableErrorCategory::Unset) { + mInfoObject->SetStatusErrorBits(cert, mOverridableErrorCategory); } } diff --git a/security/manager/ssl/SSLServerCertVerification.h b/security/manager/ssl/SSLServerCertVerification.h index fa75bb918e7f..ef7bf7c43425 100644 --- a/security/manager/ssl/SSLServerCertVerification.h +++ b/security/manager/ssl/SSLServerCertVerification.h @@ -38,6 +38,8 @@ SECStatus AuthCertificateHookWithInfo( Maybe>>& stapledOCSPResponses, Maybe>& sctsFromTLSExtension, uint32_t providerFlags); +enum class OverridableErrorCategory : uint32_t; + // Base class for dispatching the certificate verification result. class BaseSSLServerCertVerificationResult { public: @@ -47,7 +49,8 @@ class BaseSSLServerCertVerificationResult { nsTArray>&& aPeerCertChain, uint16_t aCertificateTransparencyStatus, EVStatus aEVStatus, bool aSucceeded, - PRErrorCode aFinalError, uint32_t aCollectedErrors, + PRErrorCode aFinalError, + OverridableErrorCategory aOverridableErrorCategory, bool aIsBuiltCertChainRootBuiltInRoot, uint32_t aProviderFlags) = 0; }; @@ -71,7 +74,7 @@ class SSLServerCertVerificationResult final nsTArray>&& aPeerCertChain, uint16_t aCertificateTransparencyStatus, EVStatus aEVStatus, bool aSucceeded, PRErrorCode aFinalError, - uint32_t aCollectedErrors, + OverridableErrorCategory aOverridableErrorCategory, bool aIsBuiltCertChainRootBuiltInRoot, uint32_t aProviderFlags) override; @@ -85,7 +88,7 @@ class SSLServerCertVerificationResult final EVStatus mEVStatus; bool mSucceeded; PRErrorCode mFinalError; - uint32_t mCollectedErrors; + OverridableErrorCategory mOverridableErrorCategory; bool mIsBuiltCertChainRootBuiltInRoot; uint32_t mProviderFlags; }; diff --git a/security/manager/ssl/TransportSecurityInfo.cpp b/security/manager/ssl/TransportSecurityInfo.cpp index 64fc9a72c34f..0a020249d7c9 100644 --- a/security/manager/ssl/TransportSecurityInfo.cpp +++ b/security/manager/ssl/TransportSecurityInfo.cpp @@ -955,13 +955,24 @@ void RememberCertErrorsTable::LookupCertErrorBits( } void TransportSecurityInfo::SetStatusErrorBits( - const nsCOMPtr& cert, uint32_t collected_errors) { + const nsCOMPtr& cert, + OverridableErrorCategory overridableErrorCategory) { SetServerCert(cert, EVStatus::NotEV); mHaveCertErrorBits = true; - mIsDomainMismatch = collected_errors & nsICertOverrideService::ERROR_MISMATCH; - mIsNotValidAtThisTime = collected_errors & nsICertOverrideService::ERROR_TIME; - mIsUntrusted = collected_errors & nsICertOverrideService::ERROR_UNTRUSTED; + switch (overridableErrorCategory) { + case OverridableErrorCategory::Trust: + mIsUntrusted = true; + break; + case OverridableErrorCategory::Time: + mIsNotValidAtThisTime = true; + break; + case OverridableErrorCategory::Domain: + mIsDomainMismatch = true; + break; + default: + break; + } RememberCertErrorsTable::GetInstance().RememberCertHasError(this, SECFailure); } diff --git a/security/manager/ssl/TransportSecurityInfo.h b/security/manager/ssl/TransportSecurityInfo.h index be7530a93a74..c9c899a62e00 100644 --- a/security/manager/ssl/TransportSecurityInfo.h +++ b/security/manager/ssl/TransportSecurityInfo.h @@ -27,6 +27,8 @@ namespace mozilla { namespace psm { +enum class OverridableErrorCategory : uint32_t; + class TransportSecurityInfo : public nsITransportSecurityInfo, public nsIInterfaceRequestor, public nsISerializable, @@ -79,7 +81,7 @@ class TransportSecurityInfo : public nsITransportSecurityInfo, bool IsCanceled(); void SetStatusErrorBits(const nsCOMPtr& cert, - uint32_t collected_errors); + OverridableErrorCategory overridableErrorCategory); nsresult SetFailedCertChain(nsTArray>&& certList); diff --git a/security/manager/ssl/VerifySSLServerCertChild.cpp b/security/manager/ssl/VerifySSLServerCertChild.cpp index d7cda4a70f47..d85253672ae3 100644 --- a/security/manager/ssl/VerifySSLServerCertChild.cpp +++ b/security/manager/ssl/VerifySSLServerCertChild.cpp @@ -40,23 +40,19 @@ ipc::IPCResult VerifySSLServerCertChild::RecvOnVerifiedSSLServerCertSuccess( mResultTask->Dispatch(std::move(certBytesArray), std::move(mPeerCertChain), aCertTransparencyStatus, - static_cast(aEVStatus), true, 0, 0, + static_cast(aEVStatus), true, 0, + OverridableErrorCategory::Unset, aIsBuiltCertChainRootBuiltInRoot, mProviderFlags); return IPC_OK(); } ipc::IPCResult VerifySSLServerCertChild::RecvOnVerifiedSSLServerCertFailure( - const uint32_t& aFinalError, const uint32_t& aCollectedErrors) { - MOZ_LOG(gPIPNSSLog, LogLevel::Debug, - ("[%p]VerifySSLServerCertChild::" - "RecvOnVerifiedSSLServerCertFailure - aFinalError=%u, " - "aCollectedErrors=%u", - this, aFinalError, aCollectedErrors)); - + const int32_t& aFinalError, const uint32_t& aOverridableErrorCategory) { mResultTask->Dispatch( nsTArray>(), std::move(mPeerCertChain), nsITransportSecurityInfo::CERTIFICATE_TRANSPARENCY_NOT_APPLICABLE, - EVStatus::NotEV, false, aFinalError, aCollectedErrors, false, + EVStatus::NotEV, false, aFinalError, + static_cast(aOverridableErrorCategory), false, mProviderFlags); return IPC_OK(); } diff --git a/security/manager/ssl/VerifySSLServerCertChild.h b/security/manager/ssl/VerifySSLServerCertChild.h index 12d6dafe327c..e393080083e3 100644 --- a/security/manager/ssl/VerifySSLServerCertChild.h +++ b/security/manager/ssl/VerifySSLServerCertChild.h @@ -36,7 +36,7 @@ class VerifySSLServerCertChild : public PVerifySSLServerCertChild { const bool& aIsBuiltCertChainRootBuiltInRoot); ipc::IPCResult RecvOnVerifiedSSLServerCertFailure( - const uint32_t& aFinalError, const uint32_t& aCollectedErrors); + const int32_t& aFinalError, const uint32_t& aOverridableErrorCategory); private: ~VerifySSLServerCertChild() = default; diff --git a/security/manager/ssl/VerifySSLServerCertParent.cpp b/security/manager/ssl/VerifySSLServerCertParent.cpp index 5c70ebcc472c..240d4c8934e6 100644 --- a/security/manager/ssl/VerifySSLServerCertParent.cpp +++ b/security/manager/ssl/VerifySSLServerCertParent.cpp @@ -31,7 +31,7 @@ VerifySSLServerCertParent::VerifySSLServerCertParent() {} void VerifySSLServerCertParent::OnVerifiedSSLServerCert( const nsTArray& aBuiltCertChain, uint16_t aCertificateTransparencyStatus, uint8_t aEVStatus, bool aSucceeded, - PRErrorCode aFinalError, uint32_t aCollectedErrors, + PRErrorCode aFinalError, uint32_t aOverridableErrorCategory, bool aIsBuiltCertChainRootBuiltInRoot) { AssertIsOnBackgroundThread(); @@ -44,7 +44,8 @@ void VerifySSLServerCertParent::OnVerifiedSSLServerCert( aBuiltCertChain, aCertificateTransparencyStatus, aEVStatus, aIsBuiltCertChainRootBuiltInRoot); } else { - Unused << SendOnVerifiedSSLServerCertFailure(aFinalError, aCollectedErrors); + Unused << SendOnVerifiedSSLServerCertFailure(aFinalError, + aOverridableErrorCategory); } Unused << Send__delete__(this); } @@ -65,7 +66,7 @@ class IPCServerCertVerificationResult final nsTArray>&& aPeerCertChain, uint16_t aCertificateTransparencyStatus, EVStatus aEVStatus, bool aSucceeded, PRErrorCode aFinalError, - uint32_t aCollectedErrors, + OverridableErrorCategory aOverridableErrorCategory, bool aIsBuiltCertChainRootBuiltInRoot, uint32_t aProviderFlags) override; @@ -80,7 +81,8 @@ void IPCServerCertVerificationResult::Dispatch( nsTArray>&& aBuiltChain, nsTArray>&& aPeerCertChain, uint16_t aCertificateTransparencyStatus, EVStatus aEVStatus, - bool aSucceeded, PRErrorCode aFinalError, uint32_t aCollectedErrors, + bool aSucceeded, PRErrorCode aFinalError, + OverridableErrorCategory aOverridableErrorCategory, bool aIsBuiltCertChainRootBuiltInRoot, uint32_t aProviderFlags) { nsTArray builtCertChain; if (aSucceeded) { @@ -94,7 +96,7 @@ void IPCServerCertVerificationResult::Dispatch( "psm::VerifySSLServerCertParent::OnVerifiedSSLServerCert", [parent(mParent), builtCertChain{std::move(builtCertChain)}, aCertificateTransparencyStatus, aEVStatus, aSucceeded, aFinalError, - aCollectedErrors, aIsBuiltCertChainRootBuiltInRoot, + aOverridableErrorCategory, aIsBuiltCertChainRootBuiltInRoot, aProviderFlags]() { if (aSucceeded && !(aProviderFlags & nsISocketProvider::NO_PERMANENT_STORAGE)) { @@ -109,7 +111,8 @@ void IPCServerCertVerificationResult::Dispatch( parent->OnVerifiedSSLServerCert( builtCertChain, aCertificateTransparencyStatus, static_cast(aEVStatus), aSucceeded, aFinalError, - aCollectedErrors, aIsBuiltCertChainRootBuiltInRoot); + static_cast(aOverridableErrorCategory), + aIsBuiltCertChainRootBuiltInRoot); }), NS_DISPATCH_NORMAL); MOZ_DIAGNOSTIC_ASSERT(NS_SUCCEEDED(nrv)); diff --git a/security/manager/ssl/VerifySSLServerCertParent.h b/security/manager/ssl/VerifySSLServerCertParent.h index eb8b450878b6..d3e8f2d1e7c8 100644 --- a/security/manager/ssl/VerifySSLServerCertParent.h +++ b/security/manager/ssl/VerifySSLServerCertParent.h @@ -43,7 +43,7 @@ class VerifySSLServerCertParent : public PVerifySSLServerCertParent { uint16_t aCertificateTransparencyStatus, uint8_t aEVStatus, bool aSucceeded, PRErrorCode aFinalError, - uint32_t aCollectedErrors, + uint32_t aOverridableErrorCategory, bool aIsBuiltCertChainRootBuiltInRoot); private: diff --git a/security/manager/ssl/nsCertOverrideService.cpp b/security/manager/ssl/nsCertOverrideService.cpp index c738181ccd77..de1ac1bac968 100644 --- a/security/manager/ssl/nsCertOverrideService.cpp +++ b/security/manager/ssl/nsCertOverrideService.cpp @@ -137,50 +137,6 @@ nsCertOverride::GetOriginAttributes( return NS_ERROR_FAILURE; } -void nsCertOverride::convertBitsToString(OverrideBits ob, - /*out*/ nsACString& str) { - str.Truncate(); - - if (ob & OverrideBits::Mismatch) { - str.Append('M'); - } - - if (ob & OverrideBits::Untrusted) { - str.Append('U'); - } - - if (ob & OverrideBits::Time) { - str.Append('T'); - } -} - -void nsCertOverride::convertStringToBits(const nsACString& str, - /*out*/ OverrideBits& ob) { - ob = OverrideBits::None; - - for (uint32_t i = 0; i < str.Length(); i++) { - switch (str.CharAt(i)) { - case 'm': - case 'M': - ob |= OverrideBits::Mismatch; - break; - - case 'u': - case 'U': - ob |= OverrideBits::Untrusted; - break; - - case 't': - case 'T': - ob |= OverrideBits::Time; - break; - - default: - break; - } - } -} - NS_IMPL_ISUPPORTS(nsCertOverrideService, nsICertOverrideService, nsIObserver, nsISupportsWeakReference, nsIAsyncShutdownBlocker) @@ -304,18 +260,11 @@ nsresult nsCertOverrideService::Read(const MutexAutoLock& aProofOfLock) { nsAutoCString buffer; bool isMore = true; - /* file format is: - * - * host:port:originattributes \t fingerprint-algorithm \t fingerprint \t - * override-mask \t dbKey - * - * where override-mask is a sequence of characters, - * M meaning hostname-Mismatch-override - * U meaning Untrusted-override - * T meaning Time-error-override (expired/not yet valid) - * - * if this format isn't respected we move onto the next line in the file. - */ + // Each line is of the form: + // host:port:originAttributes \t sSHA256OIDString \t fingerprint \t \t dbKey + // There may be some "bits" identifiers between the `fingerprint` and `dbKey` + // fields, but these are now ignored. + // Lines that don't match this form are silently dropped. while (isMore && NS_SUCCEEDED(lineInputStream->ReadLine(buffer, &isMore))) { if (buffer.IsEmpty() || buffer.First() == '#') { @@ -358,22 +307,20 @@ nsresult nsCertOverrideService::Read(const MutexAutoLock& aProofOfLock) { continue; } nsDependentCSubstring bitsString; - if (!parser.ReadUntil(Tokenizer::Token::Whitespace(), bitsString) || - bitsString.Length() == 0) { + if (!parser.ReadUntil(Tokenizer::Token::Whitespace(), bitsString)) { continue; } + Unused << bitsString; nsDependentCSubstring dbKey; if (!parser.ReadUntil(Tokenizer::Token::EndOfFile(), dbKey) || dbKey.Length() == 0) { continue; } - nsCertOverride::OverrideBits bits; - nsCertOverride::convertStringToBits(bitsString, bits); AddEntryToList(host, port, attributes, nullptr, // don't have the cert false, // not temporary - fingerprint, bits, dbKey, aProofOfLock); + fingerprint, dbKey, aProofOfLock); } return NS_OK; @@ -410,16 +357,13 @@ nsresult nsCertOverrideService::Write(const MutexAutoLock& aProofOfLock) { continue; } - nsAutoCString bitsString; - nsCertOverride::convertBitsToString(settings->mOverrideBits, bitsString); - output.Append(entry->mKeyString); output.Append(kTab); output.Append(sSHA256OIDString); output.Append(kTab); output.Append(settings->mFingerprint); output.Append(kTab); - output.Append(bitsString); + // the "bits" string used to go here, but it no longer exists output.Append(kTab); output.Append(settings->mDBKey); output.Append(NS_LINEBREAK); @@ -452,7 +396,7 @@ NS_IMETHODIMP nsCertOverrideService::RememberValidityOverride( const nsACString& aHostName, int32_t aPort, const OriginAttributes& aOriginAttributes, nsIX509Cert* aCert, - uint32_t aOverrideBits, bool aTemporary) { + bool aTemporary) { NS_ENSURE_ARG_POINTER(aCert); if (aHostName.IsEmpty() || !IsAscii(aHostName)) { return NS_ERROR_INVALID_ARG; @@ -503,8 +447,7 @@ nsCertOverrideService::RememberValidityOverride( AddEntryToList(aHostName, aPort, aOriginAttributes, aTemporary ? aCert : nullptr, // keep a reference to the cert for temporary overrides - aTemporary, fpStr, - (nsCertOverride::OverrideBits)aOverrideBits, dbkey, lock); + aTemporary, fpStr, dbkey, lock); if (!aTemporary) { Write(lock); } @@ -517,31 +460,26 @@ NS_IMETHODIMP nsCertOverrideService::RememberValidityOverrideScriptable( const nsACString& aHostName, int32_t aPort, JS::Handle aOriginAttributes, nsIX509Cert* aCert, - uint32_t aOverrideBits, bool aTemporary, JSContext* aCx) { + bool aTemporary, JSContext* aCx) { OriginAttributes attrs; if (!aOriginAttributes.isObject() || !attrs.Init(aCx, aOriginAttributes)) { return NS_ERROR_INVALID_ARG; } - return RememberValidityOverride(aHostName, aPort, attrs, aCert, aOverrideBits, - aTemporary); + return RememberValidityOverride(aHostName, aPort, attrs, aCert, aTemporary); } NS_IMETHODIMP nsCertOverrideService::HasMatchingOverride( const nsACString& aHostName, int32_t aPort, const OriginAttributes& aOriginAttributes, nsIX509Cert* aCert, - uint32_t* aOverrideBits, bool* aIsTemporary, bool* aRetval) { + bool* aIsTemporary, bool* aRetval) { bool disableAllSecurityCheck = false; { MutexAutoLock lock(mMutex); disableAllSecurityCheck = mDisableAllSecurityCheck; } if (disableAllSecurityCheck) { - nsCertOverride::OverrideBits all = nsCertOverride::OverrideBits::Untrusted | - nsCertOverride::OverrideBits::Mismatch | - nsCertOverride::OverrideBits::Time; - *aOverrideBits = static_cast(all); *aIsTemporary = false; *aRetval = true; return NS_OK; @@ -553,11 +491,9 @@ nsCertOverrideService::HasMatchingOverride( if (aPort < -1) return NS_ERROR_INVALID_ARG; NS_ENSURE_ARG_POINTER(aCert); - NS_ENSURE_ARG_POINTER(aOverrideBits); NS_ENSURE_ARG_POINTER(aIsTemporary); NS_ENSURE_ARG_POINTER(aRetval); *aRetval = false; - *aOverrideBits = static_cast(nsCertOverride::OverrideBits::None); RefPtr settings( GetOverrideFor(aHostName, aPort, aOriginAttributes)); @@ -570,7 +506,6 @@ nsCertOverrideService::HasMatchingOverride( return NS_OK; } - *aOverrideBits = static_cast(settings->mOverrideBits); *aIsTemporary = settings->mIsTemporary; nsAutoCString fpStr; @@ -600,23 +535,21 @@ NS_IMETHODIMP nsCertOverrideService::HasMatchingOverrideScriptable( const nsACString& aHostName, int32_t aPort, JS::Handle aOriginAttributes, nsIX509Cert* aCert, - uint32_t* aOverrideBits, bool* aIsTemporary, JSContext* aCx, - bool* aRetval) { + bool* aIsTemporary, JSContext* aCx, bool* aRetval) { OriginAttributes attrs; if (!aOriginAttributes.isObject() || !attrs.Init(aCx, aOriginAttributes)) { return NS_ERROR_INVALID_ARG; } - return HasMatchingOverride(aHostName, aPort, attrs, aCert, aOverrideBits, - aIsTemporary, aRetval); + return HasMatchingOverride(aHostName, aPort, attrs, aCert, aIsTemporary, + aRetval); } nsresult nsCertOverrideService::AddEntryToList( const nsACString& aHostName, int32_t aPort, const OriginAttributes& aOriginAttributes, nsIX509Cert* aCert, const bool aIsTemporary, const nsACString& fingerprint, - nsCertOverride::OverrideBits ob, const nsACString& dbKey, - const MutexAutoLock& aProofOfLock) { + const nsACString& dbKey, const MutexAutoLock& aProofOfLock) { mMutex.AssertCurrentThreadOwns(); nsAutoCString keyString; GetKeyString(aHostName, aPort, aOriginAttributes, keyString); @@ -637,7 +570,6 @@ nsresult nsCertOverrideService::AddEntryToList( settings->mOriginAttributes = aOriginAttributes; settings->mIsTemporary = aIsTemporary; settings->mFingerprint = fingerprint; - settings->mOverrideBits = ob; settings->mDBKey = dbKey; // remove whitespace from stored dbKey for backwards compatibility settings->mDBKey.StripWhitespace(); diff --git a/security/manager/ssl/nsCertOverrideService.h b/security/manager/ssl/nsCertOverrideService.h index e22af55d6e94..752792519b31 100644 --- a/security/manager/ssl/nsCertOverrideService.h +++ b/security/manager/ssl/nsCertOverrideService.h @@ -12,7 +12,6 @@ #include "mozilla/HashFunctions.h" #include "mozilla/Mutex.h" #include "mozilla/TaskQueue.h" -#include "mozilla/TypedEnumBits.h" #include "nsIAsyncShutdown.h" #include "nsICertOverrideService.h" #include "nsIFile.h" @@ -27,34 +26,20 @@ class nsCertOverride final : public nsICertOverride { NS_DECL_THREADSAFE_ISUPPORTS NS_DECL_NSICERTOVERRIDE - enum class OverrideBits { - None = 0, - Untrusted = nsICertOverrideService::ERROR_UNTRUSTED, - Mismatch = nsICertOverrideService::ERROR_MISMATCH, - Time = nsICertOverrideService::ERROR_TIME, - }; - - nsCertOverride() - : mPort(-1), mIsTemporary(false), mOverrideBits(OverrideBits::None) {} + nsCertOverride() : mPort(-1), mIsTemporary(false) {} nsCString mAsciiHost; int32_t mPort; OriginAttributes mOriginAttributes; bool mIsTemporary; // true: session only, false: stored on disk nsCString mFingerprint; - OverrideBits mOverrideBits; nsCString mDBKey; nsCOMPtr mCert; - static void convertBitsToString(OverrideBits ob, nsACString& str); - static void convertStringToBits(const nsACString& str, OverrideBits& ob); - private: ~nsCertOverride() = default; }; -MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS(nsCertOverride::OverrideBits) - // hash entry class class nsCertOverrideEntry final : public PLDHashEntryHdr { public: @@ -147,7 +132,6 @@ class nsCertOverrideService final : public nsICertOverrideService, const OriginAttributes& aOriginAttributes, nsIX509Cert* aCert, const bool aIsTemporary, const nsACString& fingerprint, - nsCertOverride::OverrideBits ob, const nsACString& dbKey, const mozilla::MutexAutoLock& aProofOfLock); already_AddRefed GetOverrideFor( diff --git a/security/manager/ssl/nsICertOverrideService.idl b/security/manager/ssl/nsICertOverrideService.idl index 45c1373102e1..e31cf158dcac 100644 --- a/security/manager/ssl/nsICertOverrideService.idl +++ b/security/manager/ssl/nsICertOverrideService.idl @@ -57,59 +57,32 @@ interface nsICertOverride : nsISupports { readonly attribute jsval originAttributes; }; -/** - * This represents the global list of triples - * {host:port, cert-fingerprint, allowed-overrides} - * that the user wants to accept without further warnings. - */ [scriptable, builtinclass, uuid(be019e47-22fc-4355-9f16-9ab047d6742d)] interface nsICertOverrideService : nsISupports { - /** - * Override Untrusted - */ - const short ERROR_UNTRUSTED = 1; - - /** - * Override hostname Mismatch - */ - const short ERROR_MISMATCH = 2; - - /** - * Override Time error - */ - const short ERROR_TIME = 4; - - /** - * The given cert should always be accepted for the given hostname:port, - * regardless of errors verifying the cert. - * Host:Port is a primary key, only one entry per host:port can exist. - * The implementation will store a fingerprint of the cert. - * The implementation will decide which fingerprint alg is used. + * When making a TLS connection to the given hostname and port (in the + * context of the given origin attributes), if the certificate verifier + * encounters an overridable error when verifying the given certificate, the + * connection will continue (provided overrides are allowed for that host). * - * Each override is specific to exactly the errors overridden, so - * overriding everything won't match certs at the given host:port - * which only exhibit some subset of errors. - * - * @param aHostName The host (punycode) this mapping belongs to - * @param aPort The port this mapping belongs to, if it is -1 then it - * is internaly treated as 443 - * @param aCert The cert that should always be accepted - * @param aOverrideBits The precise set of errors we want to be overriden + * @param aHostName The host (punycode) this mapping belongs to + * @param aPort The port this mapping belongs to. If it is -1 then it + * is internaly treated as 443. + * @param aOriginAttributes the origin attributes of the mapping + * @param aCert The certificate used by the server + * @param aTemporary Whether or not to only store the mapping for the session */ [binaryname(RememberValidityOverride), noscript, must_use] void rememberValidityOverrideNative(in AUTF8String aHostName, in int32_t aPort, in const_OriginAttributesRef aOriginAttributes, in nsIX509Cert aCert, - in uint32_t aOverrideBits, in boolean aTemporary); [binaryname(RememberValidityOverrideScriptable), implicit_jscontext, must_use] void rememberValidityOverride(in AUTF8String aHostName, in int32_t aPort, in jsval aOriginAttributes, in nsIX509Cert aCert, - in uint32_t aOverrideBits, in boolean aTemporary); /** @@ -122,7 +95,6 @@ interface nsICertOverrideService : nsISupports { * @param aPort The port this mapping belongs to, if it is -1 then it * is internally treated as 443 * @param aCert The certificate this mapping belongs to - * @param aOverrideBits The errors that are currently overridden * @param aIsTemporary Whether the stored override is session-only, * or permanent * @return Whether an override has been stored for this host+port+cert @@ -132,14 +104,12 @@ interface nsICertOverrideService : nsISupports { in int32_t aPort, in const_OriginAttributesRef aOriginAttributes, in nsIX509Cert aCert, - out uint32_t aOverrideBits, out boolean aIsTemporary); [binaryname(HasMatchingOverrideScriptable), implicit_jscontext, must_use] boolean hasMatchingOverride(in AUTF8String aHostName, in int32_t aPort, in jsval aOriginAttributes, in nsIX509Cert aCert, - out uint32_t aOverrideBits, out boolean aIsTemporary); /** diff --git a/security/manager/ssl/tests/mochitest/browser/browser_certificateManager.js b/security/manager/ssl/tests/mochitest/browser/browser_certificateManager.js index 019078d28316..a7422609e8ec 100644 --- a/security/manager/ssl/tests/mochitest/browser/browser_certificateManager.js +++ b/security/manager/ssl/tests/mochitest/browser/browser_certificateManager.js @@ -118,7 +118,6 @@ add_task(async function test_cert_manager_server_tab() { 443, {}, cert, - Ci.nsICertOverrideService.ERROR_UNTRUSTED, false ); diff --git a/security/manager/ssl/tests/unit/head_psm.js b/security/manager/ssl/tests/unit/head_psm.js index c9f25f1bf0c5..671a20a1749d 100644 --- a/security/manager/ssl/tests/unit/head_psm.js +++ b/security/manager/ssl/tests/unit/head_psm.js @@ -909,47 +909,23 @@ function stopOCSPResponder(responder) { // Helper function for add_cert_override_test. Probably doesn't need to be // called directly. -function add_cert_override(aHost, aExpectedBits, aSecurityInfo) { - let bits = - (aSecurityInfo.isUntrusted - ? Ci.nsICertOverrideService.ERROR_UNTRUSTED - : 0) | - (aSecurityInfo.isDomainMismatch - ? Ci.nsICertOverrideService.ERROR_MISMATCH - : 0) | - (aSecurityInfo.isNotValidAtThisTime - ? Ci.nsICertOverrideService.ERROR_TIME - : 0); - - Assert.equal( - bits, - aExpectedBits, - "Actual and expected override bits should match" - ); +function add_cert_override(aHost, aSecurityInfo) { let cert = aSecurityInfo.serverCert; let certOverrideService = Cc[ "@mozilla.org/security/certoverride;1" ].getService(Ci.nsICertOverrideService); - certOverrideService.rememberValidityOverride( - aHost, - 8443, - {}, - cert, - aExpectedBits, - true - ); + certOverrideService.rememberValidityOverride(aHost, 8443, {}, cert, true); } -// Given a host, expected error bits (see nsICertOverrideService.idl), and an -// expected error code, tests that an initial connection to the host fails -// with the expected errors and that adding an override results in a subsequent -// connection succeeding. -function add_cert_override_test(aHost, aExpectedBits, aExpectedError) { +// Given a host and an expected error code, tests that an initial connection to +// the host fails with the expected error and that adding an override results +// in a subsequent connection succeeding. +function add_cert_override_test(aHost, aExpectedError) { add_connection_test( aHost, aExpectedError, null, - add_cert_override.bind(this, aHost, aExpectedBits) + add_cert_override.bind(this, aHost) ); add_connection_test(aHost, PRErrorCodeSuccess, null, aSecurityInfo => { Assert.ok( @@ -964,54 +940,28 @@ function add_cert_override_test(aHost, aExpectedBits, aExpectedError) { // add_cert_override except it may not be the case that the connection has an // SecInfo set on it. In this case, the error was not overridable anyway, so // we consider it a success. -function attempt_adding_cert_override(aHost, aExpectedBits, aSecurityInfo) { +function attempt_adding_cert_override(aHost, aSecurityInfo) { if (aSecurityInfo.serverCert) { - let bits = - (aSecurityInfo.isUntrusted - ? Ci.nsICertOverrideService.ERROR_UNTRUSTED - : 0) | - (aSecurityInfo.isDomainMismatch - ? Ci.nsICertOverrideService.ERROR_MISMATCH - : 0) | - (aSecurityInfo.isNotValidAtThisTime - ? Ci.nsICertOverrideService.ERROR_TIME - : 0); - Assert.equal( - bits, - aExpectedBits, - "Actual and expected override bits should match" - ); let cert = aSecurityInfo.serverCert; let certOverrideService = Cc[ "@mozilla.org/security/certoverride;1" ].getService(Ci.nsICertOverrideService); - certOverrideService.rememberValidityOverride( - aHost, - 8443, - {}, - cert, - aExpectedBits, - true - ); + certOverrideService.rememberValidityOverride(aHost, 8443, {}, cert, true); } } -// Given a host, expected error bits (see nsICertOverrideService.idl), and -// an expected error code, tests that an initial connection to the host fails -// with the expected errors and that adding an override does not result in a -// subsequent connection succeeding (i.e. the same error code is encountered). +// Given a host and an expected error code, tests that an initial connection to +// the host fails with the expected error and that adding an override does not +// result in a subsequent connection succeeding (i.e. the same error code is +// encountered). // The idea here is that for HSTS hosts or hosts with key pins, no error is // overridable, even if an entry is added to the override service. -function add_prevented_cert_override_test( - aHost, - aExpectedBits, - aExpectedError -) { +function add_prevented_cert_override_test(aHost, aExpectedError) { add_connection_test( aHost, aExpectedError, null, - attempt_adding_cert_override.bind(this, aHost, aExpectedBits) + attempt_adding_cert_override.bind(this, aHost) ); add_connection_test(aHost, aExpectedError); } diff --git a/security/manager/ssl/tests/unit/test_cert_override_bits_mismatches.js b/security/manager/ssl/tests/unit/test_cert_override_bits_mismatches.js deleted file mode 100644 index 0e27a86f9de7..000000000000 --- a/security/manager/ssl/tests/unit/test_cert_override_bits_mismatches.js +++ /dev/null @@ -1,117 +0,0 @@ -// -*- indent-tabs-mode: nil; js-indent-level: 2 -*- -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this -// file, You can obtain one at http://mozilla.org/MPL/2.0/. -"use strict"; - -// Test that when an override exists for a (host, certificate) pair, -// connections will succeed only if the set of error bits in the override -// contains each bit in the set of encountered error bits. -// Strangely, it is possible to store an override with an empty set of error -// bits, so this tests that too. - -do_get_profile(); - -function add_override_bits_mismatch_test( - host, - certPath, - expectedBits, - expectedError -) { - const MAX_BITS = - Ci.nsICertOverrideService.ERROR_UNTRUSTED | - Ci.nsICertOverrideService.ERROR_MISMATCH | - Ci.nsICertOverrideService.ERROR_TIME; - let cert = constructCertFromFile(certPath); - let certOverrideService = Cc[ - "@mozilla.org/security/certoverride;1" - ].getService(Ci.nsICertOverrideService); - for (let overrideBits = 0; overrideBits <= MAX_BITS; overrideBits++) { - add_test(function() { - certOverrideService.clearValidityOverride(host, 8443, {}); - certOverrideService.rememberValidityOverride( - host, - 8443, - {}, - cert, - overrideBits, - true - ); - run_next_test(); - }); - // The override will succeed only if the set of error bits in the override - // contains each bit in the set of expected error bits. - let successExpected = (overrideBits | expectedBits) == overrideBits; - add_connection_test( - host, - successExpected ? PRErrorCodeSuccess : expectedError - ); - } -} - -function run_test() { - Services.prefs.setIntPref("security.OCSP.enabled", 1); - add_tls_server_setup("BadCertAndPinningServer", "bad_certs"); - - let fakeOCSPResponder = new HttpServer(); - fakeOCSPResponder.registerPrefixHandler("/", function(request, response) { - response.setStatusLine(request.httpVersion, 500, "Internal Server Error"); - }); - fakeOCSPResponder.start(8888); - - add_override_bits_mismatch_test( - "unknownissuer.example.com", - "bad_certs/unknownissuer.pem", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, - SEC_ERROR_UNKNOWN_ISSUER - ); - add_override_bits_mismatch_test( - "mismatch.example.com", - "bad_certs/mismatch.pem", - Ci.nsICertOverrideService.ERROR_MISMATCH, - SSL_ERROR_BAD_CERT_DOMAIN - ); - add_override_bits_mismatch_test( - "expired.example.com", - "bad_certs/expired-ee.pem", - Ci.nsICertOverrideService.ERROR_TIME, - SEC_ERROR_EXPIRED_CERTIFICATE - ); - - add_override_bits_mismatch_test( - "mismatch-untrusted.example.com", - "bad_certs/mismatch-untrusted.pem", - Ci.nsICertOverrideService.ERROR_UNTRUSTED | - Ci.nsICertOverrideService.ERROR_MISMATCH, - SEC_ERROR_UNKNOWN_ISSUER - ); - add_override_bits_mismatch_test( - "untrusted-expired.example.com", - "bad_certs/untrusted-expired.pem", - Ci.nsICertOverrideService.ERROR_UNTRUSTED | - Ci.nsICertOverrideService.ERROR_TIME, - SEC_ERROR_UNKNOWN_ISSUER - ); - add_override_bits_mismatch_test( - "mismatch-expired.example.com", - "bad_certs/mismatch-expired.pem", - Ci.nsICertOverrideService.ERROR_MISMATCH | - Ci.nsICertOverrideService.ERROR_TIME, - SSL_ERROR_BAD_CERT_DOMAIN - ); - - add_override_bits_mismatch_test( - "mismatch-untrusted-expired.example.com", - "bad_certs/mismatch-untrusted-expired.pem", - Ci.nsICertOverrideService.ERROR_UNTRUSTED | - Ci.nsICertOverrideService.ERROR_MISMATCH | - Ci.nsICertOverrideService.ERROR_TIME, - SEC_ERROR_UNKNOWN_ISSUER - ); - - add_test(function() { - fakeOCSPResponder.stop(run_next_test); - }); - - run_next_test(); -} diff --git a/security/manager/ssl/tests/unit/test_cert_override_read.js b/security/manager/ssl/tests/unit/test_cert_override_read.js index d03a5ab46146..dce2ae96a6ed 100644 --- a/security/manager/ssl/tests/unit/test_cert_override_read.js +++ b/security/manager/ssl/tests/unit/test_cert_override_read.js @@ -38,58 +38,58 @@ function run_test() { "# This is a generated file! Do not edit.", "test.example.com:443:^privateBrowsingId=1\tOID.2.16.840.1.101.3.4.2.1\t" + cert1.sha256Fingerprint + - "\tM\t" + + "\t\t" + cert1.dbKey, "test.example.com:443:^privateBrowsingId=2\tOID.2.16.840.1.101.3.4.2.1\t" + + cert1.sha256Fingerprint + + "\t\t" + + cert1.dbKey, + "test.example.com:443:^privateBrowsingId=3\tOID.2.16.840.1.101.3.4.2.1\t" + // includes bits (now obsolete) cert1.sha256Fingerprint + "\tM\t" + cert1.dbKey, "example.com:443:\tOID.2.16.840.1.101.3.4.2.1\t" + cert2.sha256Fingerprint + - "\tU\t" + + "\t\t" + cert2.dbKey, "[::1]:443:\tOID.2.16.840.1.101.3.4.2.1\t" + // IPv6 cert2.sha256Fingerprint + - "\tM\t" + + "\t\t" + cert2.dbKey, "old.example.com:443\tOID.2.16.840.1.101.3.4.2.1\t" + // missing attributes (defaulted) cert1.sha256Fingerprint + - "\tM\t" + + "\t\t" + cert1.dbKey, ":443:\tOID.2.16.840.1.101.3.4.2.1\t" + // missing host name cert3.sha256Fingerprint + - "\tU\t" + + "\t\t" + cert3.dbKey, "example.com::\tOID.2.16.840.1.101.3.4.2.1\t" + // missing port cert3.sha256Fingerprint + - "\tU\t" + + "\t\t" + cert3.dbKey, "example.com:443:\tOID.2.16.840.1.101.3.4.2.1\t" + // wrong fingerprint/dbkey cert2.sha256Fingerprint + - "\tU\t" + + "\t\t" + cert3.dbKey, "example.com:443:\tOID.0.00.000.0.000.0.0.0.0\t" + // bad OID cert3.sha256Fingerprint + - "\tU\t" + + "\t\t" + cert3.dbKey, "example.com:443:\t.0.0.0.0\t" + // malformed OID cert3.sha256Fingerprint + - "\tU\t" + + "\t\t" + cert3.dbKey, "example.com:443:\t\t" + // missing OID cert3.sha256Fingerprint + - "\tU\t" + + "\t\t" + cert3.dbKey, "example.com:443:\tOID.2.16.840.1.101.3.4.2.1\t" + // missing fingerprint - "\tU\t" + - cert3.dbKey, - "example.com:443:\tOID.2.16.840.1.101.3.4.2.1\t" + // missing override bits - cert3.sha256Fingerprint + "\t\t" + cert3.dbKey, "example.com:443:\tOID.2.16.840.1.101.3.4.2.1\t" + // missing dbkey cert3.sha256Fingerprint + - "\tU\t", + "\t\t", ]; writeLinesAndClose(lines, outputStream); let overrideService = Cc["@mozilla.org/security/certoverride;1"].getService( @@ -116,42 +116,42 @@ function run_test() { host: "test.example.com", port: 443, cert: cert1, - bits: Ci.nsICertOverrideService.ERROR_MISMATCH, attributes: { privateBrowsingId: 1 }, }, { host: "test.example.com", port: 443, cert: cert1, - bits: Ci.nsICertOverrideService.ERROR_MISMATCH, attributes: { privateBrowsingId: 2 }, }, + { + host: "test.example.com", + port: 443, + cert: cert1, + attributes: { privateBrowsingId: 3 }, + }, { host: "example.com", port: 443, cert: cert2, - bits: Ci.nsICertOverrideService.ERROR_UNTRUSTED, attributes: {}, }, { host: "::1", port: 443, cert: cert2, - bits: Ci.nsICertOverrideService.ERROR_MISMATCH, attributes: {}, }, { host: "example.com", port: 443, cert: cert2, - bits: Ci.nsICertOverrideService.ERROR_UNTRUSTED, attributes: { userContextId: 1 }, // only privateBrowsingId is used }, { host: "old.example.com", port: 443, cert: cert1, - bits: Ci.nsICertOverrideService.ERROR_MISMATCH, attributes: {}, }, ]; @@ -160,36 +160,23 @@ function run_test() { host: "test.example.com", port: 443, cert: cert1, - bits: Ci.nsICertOverrideService.ERROR_MISMATCH, - attributes: { privateBrowsingId: 3 }, // wrong attributes + attributes: { privateBrowsingId: 4 }, // wrong attributes }, { host: "test.example.com", port: 443, cert: cert3, // wrong certificate - bits: Ci.nsICertOverrideService.ERROR_UNTRUSTED, attributes: { privateBrowsingId: 1 }, }, { host: "example.com", port: 443, cert: cert3, - bits: Ci.nsICertOverrideService.ERROR_UNTRUSTED, - attributes: {}, - }, - ]; - const BAD_BIT_OVERRIDES = [ - { - host: "example.com", - port: 443, - cert: cert2, - bits: Ci.nsICertOverrideService.ERROR_MISMATCH, // wrong bits attributes: {}, }, ]; for (let override of OVERRIDES) { - let actualBits = {}; let temp = {}; ok( overrideService.hasMatchingOverride( @@ -197,17 +184,14 @@ function run_test() { override.port, override.attributes, override.cert, - actualBits, temp ), `${JSON.stringify(override)} should have an override` ); - equal(actualBits.value, override.bits); equal(temp.value, false); } for (let override of BAD_OVERRIDES) { - let actualBits = {}; let temp = {}; ok( !overrideService.hasMatchingOverride( @@ -215,27 +199,9 @@ function run_test() { override.port, override.attributes, override.cert, - actualBits, temp ), `${override} should not have an override` ); } - - for (let override of BAD_BIT_OVERRIDES) { - let actualBits = {}; - let temp = {}; - ok( - overrideService.hasMatchingOverride( - override.host, - override.port, - override.attributes, - override.cert, - actualBits, - temp - ), - `${override} should have an override` - ); - notEqual(actualBits.value, override.bits); - } } diff --git a/security/manager/ssl/tests/unit/test_cert_overrides.js b/security/manager/ssl/tests/unit/test_cert_overrides.js index f780462bb578..bd4a4b96d860 100644 --- a/security/manager/ssl/tests/unit/test_cert_overrides.js +++ b/security/manager/ssl/tests/unit/test_cert_overrides.js @@ -7,8 +7,8 @@ // Tests the certificate overrides we allow. // add_cert_override_test will queue a test that does the following: // 1. Attempt to connect to the given host. This should fail with the -// given error and override bits. -// 2. Add an override for that host/port/certificate/override bits. +// given error. +// 2. Add an override for that host/port/certificate. // 3. Connect again. This should succeed. do_get_profile(); @@ -55,12 +55,12 @@ function check_telemetry() { ); equal( histogram.values[9], - 13, + 9, "Actual and expected SSL_ERROR_BAD_CERT_DOMAIN values should match" ); equal( histogram.values[10], - 5, + 1, "Actual and expected SEC_ERROR_EXPIRED_CERTIFICATE values should match" ); equal( @@ -80,7 +80,7 @@ function check_telemetry() { ); equal( histogram.values[14], - 2, + 1, "Actual and expected MOZILLA_PKIX_ERROR_NOT_YET_VALID_CERTIFICATE values should match" ); equal( @@ -90,7 +90,7 @@ function check_telemetry() { ); equal( histogram.values[16], - 3, + 2, "Actual and expected SEC_ERROR_INVALID_TIME values should match" ); equal( @@ -146,17 +146,14 @@ function run_port_equivalency_test(inPort, outPort) { "@mozilla.org/security/certoverride;1" ].getService(Ci.nsICertOverrideService); let cert = constructCertFromFile("bad_certs/default-ee.pem"); - let expectedBits = Ci.nsICertOverrideService.ERROR_UNTRUSTED; let expectedTemporary = true; certOverrideService.rememberValidityOverride( "example.com", inPort, {}, cert, - expectedBits, expectedTemporary ); - let actualBits = {}; let actualTemporary = {}; Assert.ok( certOverrideService.hasMatchingOverride( @@ -164,30 +161,17 @@ function run_port_equivalency_test(inPort, outPort) { outPort, {}, cert, - actualBits, actualTemporary ), `override set on port ${inPort} should match port ${outPort}` ); - equal( - actualBits.value, - expectedBits, - "input override bits should match output bits" - ); equal( actualTemporary.value, expectedTemporary, "input override temporary value should match output temporary value" ); Assert.ok( - !certOverrideService.hasMatchingOverride( - "example.com", - 563, - {}, - cert, - {}, - {} - ), + !certOverrideService.hasMatchingOverride("example.com", 563, {}, cert, {}), `override set on port ${inPort} should not match port 563` ); certOverrideService.clearValidityOverride("example.com", inPort, {}); @@ -197,12 +181,10 @@ function run_port_equivalency_test(inPort, outPort) { outPort, {}, cert, - actualBits, {} ), `override cleared on port ${inPort} should match port ${outPort}` ); - equal(actualBits.value, 0, "should have no bits set if there is no override"); } function run_test() { @@ -231,98 +213,63 @@ function run_test() { } function add_simple_tests() { - add_cert_override_test( - "expired.example.com", - Ci.nsICertOverrideService.ERROR_TIME, - SEC_ERROR_EXPIRED_CERTIFICATE - ); + add_cert_override_test("expired.example.com", SEC_ERROR_EXPIRED_CERTIFICATE); add_cert_override_test( "notyetvalid.example.com", - Ci.nsICertOverrideService.ERROR_TIME, MOZILLA_PKIX_ERROR_NOT_YET_VALID_CERTIFICATE ); - add_cert_override_test( - "before-epoch.example.com", - Ci.nsICertOverrideService.ERROR_TIME, - SEC_ERROR_INVALID_TIME - ); + add_cert_override_test("before-epoch.example.com", SEC_ERROR_INVALID_TIME); add_cert_override_test( "before-epoch-self-signed.example.com", - Ci.nsICertOverrideService.ERROR_TIME | - Ci.nsICertOverrideService.ERROR_UNTRUSTED, MOZILLA_PKIX_ERROR_SELF_SIGNED_CERT ); add_cert_override_test( "selfsigned.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, MOZILLA_PKIX_ERROR_SELF_SIGNED_CERT ); - add_cert_override_test( - "unknownissuer.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, - SEC_ERROR_UNKNOWN_ISSUER - ); + add_cert_override_test("unknownissuer.example.com", SEC_ERROR_UNKNOWN_ISSUER); add_cert_override_test( "expiredissuer.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE ); add_cert_override_test( "notyetvalidissuer.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, MOZILLA_PKIX_ERROR_NOT_YET_VALID_ISSUER_CERTIFICATE ); add_cert_override_test( "before-epoch-issuer.example.com", - Ci.nsICertOverrideService.ERROR_TIME, SEC_ERROR_INVALID_TIME ); add_cert_override_test( "md5signature.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED ); add_cert_override_test( "emptyissuername.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, MOZILLA_PKIX_ERROR_EMPTY_ISSUER_NAME ); // This has name information in the subject alternative names extension, // but not the subject common name. - add_cert_override_test( - "mismatch.example.com", - Ci.nsICertOverrideService.ERROR_MISMATCH, - SSL_ERROR_BAD_CERT_DOMAIN - ); + add_cert_override_test("mismatch.example.com", SSL_ERROR_BAD_CERT_DOMAIN); // This has name information in the subject common name but not the subject // alternative names extension. - add_cert_override_test( - "mismatch-CN.example.com", - Ci.nsICertOverrideService.ERROR_MISMATCH, - SSL_ERROR_BAD_CERT_DOMAIN - ); + add_cert_override_test("mismatch-CN.example.com", SSL_ERROR_BAD_CERT_DOMAIN); // A Microsoft IIS utility generates self-signed certificates with // properties similar to the one this "host" will present. add_cert_override_test( "selfsigned-inadequateEKU.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, MOZILLA_PKIX_ERROR_SELF_SIGNED_CERT ); add_prevented_cert_override_test( "inadequatekeyusage.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, SEC_ERROR_INADEQUATE_KEY_USAGE ); // Test triggering the MitM detection. We don't set-up a proxy here. Just // set the pref. Without the pref set we expect an unkown issuer error. - add_cert_override_test( - "mitm.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, - SEC_ERROR_UNKNOWN_ISSUER - ); + add_cert_override_test("mitm.example.com", SEC_ERROR_UNKNOWN_ISSUER); add_test(function() { Services.prefs.setStringPref( "security.pki.mitm_canary_issuer", @@ -334,11 +281,7 @@ function add_simple_tests() { certOverrideService.clearValidityOverride("mitm.example.com", 8443, {}); run_next_test(); }); - add_cert_override_test( - "mitm.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, - MOZILLA_PKIX_ERROR_MITM_DETECTED - ); + add_cert_override_test("mitm.example.com", MOZILLA_PKIX_ERROR_MITM_DETECTED); add_test(function() { Services.prefs.setStringPref( "security.pki.mitm_canary_issuer", @@ -352,11 +295,7 @@ function add_simple_tests() { }); // If the canary issuer doesn't match the one we see, we exepct and unknown // issuer error. - add_cert_override_test( - "mitm.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, - SEC_ERROR_UNKNOWN_ISSUER - ); + add_cert_override_test("mitm.example.com", SEC_ERROR_UNKNOWN_ISSUER); // If security.pki.mitm_canary_issuer.enabled is false, there should always // be an unknown issuer error. add_test(function() { @@ -370,11 +309,7 @@ function add_simple_tests() { certOverrideService.clearValidityOverride("mitm.example.com", 8443, {}); run_next_test(); }); - add_cert_override_test( - "mitm.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, - SEC_ERROR_UNKNOWN_ISSUER - ); + add_cert_override_test("mitm.example.com", SEC_ERROR_UNKNOWN_ISSUER); add_test(function() { Services.prefs.clearUserPref("security.pki.mitm_canary_issuer"); run_next_test(); @@ -391,7 +326,6 @@ function add_simple_tests() { }); add_prevented_cert_override_test( "nsCertTypeCritical.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, SEC_ERROR_UNKNOWN_CRITICAL_EXTENSION ); add_test(function() { @@ -405,13 +339,11 @@ function add_simple_tests() { // is a scenario in which an override is allowed. add_cert_override_test( "self-signed-end-entity-with-cA-true.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, MOZILLA_PKIX_ERROR_SELF_SIGNED_CERT ); add_cert_override_test( "ca-used-as-end-entity.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, MOZILLA_PKIX_ERROR_CA_CERT_USED_AS_END_ENTITY ); @@ -419,7 +351,6 @@ function add_simple_tests() { // encounter an overridable error. add_cert_override_test( "end-entity-issued-by-v1-cert.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, MOZILLA_PKIX_ERROR_V1_CERT_USED_AS_CA ); // If we make that certificate a trust anchor, the connection will succeed. @@ -453,36 +384,27 @@ function add_simple_tests() { // certificates that are not valid CAs. add_cert_override_test( "end-entity-issued-by-non-CA.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, SEC_ERROR_CA_CERT_INVALID ); // This host presents a 1016-bit RSA key. add_cert_override_test( "inadequate-key-size-ee.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, MOZILLA_PKIX_ERROR_INADEQUATE_KEY_SIZE ); add_cert_override_test( "ipAddressAsDNSNameInSAN.example.com", - Ci.nsICertOverrideService.ERROR_MISMATCH, - SSL_ERROR_BAD_CERT_DOMAIN - ); - add_cert_override_test( - "noValidNames.example.com", - Ci.nsICertOverrideService.ERROR_MISMATCH, SSL_ERROR_BAD_CERT_DOMAIN ); + add_cert_override_test("noValidNames.example.com", SSL_ERROR_BAD_CERT_DOMAIN); add_cert_override_test( "badSubjectAltNames.example.com", - Ci.nsICertOverrideService.ERROR_MISMATCH, SSL_ERROR_BAD_CERT_DOMAIN ); add_cert_override_test( "bug413909.xn--hxajbheg2az3al.xn--jxalpdlp", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, SEC_ERROR_UNKNOWN_ISSUER ); add_test(function() { @@ -501,7 +423,6 @@ function add_simple_tests() { 8443, {}, cert, - {}, {} ), "IDN certificate should have matching override using ascii host" @@ -513,7 +434,6 @@ function add_simple_tests() { 8443, {}, cert, - {}, {} ), /NS_ERROR_ILLEGAL_VALUE/, @@ -529,7 +449,6 @@ function add_simple_tests() { 8443, {}, cert, - {}, {} ), /NS_ERROR_ILLEGAL_VALUE/, @@ -544,24 +463,15 @@ function add_simple_tests() { "@mozilla.org/security/certoverride;1" ].getService(Ci.nsICertOverrideService); let cert = constructCertFromFile("bad_certs/default-ee.pem"); - let expectedBits = Ci.nsICertOverrideService.ERROR_UNTRUSTED; certOverrideService.rememberValidityOverride( "example.com", 443, {}, cert, - expectedBits, false ); Assert.ok( - certOverrideService.hasMatchingOverride( - "example.com", - 443, - {}, - cert, - {}, - {} - ), + certOverrideService.hasMatchingOverride("example.com", 443, {}, cert, {}), "Should have added override for example.com:443" ); certOverrideService.rememberValidityOverride( @@ -569,26 +479,11 @@ function add_simple_tests() { 80, {}, cert, - expectedBits, - false - ); - certOverrideService.rememberValidityOverride( - "::1", - 80, - {}, - cert, - expectedBits, false ); + certOverrideService.rememberValidityOverride("::1", 80, {}, cert, false); Assert.ok( - certOverrideService.hasMatchingOverride( - "example.com", - 80, - {}, - cert, - {}, - {} - ), + certOverrideService.hasMatchingOverride("example.com", 80, {}, cert, {}), "Should have added override for example.com:80" ); certOverrideService.rememberValidityOverride( @@ -596,22 +491,14 @@ function add_simple_tests() { 443, {}, cert, - expectedBits, false ); Assert.ok( - certOverrideService.hasMatchingOverride( - "example.org", - 443, - {}, - cert, - {}, - {} - ), + certOverrideService.hasMatchingOverride("example.org", 443, {}, cert, {}), "Should have added override for example.org:443" ); Assert.ok( - certOverrideService.hasMatchingOverride("::1", 80, {}, cert, {}, {}), + certOverrideService.hasMatchingOverride("::1", 80, {}, cert, {}), "Should have added override for [::1]:80" ); // When in a private browsing context, overrides added in non-private @@ -622,7 +509,6 @@ function add_simple_tests() { 443, { privateBrowsingId: 1 }, cert, - {}, {} ), "Should have override for example.org:443 with privateBrowsingId 1" @@ -633,7 +519,6 @@ function add_simple_tests() { 443, { privateBrowsingId: 2 }, cert, - {}, {} ), "Should have override for example.org:443 with privateBrowsingId 2" @@ -644,7 +529,6 @@ function add_simple_tests() { 443, { firstPartyDomain: "example.org", userContextId: 1 }, cert, - {}, {} ), "Should ignore firstPartyDomain and userContextId when checking overrides" @@ -654,18 +538,10 @@ function add_simple_tests() { 80, {}, cert, - expectedBits, true ); Assert.ok( - certOverrideService.hasMatchingOverride( - "example.org", - 80, - {}, - cert, - {}, - {} - ), + certOverrideService.hasMatchingOverride("example.org", 80, {}, cert, {}), "Should have added override for example.org:80" ); certOverrideService.rememberValidityOverride( @@ -673,7 +549,6 @@ function add_simple_tests() { 443, { firstPartyDomain: "example.org", userContextId: 1 }, cert, - expectedBits, false ); Assert.ok( @@ -682,7 +557,6 @@ function add_simple_tests() { 443, {}, cert, - {}, {} ), "Should ignore firstPartyDomain and userContextId when adding overrides" @@ -693,7 +567,6 @@ function add_simple_tests() { 443, { firstPartyDomain: "example.com", userContextId: 2 }, cert, - {}, {} ), "Should ignore firstPartyDomain and userContextId when checking overrides" @@ -703,7 +576,6 @@ function add_simple_tests() { 443, { privateBrowsingId: 1 }, cert, - expectedBits, false ); Assert.ok( @@ -712,7 +584,6 @@ function add_simple_tests() { 443, { privateBrowsingId: 1 }, cert, - {}, {} ), "Should have added override for example.test:443 with privateBrowsingId 1" @@ -723,7 +594,6 @@ function add_simple_tests() { 443, { privateBrowsingId: 2 }, cert, - {}, {} ), "Should not have override for example.test:443 with privateBrowsingId 2" @@ -734,7 +604,6 @@ function add_simple_tests() { 443, {}, cert, - {}, {} ), "Should not have override for example.test:443 with non-private OriginAttributes" @@ -749,20 +618,12 @@ function add_simple_tests() { 443, {}, cert, - {}, {} ), "Should have removed override for example.com:443" ); Assert.ok( - !certOverrideService.hasMatchingOverride( - "example.com", - 80, - {}, - cert, - {}, - {} - ), + !certOverrideService.hasMatchingOverride("example.com", 80, {}, cert, {}), "Should have removed override for example.com:80" ); Assert.ok( @@ -771,20 +632,12 @@ function add_simple_tests() { 443, {}, cert, - {}, {} ), "Should have removed override for example.org:443" ); Assert.ok( - !certOverrideService.hasMatchingOverride( - "example.org", - 80, - {}, - cert, - {}, - {} - ), + !certOverrideService.hasMatchingOverride("example.org", 80, {}, cert, {}), "Should have removed override for example.org:80" ); Assert.ok( @@ -793,7 +646,6 @@ function add_simple_tests() { 443, { privateBrowsingId: 1 }, cert, - {}, {} ), "Should have removed override for example.org:443 with privateBrowsingId 1" @@ -804,68 +656,40 @@ function add_simple_tests() { } function add_localhost_tests() { - add_cert_override_test( - "localhost", - Ci.nsICertOverrideService.ERROR_MISMATCH | - Ci.nsICertOverrideService.ERROR_UNTRUSTED, - SEC_ERROR_UNKNOWN_ISSUER - ); - add_cert_override_test( - "127.0.0.1", - Ci.nsICertOverrideService.ERROR_MISMATCH, - SSL_ERROR_BAD_CERT_DOMAIN - ); - add_cert_override_test( - "::1", - Ci.nsICertOverrideService.ERROR_MISMATCH, - SSL_ERROR_BAD_CERT_DOMAIN - ); + add_cert_override_test("localhost", SEC_ERROR_UNKNOWN_ISSUER); + add_cert_override_test("127.0.0.1", SSL_ERROR_BAD_CERT_DOMAIN); + add_cert_override_test("::1", SSL_ERROR_BAD_CERT_DOMAIN); } function add_combo_tests() { add_cert_override_test( "mismatch-expired.example.com", - Ci.nsICertOverrideService.ERROR_MISMATCH | - Ci.nsICertOverrideService.ERROR_TIME, SSL_ERROR_BAD_CERT_DOMAIN ); add_cert_override_test( "mismatch-notYetValid.example.com", - Ci.nsICertOverrideService.ERROR_MISMATCH | - Ci.nsICertOverrideService.ERROR_TIME, SSL_ERROR_BAD_CERT_DOMAIN ); add_cert_override_test( "mismatch-untrusted.example.com", - Ci.nsICertOverrideService.ERROR_MISMATCH | - Ci.nsICertOverrideService.ERROR_UNTRUSTED, SEC_ERROR_UNKNOWN_ISSUER ); add_cert_override_test( "untrusted-expired.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED | - Ci.nsICertOverrideService.ERROR_TIME, SEC_ERROR_UNKNOWN_ISSUER ); add_cert_override_test( "mismatch-untrusted-expired.example.com", - Ci.nsICertOverrideService.ERROR_MISMATCH | - Ci.nsICertOverrideService.ERROR_UNTRUSTED | - Ci.nsICertOverrideService.ERROR_TIME, SEC_ERROR_UNKNOWN_ISSUER ); add_cert_override_test( "md5signature-expired.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED | - Ci.nsICertOverrideService.ERROR_TIME, SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED ); add_cert_override_test( "ca-used-as-end-entity-name-mismatch.example.com", - Ci.nsICertOverrideService.ERROR_MISMATCH | - Ci.nsICertOverrideService.ERROR_UNTRUSTED, MOZILLA_PKIX_ERROR_CA_CERT_USED_AS_END_ENTITY ); } @@ -902,11 +726,7 @@ function add_distrust_test(certFileName, hostName, expectedResult) { clearSessionCache(); run_next_test(); }); - add_prevented_cert_override_test( - hostName, - Ci.nsICertOverrideService.ERROR_UNTRUSTED, - expectedResult - ); + add_prevented_cert_override_test(hostName, expectedResult); add_test(function() { setCertTrust(certToDistrust, "u,,"); run_next_test(); diff --git a/security/manager/ssl/tests/unit/test_cert_overrides_read_only.js b/security/manager/ssl/tests/unit/test_cert_overrides_read_only.js index 91fea0253722..80fa27a5d41e 100644 --- a/security/manager/ssl/tests/unit/test_cert_overrides_read_only.js +++ b/security/manager/ssl/tests/unit/test_cert_overrides_read_only.js @@ -9,53 +9,25 @@ // Helper function for add_read_only_cert_override_test. Probably doesn't need // to be called directly. -function add_read_only_cert_override(aHost, aExpectedBits, aSecurityInfo) { - let bits = - (aSecurityInfo.isUntrusted - ? Ci.nsICertOverrideService.ERROR_UNTRUSTED - : 0) | - (aSecurityInfo.isDomainMismatch - ? Ci.nsICertOverrideService.ERROR_MISMATCH - : 0) | - (aSecurityInfo.isNotValidAtThisTime - ? Ci.nsICertOverrideService.ERROR_TIME - : 0); - - Assert.equal( - bits, - aExpectedBits, - "Actual and expected override bits should match" - ); +function add_read_only_cert_override(aHost, aSecurityInfo) { let cert = aSecurityInfo.serverCert; let certOverrideService = Cc[ "@mozilla.org/security/certoverride;1" ].getService(Ci.nsICertOverrideService); // Setting the last argument to false here ensures that we attempt to store a // permanent override (which is what was failing in bug 1427273). - certOverrideService.rememberValidityOverride( - aHost, - 8443, - {}, - cert, - aExpectedBits, - false - ); + certOverrideService.rememberValidityOverride(aHost, 8443, {}, cert, false); } -// Given a host, expected error bits (see nsICertOverrideService.idl), and an -// expected error code, tests that an initial connection to the host fails with -// the expected errors and that adding an override results in a subsequent -// connection succeeding. -function add_read_only_cert_override_test( - aHost, - aExpectedBits, - aExpectedError -) { +// Given a host and an expected error code, tests that an initial connection to +// the host fails with the expected errors and that adding an override results +// in a subsequent connection succeeding. +function add_read_only_cert_override_test(aHost, aExpectedError) { add_connection_test( aHost, aExpectedError, null, - add_read_only_cert_override.bind(this, aHost, aExpectedBits) + add_read_only_cert_override.bind(this, aHost) ); add_connection_test(aHost, PRErrorCodeSuccess, null, aSecurityInfo => { Assert.ok( @@ -103,19 +75,14 @@ function run_test() { // set in addition to whatever other specific error bits are necessary. add_read_only_cert_override_test( "expired.example.com", - Ci.nsICertOverrideService.ERROR_TIME | - Ci.nsICertOverrideService.ERROR_UNTRUSTED, SEC_ERROR_UNKNOWN_ISSUER ); add_read_only_cert_override_test( "selfsigned.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, MOZILLA_PKIX_ERROR_SELF_SIGNED_CERT ); add_read_only_cert_override_test( "mismatch.example.com", - Ci.nsICertOverrideService.ERROR_MISMATCH | - Ci.nsICertOverrideService.ERROR_UNTRUSTED, SEC_ERROR_UNKNOWN_ISSUER ); diff --git a/security/manager/ssl/tests/unit/test_logoutAndTeardown.js b/security/manager/ssl/tests/unit/test_logoutAndTeardown.js index 728c7ef6999d..c3299a47c1da 100644 --- a/security/manager/ssl/tests/unit/test_logoutAndTeardown.js +++ b/security/manager/ssl/tests/unit/test_logoutAndTeardown.js @@ -160,17 +160,7 @@ function storeCertOverride(port, cert) { let certOverrideService = Cc[ "@mozilla.org/security/certoverride;1" ].getService(Ci.nsICertOverrideService); - let overrideBits = - Ci.nsICertOverrideService.ERROR_UNTRUSTED | - Ci.nsICertOverrideService.ERROR_MISMATCH; - certOverrideService.rememberValidityOverride( - hostname, - port, - {}, - cert, - overrideBits, - true - ); + certOverrideService.rememberValidityOverride(hostname, port, {}, cert, true); } function startClient(port) { diff --git a/security/manager/ssl/tests/unit/test_pinning.js b/security/manager/ssl/tests/unit/test_pinning.js index 44ea28731864..fef2cd935803 100644 --- a/security/manager/ssl/tests/unit/test_pinning.js +++ b/security/manager/ssl/tests/unit/test_pinning.js @@ -52,7 +52,6 @@ function test_strict() { // this host, we don't allow overrides. add_prevented_cert_override_test( "unknownissuer.include-subdomains.pinning.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, SEC_ERROR_UNKNOWN_ISSUER ); add_clear_override("unknownissuer.include-subdomains.pinning.example.com"); @@ -103,7 +102,6 @@ function test_strict() { // Similarly, this pin is in test-mode, so it should be overridable. add_cert_override_test( "unknownissuer.test-mode.pinning.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, SEC_ERROR_UNKNOWN_ISSUER ); add_clear_override("unknownissuer.test-mode.pinning.example.com"); @@ -132,7 +130,6 @@ function test_mitm() { // anchor, so we can't allow overrides for it). add_prevented_cert_override_test( "unknownissuer.include-subdomains.pinning.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, SEC_ERROR_UNKNOWN_ISSUER ); add_clear_override("unknownissuer.include-subdomains.pinning.example.com"); @@ -155,7 +152,6 @@ function test_mitm() { add_connection_test("test-mode.pinning.example.com", PRErrorCodeSuccess); add_cert_override_test( "unknownissuer.test-mode.pinning.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, SEC_ERROR_UNKNOWN_ISSUER ); add_clear_override("unknownissuer.test-mode.pinning.example.com"); @@ -192,13 +188,11 @@ function test_disabled() { add_cert_override_test( "unknownissuer.include-subdomains.pinning.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, SEC_ERROR_UNKNOWN_ISSUER ); add_clear_override("unknownissuer.include-subdomains.pinning.example.com"); add_cert_override_test( "unknownissuer.test-mode.pinning.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, SEC_ERROR_UNKNOWN_ISSUER ); add_clear_override("unknownissuer.test-mode.pinning.example.com"); @@ -215,7 +209,6 @@ function test_enforce_test_mode() { // this host, we don't allow overrides. add_prevented_cert_override_test( "unknownissuer.include-subdomains.pinning.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, SEC_ERROR_UNKNOWN_ISSUER ); add_clear_override("unknownissuer.include-subdomains.pinning.example.com"); @@ -259,7 +252,6 @@ function test_enforce_test_mode() { // this host (and since we're enforcing test mode), we don't allow overrides. add_prevented_cert_override_test( "unknownissuer.test-mode.pinning.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED, SEC_ERROR_UNKNOWN_ISSUER ); add_clear_override("unknownissuer.test-mode.pinning.example.com"); diff --git a/security/manager/ssl/tests/unit/test_session_resumption.js b/security/manager/ssl/tests/unit/test_session_resumption.js index 532ba88f70f3..3e95f73b75c1 100644 --- a/security/manager/ssl/tests/unit/test_session_resumption.js +++ b/security/manager/ssl/tests/unit/test_session_resumption.js @@ -31,11 +31,7 @@ addCertFromFile(certdb, "bad_certs/ev-test-intermediate.pem", ",,"); // information object. function add_resume_non_ev_with_override_test() { // This adds the override and makes one successful connection. - add_cert_override_test( - "expired.example.com", - Ci.nsICertOverrideService.ERROR_TIME, - SEC_ERROR_EXPIRED_CERTIFICATE - ); + add_cert_override_test("expired.example.com", SEC_ERROR_EXPIRED_CERTIFICATE); // This connects again, using session resumption. Note that we don't clear // the TLS session cache between these operations (that would defeat the diff --git a/security/manager/ssl/tests/unit/test_ssl_status.js b/security/manager/ssl/tests/unit/test_ssl_status.js index 16484ebb59aa..8e9aad40e030 100644 --- a/security/manager/ssl/tests/unit/test_ssl_status.js +++ b/security/manager/ssl/tests/unit/test_ssl_status.js @@ -66,7 +66,6 @@ function run_test() { }; add_cert_override_test( "expired.example.com", - Ci.nsICertOverrideService.ERROR_TIME, SEC_ERROR_EXPIRED_CERTIFICATE, undefined, overrideStatus diff --git a/security/manager/ssl/tests/unit/xpcshell.ini b/security/manager/ssl/tests/unit/xpcshell.ini index 3dff8dd714ef..2b7f0e33b7a4 100644 --- a/security/manager/ssl/tests/unit/xpcshell.ini +++ b/security/manager/ssl/tests/unit/xpcshell.ini @@ -81,8 +81,6 @@ run-if = nightly_build run-sequentially = hardcoded ports [test_cert_overrides_read_only.js] run-sequentially = hardcoded ports -[test_cert_override_bits_mismatches.js] -run-sequentially = hardcoded ports [test_cert_override_read.js] [test_cert_sha1.js] [test_cert_signatures.js] diff --git a/toolkit/components/cleardata/tests/unit/test_certs.js b/toolkit/components/cleardata/tests/unit/test_certs.js index b2d0de185ca6..4b2ba88b6677 100644 --- a/toolkit/components/cleardata/tests/unit/test_certs.js +++ b/toolkit/components/cleardata/tests/unit/test_certs.js @@ -30,7 +30,6 @@ add_task(async function() { TEST_URI.port, {}, cert, - {}, {} ), `Should not have override for ${TEST_URI.asciiHost}:${TEST_URI.port} yet` @@ -51,7 +50,6 @@ add_task(async function() { TEST_URI.port, {}, cert, - {}, {} ), `Should have override for ${TEST_URI.asciiHost}:${TEST_URI.port} now` @@ -75,7 +73,6 @@ add_task(async function() { TEST_URI.port, {}, cert, - {}, {} ), `Should not have override for ${TEST_URI.asciiHost}:${TEST_URI.port} now` @@ -96,7 +93,6 @@ add_task(async function() { uri.port, { privateBrowsingId: 1 }, cert, - {}, {} ), `Should have added override for ${uri.asciiHost}:${uri.port} with private browsing ID` @@ -107,7 +103,6 @@ add_task(async function() { uri.port, { privateBrowsingId: 2 }, cert, - {}, {} ), `Should not have added override for ${uri.asciiHost}:${uri.port} with private browsing ID 2` @@ -118,7 +113,6 @@ add_task(async function() { uri.port, {}, cert, - {}, {} ), `Should not have added override for ${uri.asciiHost}:${uri.port}` @@ -137,7 +131,6 @@ add_task(async function() { uri.port, {}, cert, - {}, {} ), `Should have added override for ${uri.asciiHost}:${uri.port}` @@ -158,7 +151,6 @@ add_task(async function() { uri.port, {}, cert, - {}, {} ), `Should have removed override for ${uri.asciiHost}:${uri.port}` @@ -169,7 +161,6 @@ add_task(async function() { uri.port, { privateBrowsingId: 1 }, cert, - {}, {} ), `Should have removed override for ${uri.asciiHost}:${uri.port} with private browsing attribute` @@ -197,27 +188,16 @@ add_task(async function test_deleteByBaseDomain() { let cert = certDB.constructX509FromBase64(CERT_TEST); ok(cert, "Cert was created"); - let overrideBits = - Ci.nsICertOverrideService.ERROR_UNTRUSTED | - Ci.nsICertOverrideService.ERROR_MISMATCH; - all.forEach(({ asciiHost, port }) => { Assert.ok( - !overrideService.hasMatchingOverride(asciiHost, port, {}, cert, {}, {}), + !overrideService.hasMatchingOverride(asciiHost, port, {}, cert, {}), `Should not have override for ${asciiHost}:${port} yet` ); - overrideService.rememberValidityOverride( - asciiHost, - port, - {}, - cert, - overrideBits, - false - ); + overrideService.rememberValidityOverride(asciiHost, port, {}, cert, false); Assert.ok( - overrideService.hasMatchingOverride(asciiHost, port, {}, cert, {}, {}), + overrideService.hasMatchingOverride(asciiHost, port, {}, cert, {}), `Should have override for ${asciiHost}:${port} now` ); }); @@ -236,14 +216,14 @@ add_task(async function test_deleteByBaseDomain() { toClear.forEach(({ asciiHost, port }) => Assert.ok( - !overrideService.hasMatchingOverride(asciiHost, port, {}, cert, {}, {}), + !overrideService.hasMatchingOverride(asciiHost, port, {}, cert, {}), `Should have cleared override for ${asciiHost}:${port}` ) ); toKeep.forEach(({ asciiHost, port }) => Assert.ok( - overrideService.hasMatchingOverride(asciiHost, port, {}, cert, {}, {}), + overrideService.hasMatchingOverride(asciiHost, port, {}, cert, {}), `Should have kept override for ${asciiHost}:${port}` ) ); diff --git a/toolkit/mozapps/extensions/test/browser/head.js b/toolkit/mozapps/extensions/test/browser/head.js index 86f791a2c840..12567108c1b6 100644 --- a/toolkit/mozapps/extensions/test/browser/head.js +++ b/toolkit/mozapps/extensions/test/browser/head.js @@ -589,7 +589,7 @@ CategoryUtilities.prototype = { // Returns a promise that will resolve when the certificate error override has been added, or reject // if there is some failure. -function addCertOverride(host, bits) { +function addCertOverride(host) { return new Promise((resolve, reject) => { let req = new XMLHttpRequest(); req.open("GET", "https://" + host + "/"); @@ -608,7 +608,6 @@ function addCertOverride(host, bits) { -1, {}, securityInfo.serverCert, - bits, false ); resolve(); @@ -624,22 +623,10 @@ function addCertOverride(host, bits) { // Returns a promise that will resolve when the necessary certificate overrides have been added. function addCertOverrides() { return Promise.all([ - addCertOverride( - "nocert.example.com", - Ci.nsICertOverrideService.ERROR_MISMATCH - ), - addCertOverride( - "self-signed.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED - ), - addCertOverride( - "untrusted.example.com", - Ci.nsICertOverrideService.ERROR_UNTRUSTED - ), - addCertOverride( - "expired.example.com", - Ci.nsICertOverrideService.ERROR_TIME - ), + addCertOverride("nocert.example.com"), + addCertOverride("self-signed.example.com"), + addCertOverride("untrusted.example.com"), + addCertOverride("expired.example.com"), ]); }