This code was added in bug 787390. The relevant code bit is in InputPort.prototype.observe and does: 84 const message = subject === null ? null : 85 isLegacyWrapper(subject) ? unwrapLegacy(subject) : 86 subject.wrappedJSObject ? subject.wrappedJSObject : 87 subject; But in cases when the topic is "content-page-hidden" the subject starts out as the HTMLDocument in question. Or rather an Xray to it. This code waives the Xray, and passes the resulting (now not exactly safe) object on to consumers, so we get stacks like this: 0 getFrameElement(target = [object Window]) ["sdk/window/utils.js":423] 1 anonymous(document = [object HTMLDocument]) ["sdk/input/frame.js":32] 2 anonymous(data = [object HTMLDocument]) ["sdk/event/utils.js":163] 3 emit(target = [object Object], type = "data", args = [object HTMLDocument]) ["sdk/event/core.js":96] 4 anonymous(input = [object Object], message = [object HTMLDocument]) ["sdk/event/utils.js":115] 5 anonymous(subject = [object XrayWrapper [object HTMLDocument]], topic = "content-page-hidden", data=null) Note that we started with an Xray for an HTMLDocument, but then waived it. Then in frame.js we have: 31 const getFrame = document => 32 document && document.defaultView && getFrameElement(document.defaultView); and at this point the content page can completely control the value that gets passed to getFrameElement. And getFrameElement does: 422 const getFrameElement = target => 423 (target instanceof Ci.nsIDOMDocument ? target.defaultView : target). 424 QueryInterface(Ci.nsIInterfaceRequestor). 425 getInterface(Ci.nsIDOMWindowUtils). 426 containerElement; which means that when all is said and done the page can control what the caller of getFrame sees. Marking security-sensitive, since I bet this is exploitable if one works at it a bit. This also incidentally blocks us removing window.QueryInterface from the web, since this code is relying on that existing.

Oh, and are we actually _trying_ to unwrap Xrays here, or was that an accidental byproduct of trying to unwrap something else? Like maybe cases when "subject" is an XPCWrappedNative nsISupports for a JS object? If the latter, we should just check for an Xray before unwrapping, I'd think, and be really sad .wrappedJSObject is so overloaded.

Oh, and given bug 956129 landed for 29, I assume we need to fix this on 29 too.

Probably sec-critical if any addons actually use this? The SDK is now part of Firefox, but I assume some add-ons may have been packaged with the insecure SDK and will need to be repacked to fix this (once we have a fix). Is there any easy way to tell what version of the SDK any given AMO add-on uses?

(In reply to Boris Zbarsky [:bz] from comment #1) > Oh, and are we actually _trying_ to unwrap Xrays here, or was that an > accidental byproduct of trying to unwrap something else? Like maybe cases > when "subject" is an XPCWrappedNative nsISupports for a JS object? > Yes that was an intention here. > > If the latter, we should just check for an Xray before unwrapping, I'd > think, and be really sad .wrappedJSObject is so overloaded. I'll write a patch to do that.

(In reply to Daniel Veditz [:dveditz] from comment #3) > Probably sec-critical if any addons actually use this? > > The SDK is now part of Firefox, but I assume some add-ons may have been > packaged with the insecure SDK and will need to be repacked to fix this > (once we have a fix). Is there any easy way to tell what version of the SDK > any given AMO add-on uses? As far as I know AMO reviewers reject add-ons that bundle sdk with them & this code is fairly new. Either way can query mxr for `InputPort.prototype.observe` to see if any add-on contains this module in them. mxr contains sources of add-on's

> Probably sec-critical if any addons actually use this? It's hard to say. It depends on what they do with the value. There's no automatic code execution or anything; just a chance of the addon getting confused about things.

> Either way can query mxr for `InputPort.prototype.observe` https://mxr.mozilla.org/addons/search?string=InputPort&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=addons says no matches. Yay!

That's what I get for writing long responses. What the rest said :)

Comment on attachment 8389987 [details] Do not unwrap X-rays Fine by me if bz says that solves it

Comment on attachment 8389987 [details] Do not unwrap X-rays Yes, this is good.

This change has landed: https://hg.mozilla.org/projects/addon-sdk/rev/b0e27e5b7f6d

This was included in our uplift last night: https://hg.mozilla.org/mozilla-central/rev/f65e5e3da415

It looks like this is fixed now, so I'll close this. Please put this up for approval for landing on 29.

[Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: Potential security vulnerability Testing completed (on m-c, etc.): Landed on m-c in https://hg.mozilla.org/mozilla-central/rev/f65e5e3da415 Risk to taking this patch (and alternatives if risky): Low risk String or IDL/UUID changes made by this patch: None

Given that this issue was introduced and fixed in 29, no need to take this on ESR24.

Comment 0 says this is exploitable with some effort. No test case here, so marking [qa-] for now. If we come up with one, QA would be happy to verify.