Closed Bug 725499 Opened 8 years ago Closed 7 years ago

WorkerFinishedRunnable should not be a control runnable.

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla15
Tracking Status
firefox10 --- wontfix
firefox11 - wontfix
firefox12 + wontfix
firefox13 + wontfix
firefox14 + fixed
firefox15 + fixed
firefox-esr10 - wontfix
status1.9.2 --- unaffected

People

(Reporter: khuey, Assigned: khuey)

Details

(Keywords: sec-critical, testcase, Whiteboard: [sg:critical][advisory-tracking+] Don't unhide or check in test until ESR-10 fixed or EOL)

Attachments

(3 files)

Consider the following scenario:

A parent worker creates a child worker.  The child worker sends two messages to its parent and then closes itself.  If the parent is running the first message event handler when the delete runnable is posted to the parent thread, the parent will enter the OperationCallback and run the delete runnable.  Then when the event handler returns and we proceed to the second message we will have a WorkerPrivate* that has been deleted.

Ben believes the fix is to make the WorkerFinishRunnable not be a control runnable.
Jesse, Christian: Is this anything you could play with through the js fuzzer?
Keywords: testcase-wanted
Assignee: bent.mozilla → khuey
(In reply to Daniel Veditz [:dveditz] from comment #1)
> Jesse, Christian: Is this anything you could play with through the js fuzzer?

We cannot specifically test this behavior in the JS shell (there are workers there but they seem to be too different from the browser implementation). The only thing I see that I could do is run LangFuzz on the browser and have the driver execute some tests in Workers rather than adding them to the main page as <script> elements. I'm not sure though if this would have caught this bug though, or other worker specific bugs.
Fuzzing workers is on my list. It's exactly this kind of closing/timing issue that makes it tricky to fuzz effectively, and especially tricky to fuzz as part of the existing fuzzers.

Seeing a testcase for this bug might help me ;)
Attached patch TestcaseSplinter Review
The attached testcase will crash Beta debug/opt and Trunk debug.

Example crash report: https://crash-stats.mozilla.com/report/index/cf6fdfb8-43c2-47dc-8a6d-7d44a2120406
Attached patch Patch for trunkSplinter Review
Trunk has reference counted WorkerPrivates, so this is easy to fix there.
Attachment #612993 - Flags: review?(bent.mozilla)
Attached patch WIP for betaSplinter Review
Attempting to port this to beta but it crashes in the finalizer for some reason I don't understand.
Attachment #612993 - Flags: review?(bent.mozilla) → review+
Is there a reason the trunk patch couldn't land right now? It should also apply to Aurora at this point which might be as far back as we're comfortable backporting anyway.
Yeah, there's no easy way to fix this short of refcounting, and I'm not 100% convinced it's exploitable, so Aurora might be far enough.
https://hg.mozilla.org/mozilla-central/rev/498d2784a240
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Comment on attachment 612993 [details] [diff] [review]
Patch for trunk

[Approval Request Comment]
Regression caused by (bug #): Existed forever
User impact if declined: Possibly exploitable crashes
Testing completed (on m-c, etc.): Landed on m-c
Risk to taking this patch (and alternatives if risky): Low risk
String changes made by this patch: N/A
Attachment #612993 - Flags: approval-mozilla-aurora?
Comment on attachment 612993 [details] [diff] [review]
Patch for trunk

[Triage Comment]
Low-risk sg:crit fix, approved for Aurora 14. If we're thinking about landing this for FF13 as well, please nominate for approval ahead of tomorrow's (5/15) go-to-build.
Attachment #612993 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Are we really tracking this for ESR 13+ or should it be 14+?
This still needs to land for 14 though, right?
(In reply to Jonas Sicking (:sicking) from comment #15)
> This still needs to land for 14 though, right?

Yes.
So, there's a bit of a problem here for ESR.  A fix on ESR would be radically different (because in ESR these objects aren't reference counted, they're allocated with new and delete) and much more invasive.

Given that this was discovered internally, the crashes we did see weren't super scary (access violations on read, as opposed to write or execute), and the difficulty of backporting, I'd like to wontfix this for ESR.

Thoughts?
Are we sure that this can only cause out-of-bounds reads? Rather than that that's the only thing we've seen testcases for so far.

Leaving up to security team to decide of how important out-of-bounds reads are for ESR.
Out of bounds reads of a dead object with virtual methods (at the very least the EventTarget destructor) and pointers to objects with virtual methods (various nsI* things) could very well be exploitable.

How reliably? Dunno, but I don't want to tempt fate by publishing the testcase if we don't fix it.
Flags: in-testsuite?
Whiteboard: [sg:critical] → [sg:critical] Don't unhide or check in test until ESR-10 fixed or EOL
Are we going to fix this for ESR or should we hide this forever?
Whiteboard: [sg:critical] Don't unhide or check in test until ESR-10 fixed or EOL → [sg:critical][advisory-tracking+] Don't unhide or check in test until ESR-10 fixed or EOL
There is no safe way to get this into 10.0.6 at this point. Without answering comment 21 definitively I can still say "14+" is a lie. Flipped back to nomination.
Removing tracking for advisory. No advisory for this until fixed in ESR.
Whiteboard: [sg:critical][advisory-tracking+] Don't unhide or check in test until ESR-10 fixed or EOL → [sg:critical] Don't unhide or check in test until ESR-10 fixed or EOL
After discussion in triage (including dveditz and abillings), as per https://wiki.mozilla.org/Enterprise/Firefox/ExtendedSupport:Proposal "there may be cases where a backport cannot be applied with reasonable effort, and those cases are expected to be exceptional".  We consider this to be such a case and will wontfix this for ESR10.
Whiteboard: [sg:critical] Don't unhide or check in test until ESR-10 fixed or EOL → [sg:critical][advisory-tracking+] Don't unhide or check in test until ESR-10 fixed or EOL
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.