[:philipp] Reporter
	Description • 6 years ago

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

	Sylvestre Ledru [:Sylvestre]
	Comment 1 • 6 years ago

Any clue?

	Daniel Stenberg [:bagder]
	Comment 2 • 6 years ago

This is a call done from the Cache, maybe Michal can add something smart here?

	[:philipp] 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.

	[:philipp] 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.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) 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.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) 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.

	Pascal Chevrel:pascalc (PTO until April 26)
	Comment 8 • 6 years ago

Tracking for 63 as it is spiking since beta 4 with 250 crashes per day.

	J.C. Jones [:jcj] (he/him)
	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.

	J.C. Jones [:jcj] (he/him)
	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.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) 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.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) 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).

	Al Billings [:abillings - ex-MoCo]
	Comment 14 • 6 years ago

sec-approval+ for trunk.

Ryan, why are you tracking this for 62, which we already shipped?

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 6 years ago

Because it may end up riding in 62.0.2.

	Julien Cristau [:jcristau]
	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.

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 6 years ago

Please request Beta/Release/ESR60 approval on this.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) 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

	Pascal Chevrel:pascalc (PTO until April 26)
	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.

	Ryan VanderMeulen [:RyanVM]
	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.

	Ryan VanderMeulen [:RyanVM]
	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

	Daniel Veditz [:dveditz]
	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.

	Cristian Fogel [:cfogel]
	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?

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) 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.

	Cristian Fogel [:cfogel]
	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.