OK, jonco just confirmed to me that not doing this is bad. Like "leave a dangling JSObject* there" bad. We should fix this and uplift to all affected branches. The specific thing that needs fixing is that workerdebuggersandbox_class has an objectMovedOp (workerdebuggersandbox_moved) that does nothing. Presumably it has a non-null objectMovedOp because nsWrapperCache::SetWrapper asserts that the JSClass of the object being set has such a thing. And the reason it does _that_ is because per comments above nsWrapperCache::UpdateWrapper that objectMovedOp MUST update the wrapper cache's stored pointer if the object moves. Otherwise that pointer will be stale.

Who did you mean to needinfo, Boris? Presumably they weren't CCed or something.

I meant to needinfo the assignee. Who is "cced" or something.

Oh, and the reason it failed is that bugzilla is a buggy POS. :(

My understanding of the wrapper cache is limited, but it seems to me like this should do the trick.

Comment on attachment 8735958 [details] [diff] [review] Patch to be reviewed r=me

Try run for this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=df7d6408b522

Looks like try was busted from an earlier patch, so let's try that one again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4805ad94ed39

This code has been around for some time, so I assume we have to uplift this patch to all branches. Boris, I can request the uplift, but I don't fully understand the user impact if this patch is declined. Can you help clarify that?

The user impact is that if a worker debugger sandbox is created, then after that the wrapper cache pointer can go stale when a gc happens. After that point, we probably crash the next time we try to cycle collect on that worker (or anything else which tries to use that JS object). If we're lucky, it's not an exploitable crash; I have no idea how likely it is that we get lucky.

(In reply to Eddy Bruel [:ejpbruel] from comment #8) > This code has been around for some time, so I assume we have to uplift this > patch to all branches. Including ESR-38? This is marked as blocking bug 1092102; is that what introduced this code? That landed about a year ago--after 38--and I don't see workerdebuggingsandbox code in WorkerScope.cpp on the ESR-39 branch. ESR-45 definitely needs a patch though. A remote attacker can't create a worker debugger sandbox, right? If the best attack involves luring the victim into debugging your malicious page we could probably lower the severity to sec-moderate. On the other hand a faux-innocent "why doesn't my page work?" request or bug report could be a successful targeted attack.

(In reply to Daniel Veditz [:dveditz] from comment #11) > (In reply to Eddy Bruel [:ejpbruel] from comment #8) > > This code has been around for some time, so I assume we have to uplift this > > patch to all branches. > > Including ESR-38? This is marked as blocking bug 1092102; is that what > introduced this code? That landed about a year ago--after 38--and I don't > see workerdebuggingsandbox code in WorkerScope.cpp on the ESR-39 branch. > ESR-45 definitely needs a patch though. > > A remote attacker can't create a worker debugger sandbox, right? If the best > attack involves luring the victim into debugging your malicious page we > could probably lower the severity to sec-moderate. On the other hand a > faux-innocent "why doesn't my page work?" request or bug report could be a > successful targeted attack. In theory, the only way to create a worker debugger sandbox is by calling createSandbox on WorkerDebuggerGlobalScope. That global is only available to debugger scripts, which can only be loaded in a worker via a call to WorkerDebugger.initialize on the main thread. That method, in turn, is only available to chrome code. I should add that debugger code in a worker can evaluate code in the main worker global (by evaluating code in the console, for instance), and code in the main worker global can trigger code in the debugger's global (by hitting a breakpoint, for instance). It might be possible for the debuggee to somehow trick the debugger into creating a sandbox. To minimise the risk of attacks like that, we use an opaque security wrapper that allows the debugger to have references to debuggee objects (we need at least one such reference to tell the debugger API what global to debug), but does not allow the debugger to inspect those objects in any way. Conversely, the debuggee is not allowed to have any references ot debugger objects at all. I hope that gives you the information you need. My understanding is that we need to uplift to aurora, beta, and ESR45. Do you still need me to request approval for those, or can you take it from here?

Thanks! You should put on the beta, aurora, and esr45 requests.

Comment on attachment 8735958 [details] [diff] [review] Patch to be reviewed [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: User impact if declined: Can cause Firefox to crash. Fix Landed on Version: 48 Risk to taking this patch (and alternatives if risky): No significant risk. String or UUID changes made by this patch: N/A See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info. Approval Request Comment [Feature/regressing bug #]: 1092102 [User impact if declined]: Can cause Firefox to crash. [Describe test coverage new/current, TreeHerder]: [Risks and why]: Without this patch, the wrapper cache for a worker debugger sandbox can go stale when a gc happens. If that happens, the next time we try to do a cc, the browser could crash. It is unclear whether this crash is exploitable or not. [String/UUID change made/needed]: N/A

Hi Dan, Al, does the patch need a sec-approval+ before I accept it on all branches? Please let me know.

It should have had sec-approval+ before it was checked in. It looks like policy was ignored. Eddy, can you set sec-approval? on the patch and answer the sec-approval template questions, please?

Comment on attachment 8735958 [details] [diff] [review] Patch to be reviewed [Security approval request comment] How easily could an exploit be constructed based on the patch? Unclear. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. The problem is one of omission. It is unlikely to be spotted unless someone had intimate knowledge of the wrapper cache. Which older supported branches are affected by this flaw? aurora, beta, and esr45 are affected If not all supported branches, which bug introduced the flaw? bug 1092102 introduced the flaw Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I don't have any backports. The patch should apply as it is on all branches, with minimal to no rebasing required. How likely is this patch to cause regressions; how much testing does it need? Very unlikely.

Comment on attachment 8735958 [details] [diff] [review] Patch to be reviewed I'm giving this sec-approval+ for trunk and approving for Aurora but I expect that this is too late to go into beta or ESR45.

We could still take this in beta 11 (Thursday) and we will have an RC next Monday. bz, what do you think? Too risky for late beta? You did emphasize this should land on all branches.

I think this is totally fine to land in a late beta.

Comment on attachment 8735958 [details] [diff] [review] Patch to be reviewed Sec-high, may fix some crashes, bz says low risk. Doing it for beta 11 and esr45

https://hg.mozilla.org/releases/mozilla-beta/rev/144cc93716da https://hg.mozilla.org/releases/mozilla-esr45/rev/be7a56b8ecae