Closed Bug 353266 Opened 13 years ago Closed 13 years ago

FeedWriter accessing foo.wrappedJSObject.bar allows arbitrary code execution, if an XSS hole exists

Categories

(Firefox Graveyard :: RSS Discovery and Preview, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 2

People

(Reporter: moz_bug_r_a4, Assigned: mano)

References

Details

(Keywords: fixed1.8.1, Whiteboard: [sg:critical] post FF1.5, testcase similar to 344494)

Attachments

(3 files, 2 obsolete files)

Chrome script accessing a property of an untrusted object allows arbitrary code
execution, until Array.prototype methods trick (bug 344495) and
document.(open|write) trick (bug 346659) are fixed.

Web pages cannot access a feed preview page.  But, at this time, there is an
XSS hole (bug 351370) that can be used to access a feed preview page.

Once an attacker gets access to a feed preview page, the attacker can run
arbitrary code with chrome privileges by using Array.prototype methods trick or
document.write trick, since FeedWriter accesses XUL properties implemented in
XBL (e.g. handlersMenuList.wrappedJSObject.selectedItem).
Attached file testcase
Takes advantage of bugs blocking 1.8.1.1, but in the Firefox product the best I can do is block firefox2 itself.  Maybe blocking1.8.1.1 should be added to this and the Thunderbird product
Flags: blocking-firefox2?
Whiteboard: [sg:critical]
Plusing to but on the list for now -  if we can make progress on any of the building block bugs for this in time for 1.8.1 that would be ideal.
Flags: blocking-firefox2? → blocking-firefox2+
Why do we have code that uses .wrappedJSObject? That defeats the purpose of the XPCNativeWrapper entierly. Any such code is a very good candidate for an exploit.
So both (bug 344495) and (bug 346659) are aways off from being fixed.   Bug 351370  just landed on trunk but there is some concern about branch safety.

Do we have an alternative to fix this particular problem? 
As I told Robert on IRC, there is no real way to avoid the use of wrappedJSObject here, We're accessing various properties of xbl bindings which are not defined in XPIDL. You would think we can go ahead and define new interfaces, but how would we block content code from implementing them?

With that said, the testcase doesn't work for me.
And also, how would XPCNativeWrapper help here? These elements are implemented in XBL-js.
> which are not defined in XPIDL

How about we like .... fix that?  That would also make it possible to perform the same operations from C++ if needed, which seems like a win to me.

Or is the problem that the XBL in question is untrusted anyway so can't use implements="...."?
So, I need a working testcase first, i guess.

One option is to execute most of this code in a an about:blank sandbox, I'm not yet sure how safe is it.
(In reply to comment #8)
> > which are not defined in XPIDL

> Or is the problem that the XBL in question is untrusted anyway so can't use
> implements="...."?
> 

That too.
> And also, how would XPCNativeWrapper help here?

By not letting you call the content-defined getters or setters that the testcase sets up.
Tried:


Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1) Gecko/20060925 BonEcho/2.0

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1) Gecko/20060917 BonEcho/2.0

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1b2) Gecko/20060910 BonEcho/2.0b2

