Oriol Brufau [:Oriol] Reporter
	Description • 7 years ago

Open browser console and enter this code:

  try { Cc["@mozilla.org/startupcache/cache;1"].createInstance(Ci.nsIPresentationDeviceManager); } catch(err){ }
  try { Cc["@mozilla.org/startupcache/cache;1"].createInstance(Ci.nsISupports); } catch(err){ }


Firefox crashes.

https://crash-stats.mozilla.com/report/index/21cac851-411c-4569-8bc5-0cc640171012
https://crash-stats.mozilla.com/report/index/c76f7076-eaab-4fc1-ab32-e46b10171012
https://crash-stats.mozilla.com/report/index/969d4a89-25d2-4707-a107-045b40171012
https://crash-stats.mozilla.com/report/index/dfbe4af0-3f01-4f78-bf45-cf9db0171012

	Oriol Brufau [:Oriol] Reporter
	Comment 1 • 7 years ago

This is a use after free, similar to bug 1407751 and bug 1407740.

	Andrew McCreight [:mccr8] Assignee
	Comment 3 • 7 years ago

FYI, you need to run ASan builds with something like
  ASAN_SYMBOLIZER_PATH=/usr/lib/llvm-3.8/bin/llvm-symbolizer
in order to get useful stacks. I'll run them myself and see what it turns up.

	Andrew McCreight [:mccr8] Assignee
	Comment 5 • 7 years ago

The free and allocation stacks from my log are bogus, but fortunately the use stack is not.

This is the same kind of issue as bug 1408005: gStartupCacheWrapper is a weak reference to a singleton StartupCacheWrapper. StartupCacheWrapper::GetSingleton() returns an already addrefed pointer to the wrapper. If the caller of GetSingleton() destroys the object, as happens when the QI fails in the XPCOM constructor, then the global variable contains a dead pointer. The fix is to clear the global in the dtor.

This seems very low risk. This class was only ever used for testing purposes, and bug 1314378 removed that, so it can be eliminated entirely. I think the path forward here is to fix the code as is, because the code is simple, and backport that. Then I'll file a separate bug to delete this entirely.

Unsurprisingly, I see no references to mozilla.org/startupcache/cache;1 in addons MXR.

	Nathan Froyd [:froydnj]
	Comment 6 • 7 years ago

We have an existing bug on getting rid of that test-only class somewhere, FWIW.

	Andrew McCreight [:mccr8] Assignee
	Comment 8 • 7 years ago

It seems improbably that we'd ever do something like comment 0, but if we did it would be a UAF, so I'm going to mark this sec-high.

	Nathan Froyd [:froydnj]
	Comment 9 • 7 years ago

Comment on attachment 8920640 [details] [diff] [review]
Clear gStartupCacheWrapper in the dtor.

Review of attachment 8920640 [details] [diff] [review]:
-----------------------------------------------------------------

WFM.

	Andrew McCreight [:mccr8] Assignee
	Comment 10 • 7 years ago

Comment on attachment 8920640 [details] [diff] [review]
Clear gStartupCacheWrapper in the dtor.

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not easily. It is probably not that hard to figure out what code triggers this, but that can only be run from chrome, so I'm not sure how you'd get an exploit out of it.

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

Which older supported branches are affected by this flaw? All.

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

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

How likely is this patch to cause regressions; how much testing does it need? This class is only used for testing in 52, and not used at all in 53+ so I can't imagine it will cause any real problems.

Approval Request Comment
[Feature/Bug causing the regression]: Very old.
[User impact if declined]: Maybe combined with a weird addon it could cause some security problems.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet.
[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?]: No.
[Why is the change risky/not risky?]: The class being changed is only used for testing on 52, and isn't used at all in 53+.
[String changes made/needed]: none

	Al Billings [:abillings - ex-MoCo]
	Comment 11 • 7 years ago

Comment on attachment 8920640 [details] [diff] [review]
Clear gStartupCacheWrapper in the dtor.

sec-approval+ for trunk. I don't think we need to take this in Beta or ESR52 (and I may regret typing that someday).

	Andrew McCreight [:mccr8] Assignee
	Comment 12 • 7 years ago

Comment on attachment 8920640 [details] [diff] [review]
Clear gStartupCacheWrapper in the dtor.

I think that's fine for beta. It would be nice to get on ESR, but it doesn't have to go there this release cycle, if we're near the end.

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 7 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/6df68bc48a0d768eb6f0fa058420740ba50fa139

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 7 years ago

https://hg.mozilla.org/mozilla-central/rev/6df68bc48a0d

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 15 • 7 years ago

Let's hold off then for 52.5.0esr if we're wontfixing this for 57.  We could consider it again for the next ESR.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 16 • 7 years ago

Going ahead with wontfix for esr52.  If anyone has strong objections please let me know.

	Andrew McCreight [:mccr8] Assignee
	Comment 17 • 7 years ago

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #15)
> Let's hold off then for 52.5.0esr if we're wontfixing this for 57.  We could
> consider it again for the next ESR.

That sounds fine. Is there any flag or whatever that I can set now so it will be automatically considered for the next ESR whenever people start worrying about that?

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 18 • 7 years ago

If it can wait till 59, then that is the next major ESR release. We do still have a week left before building ESR 52.5.0 if you want it to get fixed before 59.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 19 • 7 years ago

Setting tracking flags so we think about this again for ESR52.6.0.  There isn't all that much reason to wait, except that it seems odd to fix this in ESR now but not in the corresponding Firefox release (57)

	Andrew McCreight [:mccr8] Assignee
	Comment 20 • 7 years ago

Thanks. 52.6 is fine. I don't think there's a huge danger in leaving this as it is, which is why I think it is okay to not be in 57, but on the other hand the fix is so simple and low risk I don't want to leave it around for many release in ESR.

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 7 years ago

Comment on attachment 8920640 [details] [diff] [review]
Clear gStartupCacheWrapper in the dtor.

I can't reset the ESR52 approval flag, but I believe we want to approve this now for 52.6?

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 22 • 7 years ago

Comment on attachment 8920640 [details] [diff] [review]
Clear gStartupCacheWrapper in the dtor.

sec-high, ESR52+

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 7 years ago
uplift

https://hg.mozilla.org/releases/mozilla-esr52/rev/aa4b11615431

	Iulia Cristescu, QA [:JuliaC] (away, please needinfo? cornel.ionce@softvision.ro)
	Comment 24 • 7 years ago

I managed to reproduce the initial issue on 57.0.4 (20180103231032).  I also can confirm that 58.0 (20180115093319) and 52.6.0esr (20180116134019) builds are verified fixed using Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.13.2.