ccing people who seem to have review/code blame for the relevant bits over time... In toolkit/devtools/server/actors/script.js we have this bit: 451 this.global = this.globalSafe = aGlobal; 452 if (aGlobal && aGlobal.wrappedJSObject) { 453 this.global = aGlobal.wrappedJSObject; 454 } and then various now-unsafe uses of this.global (ones where the page can just control what the methods return; I've included the ones where it's obvious that this.global flows into the expression): 1746 let nodes = this.global.document.getElementsByTagName("*"); ... 1764 let selector = node.tagName ? findCssSelector(node) : "window"; ... 1774 listenerForm.isEventHandler = !!node["on" + listenerForm.type]; ... and most importantly for purposes of bug 789261: 5385 function getInnerId(window) { 5386 return window.QueryInterface(Ci.nsIInterfaceRequestor). 5387 getInterface(Ci.nsIDOMWindowUtils).currentInnerWindowID; 5388 }; Why exactly are we working with the unwrapped object here? The blame for the .wrappedJSObject bit goes back a ways; I tracked it to at least http://hg.mozilla.org/mozilla-central/rev/81e2e423fc63 Bug 901930 added the globalSafe bit, but only used it for toString, not all the other places this code is poking at. For the other uses above, they all seem to date back to <http://hg.mozilla.org/mozilla-central/rev/5d860e628af6>, so at least ESR24 is not affected, but every other branch we have is, in case people feel this needs backporting. How much of a security problem this is in practice is hard to tell, of course. My gut instinct here is to just nix globalSafe and drop the .wrappedJSObject thing completely, but is there a _reason_ we want the non-Xray window here, exactly?

> For the other uses above Except getInnerId, of course. That one dates to http://hg.mozilla.org/mozilla-central/rev/81c63efd1278 which is a new thing in Firefox 30.

I think the use of the unwrapped object was there since the beginning, but as you pointed out I tried to just use the wrapped one instead in bug 901930. This didn't work for all cases however, because if I remember correctly the event listener service didn't like the wrapped object. I didn't spend too much time figuring out why though, or what it would take to change this. I would certainly be happy if we could just use the wrapped object everywhere. I just found out about the unwrapped object use in getInnerId. I would assume that it doesn't need it, but I don't know for sure.

> the event listener service didn't like the wrapped object Should work fine. > I would assume that it doesn't need it Not only does it not need it, but using it there is just broken with WebIDL window. I have a patch for this stuff that seems to be green on try.

Comment on attachment 8397058 [details] [diff] [review] No more unnecessary .wrappedJSObject in debugger. Review of attachment 8397058 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Ran a bunch of smoketests on desktop, fennec and b2g (targeting both chrome & content) without noticing any regressions.

Comment on attachment 8397058 [details] [diff] [review] No more unnecessary .wrappedJSObject in debugger. [Security approval request comment] How easily could an exploit be constructed based on the patch? I don't know. We have no known exploit at this time, but I didn't look too hard for one. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Not any more than the code. Which older supported branches are affected by this flaw? 28, 29, 30. If not all supported branches, which bug introduced the flaw? Let's call it bug 832982. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I do not, but I'd think backporting would be simple. How likely is this patch to cause regressions; how much testing does it need? Medium risk, imo. It's not clear whether we in fact care about backporting this.

Comment on attachment 8397058 [details] [diff] [review] No more unnecessary .wrappedJSObject in debugger. sec-approval+ for trunk. Let's get this on Aurora and Beta as well.

The trunk patch applies to Aurora as well

Comment on attachment 8397058 [details] [diff] [review] No more unnecessary .wrappedJSObject in debugger. [Approval Request Comment] Bug caused by (feature/regressing bug #): Let's claim bug 832982, except it's not quite true. See comment 0. User impact if declined: Might be possible to trick debugger code into doing something unexpected on behalf of the page. Hard to tell. Testing completed (on m-c, etc.): Passes try. Risk to taking this patch (and alternatives if risky): I would say medium-to-high risk. There's no try test coverage for this code on Android or b2g, and the changes being made can subtly break stuff, unfortunately. :( String or IDL/UUID changes made by this patch: None.

Comment on attachment 8397387 [details] [diff] [review] Patch for beta Requesting beta approval, but I'm not 100% sure we want to take the risk or this on beta...

Comment on attachment 8397058 [details] [diff] [review] No more unnecessary .wrappedJSObject in debugger. I am going to approve it for aurora today. About beta, I propose that we discuss in a few days about its uplift.

Comment on attachment 8397387 [details] [diff] [review] Patch for beta 3 days in m-c. 2 in aurora. Let's do it in beta now!

Marking qa- based on comment 10 and difficulty in exploiting.