Closed
Bug 1408017
Opened 7 years ago
Closed 7 years ago
Crash with failed "@mozilla.org/startupcache/cache;1" instances
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
VERIFIED
FIXED
mozilla58
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)
17.54 KB,
text/plain
|
Details | |
1.63 KB,
patch
|
froydnj
:
review+
ritu
:
approval-mozilla-esr52+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•7 years ago
|
||
This is a use after free, similar to bug 1407751 and bug 1407740.
Group: core-security
Keywords: csectype-uaf
Reporter | ||
Comment 2•7 years ago
|
||
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.
Assignee | ||
Comment 4•7 years ago
|
||
Assignee: nobody → continuation
Attachment #8920312 -
Attachment is obsolete: true
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.
![]() |
||
Comment 6•7 years ago
|
||
We have an existing bug on getting rid of that test-only class somewhere, FWIW.
Assignee | ||
Comment 7•7 years ago
|
||
Attachment #8920640 -
Flags: review?(nfroyd)
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.
Keywords: sec-high
![]() |
||
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.
Attachment #8920640 -
Flags: review?(nfroyd) → review+
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
Attachment #8920640 -
Flags: sec-approval?
Attachment #8920640 -
Flags: approval-mozilla-esr52?
Attachment #8920640 -
Flags: approval-mozilla-beta?
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).
Attachment #8920640 -
Flags: sec-approval? → sec-approval+
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.
Attachment #8920640 -
Flags: approval-mozilla-beta?
Assignee | ||
Updated•7 years ago
|
status-firefox56:
--- → wontfix
status-firefox57:
--- → wontfix
status-firefox58:
--- → affected
status-firefox-esr52:
--- → affected
Assignee | ||
Updated•7 years ago
|
Group: core-security → dom-core-security
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 13•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6df68bc48a0d768eb6f0fa058420740ba50fa139
Keywords: checkin-needed
Comment 14•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6df68bc48a0d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Updated•7 years ago
|
Group: dom-core-security → core-security-release
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.
Comment 16•7 years ago
|
||
Going ahead with wontfix for esr52. If anyone has strong objections please let me know.
Updated•7 years ago
|
Attachment #8920640 -
Flags: approval-mozilla-esr52? → approval-mozilla-esr52-
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?
Flags: needinfo?(lhenry)
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.
Flags: needinfo?(lhenry) → needinfo?(continuation)
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)
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.
Flags: needinfo?(continuation)
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?
Flags: needinfo?(lhenry)
Updated•7 years ago
|
Attachment #8920640 -
Flags: approval-mozilla-esr52- → approval-mozilla-esr52?
Updated•7 years ago
|
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+
Comment 23•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/aa4b11615431
Updated•7 years ago
|
Whiteboard: [adv-main58+][adv-esr52.6+]
Updated•7 years ago
|
Flags: qe-verify+
Whiteboard: [adv-main58+][adv-esr52.6+] → [adv-main58+][adv-esr52.6+][post-critsmash-triage]
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Updated•6 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•