Crash in nsSSLIOLayerHelpers::adjustForTLSIntolerance

RESOLVED FIXED in Firefox -esr60

Status

()

defect
P1
critical
RESOLVED FIXED
11 months ago
12 days ago

People

(Reporter: calixte, Assigned: keeler)

Tracking

(Blocks 1 bug, {crash, regression, sec-moderate})

64 Branch
mozilla64
Unspecified
Windows 10
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr6063+ fixed, firefox62 wontfix, firefox63+ fixed, firefox64+ fixed)

Details

(Whiteboard: [psm-assigned][post-critsmash-triage][adv-main63+][adv-esr60.3+], crash signature)

Attachments

(1 attachment)

This bug was filed from the Socorro interface and is
report bp-06009b73-e113-49d3-ad99-eaaa40181004.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll void nsSSLIOLayerHelpers::adjustForTLSIntolerance security/manager/ssl/nsNSSIOLayer.cpp:913
1 xul.dll nsNSSCertificate::Release security/manager/ssl/nsNSSCertificate.cpp:82
2 xul.dll nsNSSCertList::ForEachCertificateInChain security/manager/ssl/nsNSSCertificate.cpp:1164
3 xul.dll static nsresult EvalChain security/manager/ssl/PublicKeyPinningService.cpp:114
4 xul.dll static nsresult CheckPinsForHostname security/manager/ssl/PublicKeyPinningService.cpp:276
5 xul.dll mozilla::psm::PublicKeyPinningService::ChainHasValidPins security/manager/ssl/PublicKeyPinningService.cpp:359
6 xul.dll mozilla::psm::NSSCertDBTrustDomain::IsChainValid security/certverifier/NSSCertDBTrustDomain.cpp:819
7 xul.dll static mozilla::pkix::Result mozilla::pkix::BuildForward security/nss/lib/mozpkix/lib/pkixbuild.cpp:338
8 xul.dll mozilla::pkix::PathBuildingStep::Check security/nss/lib/mozpkix/lib/pkixbuild.cpp:211
9 xul.dll static mozilla::pkix::Result mozilla::psm::FindIssuerInner security/certverifier/NSSCertDBTrustDomain.cpp:138

=============================================================

There are 14 crashes (from 1 installation) in nightly 64 with buildid 20181003100127. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1488622.

[1] https://hg.mozilla.org/mozilla-central/rev?node=7f966968076c
Flags: needinfo?(jjones)
Please take a look, Dana. The major NSS changes are moz::pkix->NSS and esni.
Flags: needinfo?(jjones) → needinfo?(dkeeler)
Well, there's a couple of things that seem wrong in nsNSSCertList. 1: nsNSSCertList::Read passes cert to nsNSSCertList::AddCert without checking that it actually got an nsIX509Cert (in which case that parameter will be null, which is important because...) 2: nsNSSCertList::AddCert doesn't null-check its input before calling nsIX509Cert::GetCert. I would expect this to manifest as a null or near-null dereference, but maybe with some optimizations the bad cert would survive long enough to get put in the list, whereupon when its destructor gets called in nsNSSCertList::ForEachCertificateInChain, it jumps to nsSSLIOLayerHelpers::adjustForTLSIntolerance via a coincidental pointer value?

I think nsNSSCertList::Read has been wrong since bug 1029155, when it was added. nsNSSCertList::AddCert has been unsafe in this way since at least the move to mercurial in 2007.

This is very similar to bug 1490585 in that it should only be exploitable if an attacker already has write access to the profile directory.
Assignee: nobody → dkeeler
Group: crypto-core-security
Flags: needinfo?(dkeeler)
Keywords: sec-moderate
Priority: -- → P1
Whiteboard: [psm-assigned]
This grafts cleanly to Beta & ESR60 and looks like a pretty simple patch. Do you think it's worth taking on Beta & ESR60?
Flags: needinfo?(dkeeler)
Flags: in-testsuite+
I'll vote 'yes', but I defer to Dana.
Comment on attachment 9014622 [details]
bug 1496340 - make sure each nsISupports is an nsIX509Cert in nsNSSCertList::Read r?jcj

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: bug 1029155

User impact if declined: Potential malicious code execution if an attacker already has write access to the profile directory.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: none

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This basically adds two null checks. It almost certainly can't be worse than what we have.

String changes made/needed: none

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: We've seen some crashes in the wild for (probably) this. It's a simple, low-risk patch.

User impact if declined: Potential malicious code execution if an attacker already has write access to the profile directory.

Fix Landed on Version: 64

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This basically adds two null checks. It almost certainly can't be worse than what we have.

String or UUID changes made by this patch: none
Flags: needinfo?(dkeeler)
Attachment #9014622 - Flags: approval-mozilla-esr60?
Attachment #9014622 - Flags: approval-mozilla-beta?
Comment on attachment 9014622 [details]
bug 1496340 - make sure each nsISupports is an nsIX509Cert in nsNSSCertList::Read r?jcj

Looks safe, uplift approved for 63 beta 14, thanks.
Attachment #9014622 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 9014622 [details]
bug 1496340 - make sure each nsISupports is an nsIX509Cert in nsNSSCertList::Read r?jcj

Fixes a crash with security implications that's been seen in the wild. Approved for ESR 60.3.
Attachment #9014622 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Group: crypto-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [psm-assigned] → [psm-assigned][post-critsmash-triage]
Whiteboard: [psm-assigned][post-critsmash-triage] → [psm-assigned][post-critsmash-triage][adv-main63+][adv-esr60.3+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.