bug 1345612 - avoid calling NS_NewURI on IP addresses when checking certificate overrides r?Cykesiopka
When determining if a certificate error override is allowed for a host, we
consult nsISiteSecurityService::IsSecureURI to see if the host is HSTS/HPKP.
This API takes an nsIURI, but the calling code only has a hostname as an
nsCString. Calling NS_NewURI works in all situations we will encounter except
when the hostname is an IPv6 address. Since IP addresses are never HSTS/HPKP
anyway, we can skip the NS_NewURI / IsSecureURI calls in those cases as a
workaround.
MozReview-Commit-ID: JXa8cGvqqTA
--- a/security/manager/ssl/SSLServerCertVerification.cpp
+++ b/security/manager/ssl/SSLServerCertVerification.cpp
@@ -280,16 +280,17 @@ class CertErrorRunnable : public SyncRun
mProviderFlags(providerFlags)
{
}
virtual void RunOnTargetThread();
RefPtr<SSLServerCertVerificationResult> mResult; // out
private:
SSLServerCertVerificationResult* CheckCertOverrides();
+ nsresult OverrideAllowedForHost(/*out*/ bool& overrideAllowed);
const void* const mFdForLogging; // may become an invalid pointer; do not dereference
const nsCOMPtr<nsIX509Cert> mCert;
const RefPtr<nsNSSSocketInfo> mInfoObject;
const PRErrorCode mDefaultErrorCodeToReport;
const uint32_t mCollectedErrors;
const PRErrorCode mErrorCodeTrust;
const PRErrorCode mErrorCodeMismatch;
@@ -468,16 +469,83 @@ DetermineCertOverrideErrors(const Unique
PR_SetError(MapResultToPRErrorCode(result), 0);
return SECFailure;
}
}
return SECSuccess;
}
+// Helper function to determine if overrides are allowed for this host.
+// Overrides are not allowed for known HSTS or HPKP hosts. However, an IP
+// address is never considered an HSTS or HPKP host.
+nsresult
+CertErrorRunnable::OverrideAllowedForHost(/*out*/ bool& overrideAllowed)
+{
+ overrideAllowed = false;
+
+ // If this is an IP address, overrides are allowed, because an IP address is
+ // never an HSTS or HPKP host. nsISiteSecurityService takes this into account
+ // already, but the real problem here is that calling NS_NewURI with an IPv6
+ // address fails. We do this to avoid that. A more comprehensive fix would be
+ // to have Necko provide an nsIURI to PSM and to use that here (and
+ // everywhere). However, that would be a wide-spanning change.
+ const nsACString& hostname = mInfoObject->GetHostName();
+ if (net_IsValidIPv6Addr(hostname.BeginReading(), hostname.Length())) {
+ overrideAllowed = true;
+ return NS_OK;
+ }
+
+ // If this is an HTTP Strict Transport Security host or a pinned host and the
+ // certificate is bad, don't allow overrides (RFC 6797 section 12.1,
+ // HPKP draft spec section 2.6).
+ bool strictTransportSecurityEnabled = false;
+ bool hasPinningInformation = false;
+ nsCOMPtr<nsISiteSecurityService> sss(do_GetService(NS_SSSERVICE_CONTRACTID));
+ if (!sss) {
+ MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
+ ("[%p][%p] couldn't get nsISiteSecurityService to check HSTS/HPKP",
+ mFdForLogging, this));
+ return NS_ERROR_FAILURE;
+ }
+ nsCOMPtr<nsIURI> uri;
+ nsresult rv = NS_NewURI(getter_AddRefs(uri),
+ NS_LITERAL_CSTRING("https://") + hostname);
+ if (NS_FAILED(rv)) {
+ MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
+ ("[%p][%p] Creating new URI failed", mFdForLogging, this));
+ return rv;
+ }
+ rv = sss->IsSecureURI(nsISiteSecurityService::HEADER_HSTS,
+ uri,
+ mProviderFlags,
+ mInfoObject->GetOriginAttributes(),
+ nullptr,
+ &strictTransportSecurityEnabled);
+ if (NS_FAILED(rv)) {
+ MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
+ ("[%p][%p] checking for HSTS failed", mFdForLogging, this));
+ return rv;
+ }
+ rv = sss->IsSecureURI(nsISiteSecurityService::HEADER_HPKP,
+ uri,
+ mProviderFlags,
+ mInfoObject->GetOriginAttributes(),
+ nullptr,
+ &hasPinningInformation);
+ if (NS_FAILED(rv)) {
+ MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
+ ("[%p][%p] checking for HPKP failed", mFdForLogging, this));
+ return rv;
+ }
+
+ overrideAllowed = !strictTransportSecurityEnabled && !hasPinningInformation;
+ return NS_OK;
+}
+
SSLServerCertVerificationResult*
CertErrorRunnable::CheckCertOverrides()
{
MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("[%p][%p] top of CheckCertOverrides\n",
mFdForLogging, this));
// "Use" mFdForLogging in non-PR_LOGGING builds, too, to suppress
// clang's -Wunused-private-field build warning for this variable:
Unused << mFdForLogging;
@@ -492,87 +560,42 @@ CertErrorRunnable::CheckCertOverrides()
mInfoObject->GetPort(&port);
nsAutoCString hostWithPortString(mInfoObject->GetHostName());
hostWithPortString.Append(':');
hostWithPortString.AppendInt(port);
uint32_t remaining_display_errors = mCollectedErrors;
-
- // If this is an HTTP Strict Transport Security host or a pinned host and the
- // certificate is bad, don't allow overrides (RFC 6797 section 12.1,
- // HPKP draft spec section 2.6).
- bool strictTransportSecurityEnabled = false;
- bool hasPinningInformation = false;
- nsCOMPtr<nsISiteSecurityService> sss(do_GetService(NS_SSSERVICE_CONTRACTID));
- if (!sss) {
- MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
- ("[%p][%p] couldn't get nsISiteSecurityService to check for HSTS/HPKP\n",
- mFdForLogging, this));
- return new SSLServerCertVerificationResult(mInfoObject,
- mDefaultErrorCodeToReport);
- }
- nsCOMPtr<nsIURI> uri;
- nsresult nsrv = NS_NewURI(getter_AddRefs(uri),
- NS_LITERAL_CSTRING("https://") +
- mInfoObject->GetHostName());
- if (NS_FAILED(nsrv)) {
- MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
- ("[%p][%p] Creating new URI failed\n", mFdForLogging, this));
- return new SSLServerCertVerificationResult(mInfoObject,
- mDefaultErrorCodeToReport);
- }
- nsrv = sss->IsSecureURI(nsISiteSecurityService::HEADER_HSTS,
- uri,
- mProviderFlags,
- mInfoObject->GetOriginAttributes(),
- nullptr,
- &strictTransportSecurityEnabled);
- if (NS_FAILED(nsrv)) {
- MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
- ("[%p][%p] checking for HSTS failed\n", mFdForLogging, this));
- return new SSLServerCertVerificationResult(mInfoObject,
- mDefaultErrorCodeToReport);
- }
- nsrv = sss->IsSecureURI(nsISiteSecurityService::HEADER_HPKP,
- uri,
- mProviderFlags,
- mInfoObject->GetOriginAttributes(),
- nullptr,
- &hasPinningInformation);
- if (NS_FAILED(nsrv)) {
- MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
- ("[%p][%p] checking for HPKP failed\n", mFdForLogging, this));
+ bool overrideAllowed;
+ if (NS_FAILED(OverrideAllowedForHost(overrideAllowed))) {
return new SSLServerCertVerificationResult(mInfoObject,
mDefaultErrorCodeToReport);
}
- if (!strictTransportSecurityEnabled && !hasPinningInformation) {
+ if (overrideAllowed) {
MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
("[%p][%p] no HSTS or HPKP - overrides allowed\n",
mFdForLogging, this));
nsCOMPtr<nsICertOverrideService> overrideService =
do_GetService(NS_CERTOVERRIDE_CONTRACTID);
// it is fine to continue without the nsICertOverrideService
uint32_t overrideBits = 0;
- if (overrideService)
- {
+ if (overrideService) {
bool haveOverride;
bool isTemporaryOverride; // we don't care
const nsACString& hostString(mInfoObject->GetHostName());
- nsrv = overrideService->HasMatchingOverride(hostString, port,
- mCert,
- &overrideBits,
- &isTemporaryOverride,
- &haveOverride);
- if (NS_SUCCEEDED(nsrv) && haveOverride)
- {
+ nsresult rv = overrideService->HasMatchingOverride(hostString, port,
+ mCert,
+ &overrideBits,
+ &isTemporaryOverride,
+ &haveOverride);
+ if (NS_SUCCEEDED(rv) && haveOverride) {
// remove the errors that are already overriden
remaining_display_errors &= ~overrideBits;
}
}
if (!remaining_display_errors) {
// This can double- or triple-count one certificate with multiple
// different types of errors. Since this is telemetry and we just
@@ -616,18 +639,18 @@ CertErrorRunnable::CheckCertOverrides()
nsCOMPtr<nsIInterfaceRequestor> cb;
sslSocketControl->GetNotificationCallbacks(getter_AddRefs(cb));
if (cb) {
nsCOMPtr<nsIBadCertListener2> bcl = do_GetInterface(cb);
if (bcl) {
nsIInterfaceRequestor* csi
= static_cast<nsIInterfaceRequestor*>(mInfoObject);
bool suppressMessage = false; // obsolete, ignored
- nsrv = bcl->NotifyCertProblem(csi, mInfoObject->SSLStatus(),
- hostWithPortString, &suppressMessage);
+ Unused << bcl->NotifyCertProblem(csi, mInfoObject->SSLStatus(),
+ hostWithPortString, &suppressMessage);
}
}
}
// pick the error code to report by priority
PRErrorCode errorCodeToReport = mErrorCodeTrust ? mErrorCodeTrust
: mErrorCodeMismatch ? mErrorCodeMismatch
: mErrorCodeTime ? mErrorCodeTime
--- a/security/manager/ssl/tests/unit/test_cert_overrides.js
+++ b/security/manager/ssl/tests/unit/test_cert_overrides.js
@@ -14,31 +14,31 @@
do_get_profile();
function check_telemetry() {
let histogram = Cc["@mozilla.org/base/telemetry;1"]
.getService(Ci.nsITelemetry)
.getHistogramById("SSL_CERT_ERROR_OVERRIDES")
.snapshot();
equal(histogram.counts[0], 0, "Should have 0 unclassified counts");
- equal(histogram.counts[2], 8,
+ equal(histogram.counts[2], 9,
"Actual and expected SEC_ERROR_UNKNOWN_ISSUER counts should match");
equal(histogram.counts[3], 1,
"Actual and expected SEC_ERROR_CA_CERT_INVALID counts should match");
equal(histogram.counts[4], 0,
"Actual and expected SEC_ERROR_UNTRUSTED_ISSUER counts should match");
equal(histogram.counts[5], 1,
"Actual and expected SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE counts should match");
equal(histogram.counts[6], 0,
"Actual and expected SEC_ERROR_UNTRUSTED_CERT counts should match");
equal(histogram.counts[7], 0,
"Actual and expected SEC_ERROR_INADEQUATE_KEY_USAGE counts should match");
equal(histogram.counts[8], 2,
"Actual and expected SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED counts should match");
- equal(histogram.counts[9], 10,
+ equal(histogram.counts[9], 13,
"Actual and expected SSL_ERROR_BAD_CERT_DOMAIN counts should match");
equal(histogram.counts[10], 5,
"Actual and expected SEC_ERROR_EXPIRED_CERTIFICATE counts should match");
equal(histogram.counts[11], 2,
"Actual and expected MOZILLA_PKIX_ERROR_CA_CERT_USED_AS_END_ENTITY counts should match");
equal(histogram.counts[12], 1,
"Actual and expected MOZILLA_PKIX_ERROR_V1_CERT_USED_AS_CA counts should match");
equal(histogram.counts[13], 1,
@@ -53,21 +53,21 @@ function check_telemetry() {
"Actual and expected MOZILLA_PKIX_ERROR_EMPTY_ISSUER_NAME counts should match");
let keySizeHistogram = Cc["@mozilla.org/base/telemetry;1"]
.getService(Ci.nsITelemetry)
.getHistogramById("CERT_CHAIN_KEY_SIZE_STATUS")
.snapshot();
equal(keySizeHistogram.counts[0], 0,
"Actual and expected unchecked key size counts should match");
- equal(keySizeHistogram.counts[1], 12,
+ equal(keySizeHistogram.counts[1], 16,
"Actual and expected successful verifications of 2048-bit keys should match");
equal(keySizeHistogram.counts[2], 0,
"Actual and expected successful verifications of 1024-bit keys should match");
- equal(keySizeHistogram.counts[3], 58,
+ equal(keySizeHistogram.counts[3], 60,
"Actual and expected verification failures unrelated to key size should match");
run_next_test();
}
// Internally, specifying "port" -1 is the same as port 443. This tests that.
function run_port_equivalency_test(inPort, outPort) {
Assert.ok((inPort == 443 && outPort == -1) || (inPort == -1 && outPort == 443),
@@ -108,16 +108,17 @@ function run_test() {
let fakeOCSPResponder = new HttpServer();
fakeOCSPResponder.registerPrefixHandler("/", function (request, response) {
response.setStatusLine(request.httpVersion, 500, "Internal Server Error");
});
fakeOCSPResponder.start(8888);
add_simple_tests();
+ add_localhost_tests();
add_combo_tests();
add_distrust_tests();
add_test(function () {
fakeOCSPResponder.stop(check_telemetry);
});
run_next_test();
@@ -266,16 +267,29 @@ function add_simple_tests() {
Assert.ok(certOverrideService.hasMatchingOverride(uri.asciiHost, 8443, cert, {}, {}),
"IDN certificate should have matching override using ascii host");
Assert.ok(!certOverrideService.hasMatchingOverride(uri.host, 8443, cert, {}, {}),
"IDN certificate should not have matching override using (non-ascii) host");
run_next_test();
});
}
+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);
+}
+
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,
/The certificate is only valid for <a id="cert_domain_link" title="doesntmatch\.example\.com">doesntmatch\.example\.com<\/a>/);
add_cert_override_test("mismatch-notYetValid.example.com",
Ci.nsICertOverrideService.ERROR_MISMATCH |
--- a/security/manager/ssl/tests/unit/tlsserver/cmd/BadCertServer.cpp
+++ b/security/manager/ssl/tests/unit/tlsserver/cmd/BadCertServer.cpp
@@ -72,16 +72,17 @@ const BadCertHost sBadCertHosts[] =
{ "end-entity-issued-by-non-CA.example.com", "eeIssuedByNonCA" },
{ "inadequate-key-size-ee.example.com", "inadequateKeySizeEE" },
{ "badSubjectAltNames.example.com", "badSubjectAltNames" },
{ "ipAddressAsDNSNameInSAN.example.com", "ipAddressAsDNSNameInSAN" },
{ "noValidNames.example.com", "noValidNames" },
{ "bug413909.xn--hxajbheg2az3al.xn--jxalpdlp", "idn-certificate" },
{ "emptyissuername.example.com", "emptyIssuerName" },
{ "ev-test.example.com", "ev-test" },
+ { "localhost", "unknownissuer" },
{ nullptr, nullptr }
};
int32_t
DoSNISocketConfigBySubjectCN(PRFileDesc* aFd, const SECItem* aSrvNameArr,
uint32_t aSrvNameArrSize)
{
for (uint32_t i = 0; i < aSrvNameArrSize; i++) {