725499 - WorkerFinishedRunnable should not be a control runnable.

Status: RESOLVED FIXED

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

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 1

[Daniel Veditz [:dveditz]]

Jesse, Christian: Is this anything you could play with through the js fuzzer?

Comment 2

[Christian Holler (:decoder)]

(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.

Comment 3

[Jesse Ruderman]

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 ;)

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Testcase

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

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch for trunk

Trunk has reference counted WorkerPrivates, so this is easy to fix there.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: WIP for beta

Attempting to port this to beta but it crashes in the finalizer for some reason I don't understand.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Landing-ping?

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/498d2784a240

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 12

[Alex Keybl [:akeybl]]

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.

Comment 13

[Al Billings [:abillings - ex-MoCo]]

Are we really tracking this for ESR 13+ or should it be 14+?

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Lets let this ride to 14.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This still needs to land for 14 though, right?

Comment 16

[Al Billings [:abillings - ex-MoCo]]

(In reply to Jonas Sicking (:sicking) from comment #15)
> This still needs to land for 14 though, right?

Yes.

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c53704c5e1e7

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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?

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 20

[Daniel Veditz [:dveditz]]

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.

Comment 21

[Al Billings [:abillings - ex-MoCo]]

Are we going to fix this for ESR or should we hide this forever?

Comment 22

[Daniel Veditz [:dveditz]]

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.

Comment 23

[Al Billings [:abillings - ex-MoCo]]

Removing tracking for advisory. No advisory for this until fixed in ESR.

Comment 24

[Lukas Blakk [:lsblakk] use ?needinfo]

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.