Closed Bug 1408017 Opened 2 years ago Closed 2 years ago

Crash with failed "@mozilla.org/startupcache/cache;1" instances

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla58
Tracking Status
firefox-esr52 58+ verified
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- verified

People

(Reporter: Oriol, Assigned: mccr8)

Details

(Keywords: crash, csectype-uaf, sec-high, Whiteboard: [adv-main58+][adv-esr52.6+][post-critsmash-triage])

Crash Data

Attachments

(2 files, 1 obsolete file)

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
This is a use after free, similar to bug 1407751 and bug 1407740.
Group: core-security
Keywords: csectype-uaf
Attached file asan log.txt (obsolete) —
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.
Attached file ASan log
Assignee: nobody → continuation
Attachment #8920312 - Attachment is obsolete: true
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.
We have an existing bug on getting rid of that test-only class somewhere, FWIW.
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.
Keywords: sec-high
Comment on attachment 8920640 [details] [diff] [review]
Clear gStartupCacheWrapper in the dtor.

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

WFM.
Attachment #8920640 - Flags: review?(nfroyd) → review+
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
Attachment #8920640 - Flags: sec-approval?
Attachment #8920640 - Flags: approval-mozilla-esr52?
Attachment #8920640 - Flags: approval-mozilla-beta?
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).
Attachment #8920640 - Flags: sec-approval? → sec-approval+
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.
Attachment #8920640 - Flags: approval-mozilla-beta?
Group: core-security → dom-core-security
https://hg.mozilla.org/mozilla-central/rev/6df68bc48a0d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Group: dom-core-security → core-security-release
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.
Going ahead with wontfix for esr52.  If anyone has strong objections please let me know.
Attachment #8920640 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52-
(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?
Flags: needinfo?(lhenry)
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.
Flags: needinfo?(lhenry) → needinfo?(continuation)
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)
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.
Flags: needinfo?(continuation)
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?
Flags: needinfo?(lhenry)
Attachment #8920640 - Flags: approval-mozilla-esr52- → approval-mozilla-esr52?
Flags: needinfo?(lhenry) → needinfo?(rkothari)
Flags: needinfo?(rkothari)
Comment on attachment 8920640 [details] [diff] [review]
Clear gStartupCacheWrapper in the dtor.

sec-high, ESR52+
Attachment #8920640 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Whiteboard: [adv-main58+][adv-esr52.6+]
Flags: qe-verify+
Whiteboard: [adv-main58+][adv-esr52.6+] → [adv-main58+][adv-esr52.6+][post-critsmash-triage]
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.