Boris Zbarsky [:bzbarsky] Assignee
	Description • 10 years ago

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?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 1 • 10 years ago

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

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 2 • 10 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 3 • 10 years ago

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

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 5 • 10 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 6 • 10 years ago

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.

	Al Billings [:abillings - ex-MoCo]
	Comment 7 • 10 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 8 • 10 years ago

The trunk patch applies to Aurora as well

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 9 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/475ee7cda2d1

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 10 • 10 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 11 • 10 years ago

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

	Sylvestre Ledru [:Sylvestre]
	Comment 12 • 10 years ago

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.

	Carsten Book [:Tomcat]
	Comment 13 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/475ee7cda2d1

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/1abc086e66dc

	Sylvestre Ledru [:Sylvestre]
	Comment 15 • 10 years ago

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!

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 10 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/f027f38e0744

	Matt Wobensmith [:mwobensmith][:matt:]
	Comment 17 • 10 years ago

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