1496340 - Crash in nsSSLIOLayerHelpers::adjustForTLSIntolerance

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Calixte Denizet (:calixte)]

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

Comment 1

[J.C. Jones [:jcj] (he/him)]

Please take a look, Dana. The major NSS changes are moz::pkix->NSS and esni.

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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.

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: bug 1496340 - make sure each nsISupports is an nsIX509Cert in nsNSSCertList::Read r?jcj

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f59ae5c08ae1a700fb60d7af951028d5da96429a
https://hg.mozilla.org/mozilla-central/rev/f59ae5c08ae1

Comment 5

[Ryan VanderMeulen [:RyanVM]]

This grafts cleanly to Beta & ESR60 and looks like a pretty simple patch. Do you think it's worth taking on Beta & ESR60?

Comment 6

[J.C. Jones [:jcj] (he/him)]

I'll vote 'yes', but I defer to Dana.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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

Comment 8

[Pascal Chevrel:pascalc (PTO until April 26)]

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.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8b900972f8fe

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/9d51e65c797a