Not able to repro the testcase...  MozBug is there something we are missing?
(In reply to comment #11)
> > And also, how would XPCNativeWrapper help here?
> 
> By not letting you call the content-defined getters or setters that the
> testcase sets up.
> 

Even if the property is defined in XPIDL (and only implemented in XBL/JS)? We've been there before.
> Even if the property is defined in XPIDL (and only implemented in XBL/JS)?

No, that case it won't help with; but any XBL that can implement interfaces wouldn't be accessible to untrusted script (it'd be chrome).
Now, since I'm not CCed on at least one of the blockers here, and since the testcase doesn't work for me, could we hide this issue right under my carpet by refusing to handle feeds in iframes until 1.8.1.1?
Attached patch patch? (obsolete) — Splinter Review
This will do as a hack until 1.8.1.1 *if* the same sort of holes cannot be triggered by window.open
(In reply to comment #12)
> Not able to repro the testcase...  MozBug is there something we are missing?

hmm... I don't know why.  I can reproduce the testcase on Windows and Linux.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20060925 BonEcho/2.0
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1) Gecko/20060925 BonEcho/2.0


(In reply to comment #16)
> This will do as a hack until 1.8.1.1 *if* the same sort of holes cannot be
> triggered by window.open

window.open can be used to exploit, instead of using a subframe.
both testcases fail here with:
Error: uncaught exception: Permission denied to set property Window.escape

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1) Gecko/20060918 BonEcho/2.0
(In reply to comment #19)
> Error: uncaught exception: Permission denied to set property Window.escape

I'll attach new testcases that use |.call(document)| instead of |.call('')|.
Could you try these?
For some reason, _safeG/SetProperty throws
Error: [Exception... "'Permission denied to get property XULElement.selectedItem' when calling method: [nsIFeedWriter::write]"  nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS frame :: chrome://browser/content/feeds/subscribe.js :: SH_init :: line 46"  data: no]
Source File: chrome://browser/content/feeds/subscribe.js
Line: 46

even though the sandbox has the same origin.
Assignee: nobody → mano
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → Firefox 2
Thanks moz_bug, those work.
(In reply to comment #6)
>We're accessing various properties of xbl bindings which are not defined in XPIDL.
XPCNativeWrappers don't allow access those defined in IDL either.
Attached patch branch patchSplinter Review
Branch-only.
Attachment #240136 - Flags: review?(mconnor)
To answer the XBL ideas bz raised:

I originally had the same idea, that these properties should be defined on XPIDL interfaces implemented by the bindings, and XPCNativeWrapper should thereafter allow access to the properties.

However, allowing XPCNativeWrapper to do that would be unsafe I realized. Any page could write any bindings and bind that to its content. If we allowed XPCNativeWrapper to call such methods that'd be the same thing as letting XPCNativeWrapper wrap any js-object implemented by the page. Remember that these bindings have the privilege level of the page they are bound to.

We could possibly make an exception for bindings with a chrome url, however we've been avoiding that in the past and it'd probably be as risky here.
Comment on attachment 240136 [details] [diff] [review]
branch patch

looks good to me, stops the exploit and avoids any usage of wrappedJSObject

thanks!

sicking, can you sr here? bz/jst would be good as well (more eyes is better)
Attachment #240136 - Flags: superreview?(bugmail)
Attachment #240136 - Flags: review?(mconnor)
Attachment #240136 - Flags: review+
Comment on attachment 240136 [details] [diff] [review]
branch patch

Looks good to me too. I think this is the best solution so far for how to deal for 2.0.
Attachment #240136 - Flags: superreview?(bugmail) → superreview+
Comment on attachment 240136 [details] [diff] [review]
branch patch

Approved for RC2.   Nice work Mano, bz, sicking and company.   Is there equivalent we need for trunk?
Attachment #240136 - Flags: approval1.8.1? → approval1.8.1+
I will port this back to trunk once blockers stop jumping on me (or vice-versa). ;)
1.8: mozilla/browser/components/feeds/src/FeedWriter.js 1.2.2.24
Keywords: fixed1.8.1
Whiteboard: [sg:critical] → [sg:critical] post ff1.5
Attached patch trunk version (obsolete) — Splinter Review
Gavin, can you sanity-check this?
Attachment #240629 - Flags: review?(gavin.sharp)
Attachment #240629 - Attachment is obsolete: true
Attachment #240630 - Flags: review?(gavin.sharp)
Attachment #240629 - Flags: review?(gavin.sharp)
Comment on attachment 240630 [details] [diff] [review]
with the right file picked

>Index: browser/components/feeds/src/FeedWriter.js

> #ifdef XP_WIN
>             // Only show the default reader menuitem if the default reader
>             // isn't the selected application
>             var defaultHandlerMenuItem =
>             this._document.getElementById("defaultHandlerMenuItem");

r=me with this changed to "var defaultHandlerMenuItem = this.defaultSystemReaderItemWrapped;".
Attachment #240630 - Flags: review?(gavin.sharp) → review+
trunk: mozilla/browser/components/feeds/src/FeedWriter.js 1.23
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
OS: Windows XP → All
Hardware: PC → All
Group: security
Group: security
Whiteboard: [sg:critical] post ff1.5 → [sg:critical] post FF1.5, testcase similar to 344494
Group: core-security
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.