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

Status: RESOLVED FIXED

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

Description

[moz_bug_r_a4]

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

Comment 1

[moz_bug_r_a4]

Attachment: testcase

Comment 2

[Daniel Veditz [:dveditz]]

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

Comment 3

[Mike Schroepfer]

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.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 5

[Mike Schroepfer]

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? 

Comment 6

[Asaf Romano (gone)]

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.

Comment 7

[Asaf Romano (gone)]

And also, how would XPCNativeWrapper help here? These elements are implemented in XBL-js.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> 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="...."?

Comment 9

[Asaf Romano (gone)]

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.

Comment 10

[Asaf Romano (gone)]

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

> And also, how would XPCNativeWrapper help here?

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

Comment 12

[Mike Schroepfer]

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?

Comment 13

[Asaf Romano (gone)]

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

Comment 14

[Boris Zbarsky [:bzbarsky]]

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

Comment 15

[Asaf Romano (gone)]

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?

Comment 16

[Asaf Romano (gone)]

Attachment: patch?

This will do as a hack until 1.8.1.1 *if* the same sort of holes cannot be triggered by window.open

Comment 17

[moz_bug_r_a4]

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

Comment 18

[moz_bug_r_a4]

Attachment: testcase 2 - using window.open

Comment 19

[Asaf Romano (gone)]

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

Comment 20

[moz_bug_r_a4]

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

Comment 21

[moz_bug_r_a4]

Attachment: testcase 3 - .call(document) with subframe

Comment 22

[moz_bug_r_a4]

Attachment: testcase 4 - .call(document) with window.open

Comment 23

[Asaf Romano (gone)]

Attachment: should have worked...

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.

Comment 24

[Asaf Romano (gone)]

Thanks moz_bug, those work.

Comment 25

[[e-mail]]

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

Comment 26

[Asaf Romano (gone)]

Attachment: branch patch

Branch-only.

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 28

[Mike Connor [:mconnor]]

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)

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 30

[Mike Schroepfer]

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?

Comment 31

[Asaf Romano (gone)]

I will port this back to trunk once blockers stop jumping on me (or vice-versa). ;)

Comment 32

[Asaf Romano (gone)]

1.8: mozilla/browser/components/feeds/src/FeedWriter.js 1.2.2.24

Comment 33

[Asaf Romano (gone)]

Attachment: trunk version

Gavin, can you sanity-check this?

Comment 34

[Asaf Romano (gone)]

Attachment: with the right file picked

Comment 35

[:Gavin Sharp [email: [e-mail]]]

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

Comment 36

[Asaf Romano (gone)]

trunk: mozilla/browser/components/feeds/src/FeedWriter.js 1.23