Closed Bug 982684 Opened 7 years ago Closed 7 years ago

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

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(firefox28 unaffected, firefox29+ fixed, firefox30+ fixed, firefox-esr24- unaffected)

RESOLVED FIXED
mozilla30
Tracking Status
firefox28 --- unaffected
firefox29 + fixed
firefox30 + fixed
firefox-esr24 - unaffected

People

(Reporter: bzbarsky, Assigned: irakli)

References

Details

(Keywords: regression, sec-critical, Whiteboard: [qa-])

Attachments

(2 files)

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?
Flags: needinfo?(jorge)
(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 :)
Flags: needinfo?(jorge)
Attached file Do not unwrap X-rays
Attachment #8389987 - Flags: review?(dtownsend+bugmail)
Attachment #8389987 - Flags: feedback?(bzbarsky)
Comment on attachment 8389987 [details] [review]
Do not unwrap X-rays

Fine by me if bz says that solves it
Attachment #8389987 - Flags: review?(dtownsend+bugmail) → review+
Comment on attachment 8389987 [details] [review]
Do not unwrap X-rays

Yes, this is good.
Attachment #8389987 - Flags: feedback?(bzbarsky) → feedback+
This was included in our uplift last night: https://hg.mozilla.org/mozilla-central/rev/f65e5e3da415
Target Milestone: --- → mozilla30
It looks like this is fixed now, so I'll close this. Please put this up for approval for landing on 29.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Attached patch bug982684.patchSplinter Review
[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
Attachment #8391518 - Flags: approval-mozilla-aurora?
Attachment #8391518 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee: nobody → rFobic
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.
Whiteboard: [qa-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.