Closed
Bug 1490585
(CVE-2018-12385)
Opened 6 years ago
Closed 6 years ago
Startup crash in nsSSLStatus::~nsSSLStatus after downgrading profile
Categories
(Core :: Security: PSM, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla64
People
(Reporter: philipp, Assigned: keeler)
References
Details
(4 keywords, Whiteboard: [psm-assigned][post-critsmash-triage])
Crash Data
Attachments
(1 file)
46 bytes,
text/x-phabricator-request
|
jcj
:
review+
pascalc
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-release+
RyanVM
:
approval-mozilla-esr60+
abillings
:
sec-approval+
|
Details | Review |
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
Comment 2•6 years ago
|
||
This is a call done from the Cache, maybe Michal can add something smart here?
Flags: needinfo?(daniel) → needinfo?(michal.novotny)
Reporter | ||
Comment 3•6 years ago
|
||
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.
Reporter | ||
Comment 4•6 years ago
|
||
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)
Reporter | ||
Updated•6 years ago
|
Summary: Startup crash in nsSSLStatus::~nsSSLStatus after Windows 2018-09 patchday → Startup crash in nsSSLStatus::~nsSSLStatus after downgrading profile
Assignee | ||
Comment 5•6 years ago
|
||
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)
Reporter | ||
Updated•6 years ago
|
Crash Signature: [@ nsSSLStatus::~nsSSLStatus] → [@ nsSSLStatus::~nsSSLStatus]
[@ Gecko_SetLengthString]
[@ nsNSSCertificate::GetIssuerCommonName]
[@ nsTSubstring<T>::SetCapacity]
[@ nsTSubstring<T>::SetLength]
[@ ReleaseData | std::vector<T>::_Tidy]
[@ nsSSLStatus::Release]
Assignee | ||
Comment 7•6 years ago
|
||
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
tracking-firefox62:
--- → ?
tracking-firefox63:
--- → ?
tracking-firefox-esr60:
--- → ?
Keywords: sec-high
Priority: -- → P1
Whiteboard: [psm-assigned]
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(michal.novotny)
Flags: needinfo?(bugzilla)
Updated•6 years ago
|
Group: network-core-security, core-security
Assignee | ||
Comment 9•6 years ago
|
||
Updated•6 years ago
|
Group: crypto-core-security → core-security-release
Comment 10•6 years ago
|
||
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+
Comment 11•6 years ago
|
||
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.
Assignee | ||
Comment 12•6 years ago
|
||
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?
Assignee | ||
Comment 13•6 years ago
|
||
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).
Updated•6 years ago
|
Comment 14•6 years ago
|
||
sec-approval+ for trunk.
Ryan, why are you tracking this for 62, which we already shipped?
Flags: needinfo?(ryanvm)
Updated•6 years ago
|
Attachment #9008888 -
Flags: sec-approval? → sec-approval+
Updated•6 years ago
|
Comment 16•6 years ago
|
||
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.
Comment 17•6 years ago
|
||
Please request Beta/Release/ESR60 approval on this.
Flags: needinfo?(dkeeler)
Assignee | ||
Comment 18•6 years ago
|
||
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 19•6 years ago
|
||
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 20•6 years ago
|
||
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+
Comment 21•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/45359b962851
https://hg.mozilla.org/releases/mozilla-release/rev/e9dd9434ad9a
https://hg.mozilla.org/releases/mozilla-esr60/rev/80a4a7ef2813
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Updated•6 years ago
|
Flags: qe-verify+
Whiteboard: [psm-assigned] → [psm-assigned][post-critsmash-triage]
Comment 23•6 years ago
|
||
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-high → sec-moderate
Comment 24•6 years ago
|
||
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
Assignee | ||
Comment 25•6 years ago
|
||
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)
Updated•6 years ago
|
Alias: CVE-2018-12385
Comment 26•6 years ago
|
||
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)
Updated•6 years ago
|
Flags: qe-verify+
Updated•5 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•