Closed Bug 975052 Opened 6 years ago Closed 6 years ago

The global object in workers is not rooted properly

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox27 --- unaffected
firefox28 + fixed
firefox29 + fixed
firefox30 + fixed
firefox-esr24 --- unaffected
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- unaffected
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: khuey, Assigned: khuey)

References

Details

(Keywords: csectype-uaf, sec-critical, Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
ejpbruel discovered that we're not actually tracing the worker global's JSObject.  Turns out that PreserveWrapper() is required, not just HoldJSObjects.
Attachment #8379176 - Flags: review?(bent.mozilla)
Why HoldJSObjects isn't enough?
No longer blocks: 936327
Blocks: 936327
Fwiw, I haven't actually been able to get this to happen in practice.  Any event listener or timeout will entrain the global cyclically.  So while this is sec-critical in theory I'm not sure how one would go about exploiting anything.
Comment on attachment 8379176 [details] [diff] [review]
Patch

I'm not the right reviewer for this.
Attachment #8379176 - Flags: review?(bent.mozilla) → review?(bugs)
Attachment #8379176 - Flags: review?(bugs) → review+
Comment on attachment 8379176 [details] [diff] [review]
Patch

Actually, still trying to understand this (since I'm not familiar with worker global stuff).
Attachment #8379176 - Flags: review+ → review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #5)
> Please update the comment in
> http://mxr.mozilla.org/mozilla-central/source/dom/workers/WorkerScope.cpp#45

Actually I think that could go away entirely.
Comment on attachment 8379176 [details] [diff] [review]
Patch

Could we assert in CreateGlobal that UnwrapDOMObjectToISupports(global)
is non-null. (Just because TryPreserveWrapper has odd behavior.)
Attachment #8379176 - Flags: review?(bugs) → review+
Do I need to request sec-approval here or can we go straight to branch approvals?
Flags: needinfo?(continuation)
This affects more than trunk, and is sec-high or sec-critical, so it needs sec-approval.  Al can probably give you branch approvals at the same time.
Flags: needinfo?(continuation)
Comment on attachment 8379176 [details] [diff] [review]
Patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Unclear.  My attempts to produce a crash were unsuccessful.

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

Yes.  I think it's reasonably obvious what is going on.

Which older supported branches are affected by this flaw?

Beta is as far back as it goes.

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

Bug 936327.

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

Backports are trivial.

How likely is this patch to cause regressions; how much testing does it need?

Unlikely to cause regressions.
Attachment #8379176 - Flags: sec-approval?
Comment on attachment 8379176 [details] [diff] [review]
Patch

sec-approval+ for trunk. Please backport to Beta and Aurora and nominate those patches so we can get them in following mozilla-central.
Attachment #8379176 - Flags: sec-approval? → sec-approval+
Is this ready to land, Kyle?  Thanks.
Flags: needinfo?(khuey)
https://hg.mozilla.org/mozilla-central/rev/36b02f8ee773
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Please nominate this for Aurora/Beta uplift when ready :)
Flags: needinfo?(khuey)
Oh, is that a separate step?  Lovely.
Flags: needinfo?(khuey)
Attached patch As landedSplinter Review
[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 936327
User impact if declined: Potentially an sg:crit, although we don't know how to exploit it.
Testing completed (on m-c, etc.): Baked on m-c for a few days, simple enough to verify via code inspection.
Risk to taking this patch (and alternatives if risky): Low risk
String or IDL/UUID changes made by this patch:None
Attachment #8379176 - Attachment is obsolete: true
Attachment #8383934 - Flags: approval-mozilla-beta?
Attachment #8383934 - Flags: approval-mozilla-aurora?
Comment on attachment 8383934 [details] [diff] [review]
As landed

Yes, each branch is its own thing, especially for security bugs. Approved for beta and aurora.
Attachment #8383934 - Flags: approval-mozilla-beta?
Attachment #8383934 - Flags: approval-mozilla-beta+
Attachment #8383934 - Flags: approval-mozilla-aurora?
Attachment #8383934 - Flags: approval-mozilla-aurora+
Whiteboard: [qa-]
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.