ejpbruel discovered that we're not actually tracing the worker global's JSObject. Turns out that PreserveWrapper() is required, not just HoldJSObjects.

Why HoldJSObjects isn't enough?

Because we don't trace the wrapper unless it's preserved. http://mxr.mozilla.org/mozilla-central/source/dom/base/nsWrapperCache.h#172

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.

Please update the comment in http://mxr.mozilla.org/mozilla-central/source/dom/workers/WorkerScope.cpp#45

Comment on attachment 8379176 [details] [diff] [review] Patch Actually, still trying to understand this (since I'm not familiar with worker global stuff).

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

Do I need to request sec-approval here or can we go straight to branch approvals?

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.

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.

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.

Is this ready to land, Kyle? Thanks.

Please nominate this for Aurora/Beta uplift when ready :)

Oh, is that a separate step? Lovely.

[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

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.

https://hg.mozilla.org/releases/mozilla-aurora/rev/9d0bf1e91ffd https://hg.mozilla.org/releases/mozilla-beta/rev/1d3379354e09