982684 - SDK toolbar notification dispatch code uses .wrappedJSObject, which means callees can't trust the argument

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

Comment 3

[Daniel Veditz [:dveditz]]

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?

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

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

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(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

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

Comment 9

[Jorge Villalobos [:jorgev] (he/him)]

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

Comment 10

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Do not unwrap X-rays

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 8389987 [details] [review]
Do not unwrap X-rays

Fine by me if bz says that solves it

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8389987 [details] [review]
Do not unwrap X-rays

Yes, this is good.

Comment 13

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

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

Comment 15

[Dave Townsend [:mossop]]

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

Comment 16

[Andrew McCreight [:mccr8]]

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

Comment 17

[Dave Townsend [:mossop]]

Attachment: bug982684.patch

[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

Comment 18

[Dave Townsend [:mossop]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/052b11891800

Comment 19

[Lawrence Mandel [:lmandel] (use needinfo)]

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

Comment 20

[Matt Wobensmith [:mwobensmith][:matt:]]

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.