Closed Bug 1490585 (CVE-2018-12385) Opened 2 years ago Closed 2 years ago

Startup crash in nsSSLStatus::~nsSSLStatus after downgrading profile

Categories

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

62 Branch
All
Windows
defect

Tracking

()

VERIFIED FIXED
mozilla64
Tracking Status
firefox-esr60 62+ verified
firefox62 + verified
firefox63 + verified
firefox64 --- unaffected

People

(Reporter: philipp, Assigned: keeler)

References

Details

(4 keywords, Whiteboard: [psm-assigned][post-critsmash-triage])

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is
report bp-b21e8499-5fa4-4bed-9d81-2c19f0180912.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll nsSSLStatus::~nsSSLStatus security/manager/ssl/nsSSLStatus.h:34
1 xul.dll nsSSLStatus::Release security/manager/ssl/nsSSLStatus.cpp:387
2 xul.dll mozilla::psm::TransportSecurityInfo::~TransportSecurityInfo security/manager/ssl/TransportSecurityInfo.h:35
3 xul.dll mozilla::psm::TransportSecurityInfo::`scalar deleting destructor' 
4 xul.dll mozilla::psm::TransportSecurityInfo::Release security/manager/ssl/TransportSecurityInfo.cpp:50
5 xul.dll nsBinaryInputStream::ReadObject xpcom/io/nsBinaryStream.cpp:989
6 xul.dll NS_DeserializeObject netwerk/base/nsSerializationHelper.cpp:48
7 xul.dll mozilla::net::CacheEntry::GetSecurityInfo netwerk/cache2/CacheEntry.cpp:1365
8 xul.dll mozilla::net::CacheEntryHandle::GetSecurityInfo netwerk/cache2/CacheEntry.h:451
9 xul.dll mozilla::net::nsHttpChannel::OpenCacheInputStream netwerk/protocol/http/nsHttpChannel.cpp:4698

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

this startup crash signature is taking off in volume over the past hours:
https://crash-stats.mozilla.com/signature/?product=Firefox&signature=nsSSLStatus%3A%3A~nsSSLStatus&date=%3E%3D2018-08-29#graphs

we have users linking that to installing the latest windows updates from yesterday's windows updates which would fit timing-wise, as the signature started increasing then across multiple release channels:
https://support.mozilla.org/en-US/questions/1233386
Any clue?
Flags: needinfo?(dkeeler)
Flags: needinfo?(daniel)
See Also: → 1490523
This is a call done from the Cache, maybe Michal can add something smart here?
Flags: needinfo?(daniel) → needinfo?(michal.novotny)
See Also: → 1490653
the relation timing-wise to the windows patch day might be a red herring.
over in bug 1490653 and some other places affected people are saying the issue occurred after they moved with their profile last touched in a recent nightly to prior releases or even older nightly builds.
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fea371cafd2c73be689184ca6670e33c3fd1636c&tochange=423bdf is the pushlog where i suspect the issue was introduced - out of that range bug 1468222 seems the most likely regressing cause for this crash and related ones in bug 1490523.
Blocks: 1468222
Flags: needinfo?(bugzilla)
Summary: Startup crash in nsSSLStatus::~nsSSLStatus after Windows 2018-09 patchday → Startup crash in nsSSLStatus::~nsSSLStatus after downgrading profile
Yeah, we changed the serialization in bug 1468222. In theory earlier versions should just return a failing nsresult. I'm looking in to why this results in crashes instead.
Flags: needinfo?(dkeeler)
Duplicate of this bug: 1490653
Crash Signature: [@ nsSSLStatus::~nsSSLStatus] → [@ nsSSLStatus::~nsSSLStatus] [@ Gecko_SetLengthString] [@ nsNSSCertificate::GetIssuerCommonName] [@ nsTSubstring<T>::SetCapacity] [@ nsTSubstring<T>::SetLength] [@ ReleaseData | std::vector<T>::_Tidy] [@ nsSSLStatus::Release]
I think the problem is that this (in TransportSecurityInfo::Read) is not at all safe:

296  nsCOMPtr<nsISupports> supports;
297  rv = NS_ReadOptionalObject(stream, true, getter_AddRefs(supports));
298  if (NS_FAILED(rv)) {
299    return rv;
300  }
301  mSSLStatus = BitwiseCast<nsSSLStatus*, nsISupports*>(supports.get());

( starts at https://hg.mozilla.org/releases/mozilla-release/annotate/b54db6622358/security/manager/ssl/TransportSecurityInfo.cpp#l296 )

This reads something that is an nsISupports, but we have no idea what it is beyond that. Safe code would do_QueryInterface to nsISSLStatus (and if that results in a null pointer, it's not an nsISSLStatus), but this code wants the underlying implementation (nsSSLStatus) rather than the interface, so it just grabs the raw pointer. This appears to have ultimately been introduced in bug 262116 (in 2007).

(The reason this crashes is that when the TransportSecurityInfo is destructed, nsSSLStatus::~nsSSLStatus is called on mSSLStatus, which isn't actually an nsSSLStatus, and things start to go even more awry from there.)

While in theory this can result in arbitrary code execution, luckily the only inputs to this function *should* come from the user's profile directory (i.e. the cache/session restore/etc.) or privileged Firefox internals, so I don't think this is a remote code execution vulnerability. I'm rating this a sec-high based on that, but feel free to re-assess.

Ironically, bug 1468222 fixed this in Nightly by removing this code.
Assignee: nobody → dkeeler
Blocks: 262116
No longer blocks: 1468222
Keywords: sec-high
Priority: -- → P1
Whiteboard: [psm-assigned]
Flags: needinfo?(michal.novotny)
Flags: needinfo?(bugzilla)
Group: network-core-security, core-security
Tracking for 63 as it is spiking since beta 4 with 250 crashes per day.
Group: crypto-core-security → core-security-release
Comment on attachment 9008888 [details]
bug 1490585 - ensure we got an nsISSLStatus when deserializing in TransportSecurityInfo r?jcj

J.C. Jones [:jcj] (he/him) has approved the revision.
Attachment #9008888 - Flags: review+
If there's any way to misuse this, just saying: there's no way to patch this that doesn't look super-obvious about where the issue lies.
Comment on attachment 9008888 [details]
bug 1490585 - ensure we got an nsISSLStatus when deserializing in TransportSecurityInfo r?jcj

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Like J.C. said, it's pretty clear what's going on here. That said, I believe this would require another vulnerability to exploit. Something that gave an attacker write access to the cache directory in the user's profile would be enough, for example.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Yes, but then again, so does the patch itself.

Which older supported branches are affected by this flaw?

All of them.

If not all supported branches, which bug introduced the flaw?

n/a, but bug 262116

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

This applies to all affected branches.

How likely is this patch to cause regressions; how much testing does it need?

This should be strictly more correct than before, so it's unlikely to cause new regressions. Also, it does have an automated test, so we should be good there.
Attachment #9008888 - Flags: sec-approval?
I should note this is incidentally causing problems for people switching between nightly and release right now, because we changed the serialization in bug 1468222 (which is how we discovered this).
sec-approval+ for trunk.

Ryan, why are you tracking this for 62, which we already shipped?
Flags: needinfo?(ryanvm)
Attachment #9008888 - Flags: sec-approval? → sec-approval+
Because it may end up riding in 62.0.2.
Flags: needinfo?(ryanvm)
I got confused about status in 64.  This code was removed by https://hg.mozilla.org/mozilla-central/rev/5cfda4227c6a so I guess we're just missing uplift requests here.
Please request Beta/Release/ESR60 approval on this.
Flags: needinfo?(dkeeler)
Comment on attachment 9008888 [details]
bug 1490585 - ensure we got an nsISSLStatus when deserializing in TransportSecurityInfo r?jcj

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
User impact if declined: if a user switches to Nightly (64) and back using the same profile, Firefox will just perma-crash. Also if an attacker can write to the cache directory, they could parley that into arbitrary code execution.
Fix Landed on Version: n/a - the affected code (and thus the bug) was removed in bug 832834 by https://hg.mozilla.org/mozilla-central/rev/2f4adf14e623
Risk to taking this patch (and alternatives if risky): low - this almost certainly isn't worse. The alternative is uplifting bug 832834, which is a bad idea since it's much larger and riskier.
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: bug 262116
[User impact if declined]: if a user switches to Nightly (64) and back using the same profile, Firefox will just perma-crash. Also if an attacker can write to the cache directory, they could parley that into arbitrary code execution.
[Is this code covered by automated tests?]: includes a test, other tests also cover this area
[Has the fix been verified in Nightly?]: n/a - the affected code (and thus the bug) was removed in bug 832834 by https://hg.mozilla.org/mozilla-central/rev/2f4adf14e623
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not very
[Why is the change risky/not risky?]: it's the equivalent of adding a null-check
[String changes made/needed]: none
Flags: needinfo?(dkeeler)
Attachment #9008888 - Flags: approval-mozilla-release?
Attachment #9008888 - Flags: approval-mozilla-esr60?
Attachment #9008888 - Flags: approval-mozilla-beta?
Comment on attachment 9008888 [details]
bug 1490585 - ensure we got an nsISSLStatus when deserializing in TransportSecurityInfo r?jcj

Fix for a topcrash on beta, pproved for 63 Beta 8, thanks.
Attachment #9008888 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 9008888 [details]
bug 1490585 - ensure we got an nsISSLStatus when deserializing in TransportSecurityInfo r?jcj

Approved for 62.0.2 and ESR 60.2.1 also.
Attachment #9008888 - Flags: approval-mozilla-release?
Attachment #9008888 - Flags: approval-mozilla-release+
Attachment #9008888 - Flags: approval-mozilla-esr60?
Attachment #9008888 - Flags: approval-mozilla-esr60+
Flags: qe-verify+
Whiteboard: [psm-assigned] → [psm-assigned][post-critsmash-triage]
Duplicate of this bug: 1492491
Since it would require another bug to exploit (comment 12), or equivalently a rogue local program, to write to the cache directory  and further manifests as a start-up crash, sec-high overstates the severity. Going for sec-moderate.
Keywords: sec-highsec-moderate
Managed to reproduce using live builds: 64.0a1 -> 63.0b7, 62.0.
Fix verified on the following asan builds: 60.2.1esr(20180919011139), 62.0.2(20180919010706), 63.0b8(20180919010441).
Did not encounter any crash when starting with a new profile. 

However, I've noticed that with 62.0.2 and 63.0b8 if trying to access a profile that was opened with a build that crashed; causes these 2 to crash as well.
Scenario:
- create profile with 64;
- open with an affected build;
- open with one of the fixed builds

Do we consider it fixed for these 2(62,63) as well?
Status: RESOLVED → VERIFIED
Cristi, do you have links to crash reports for those? I tried creating a new profile in Nightly (64), opening that profile in 62 (it crashed), and then opening the same profile with the release version this patch landed in and it didn't crash for me.
Flags: needinfo?(cristian.fogel)
Alias: CVE-2018-12385
Good news, tried to get the crash signatures but the issue mentioned is no longer present on the 2 versions. 

Marking them as verified as well.
Flags: needinfo?(cristian.fogel)
Duplicate of this bug: 1495385
Flags: qe-verify+
Duplicate of this bug: 1509654
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.