352124 - Arbitrary code execution using FeedWriter object

Status: RESOLVED FIXED

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

Description

[moz_bug_r_a4]

This is almost the same issue as "Arbitrary code execution with certain
extensions" bugs (bug 344494, bug 344751, bug 345305, bug 346663, bug 346664
and bug 346665), but this does not require neither extensions installed nor
user interactions.

FeedWriter.prototype.write method and FeedWriter.prototype.handleEvent method
accept a content-defined object as an argument.  When a content-defined object
is passed to feedWriter.write(x) or feedWriter.handleEvent(x) as an argument,
content-defined methods (e.g. x.QueryInterface, etc.) can be called by chrome
script.  Thus, content can run arbitrary code with chrome privileges by using
Array.prototype methods trick or document.write trick (the latter seems to have
been fixed on trunk).

fx1.5.0.x is not affected, since only fx2.0 and fx3.0 have FeedWriter object.

Comment 3

[moz_bug_r_a4]

Attachment: testcase 3

This uses feedWriter.write and document.write.

This does not work on trunk.

Comment 4

[Daniel Veditz [:dveditz]]

Why is BrowserFeedWriter exposed to any and all web pages in the first place?

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Blocking based on the fact that this is a serious issue with a new feature; we might minus it if we get a sense that the risk isn't as bad as it looks, but for now it goes on the list.

Comment 6

[Daniel Veditz [:dveditz]]

> might minus it if we get a sense that the risk isn't as bad as it looks

It *is* as bad as it looks: open page, user pwned.

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

Is there a way to make sure that a certain object is only available to certain pages? 

FeedWriter was introduced to allow web-privileged content (the feed preview page) to generate a page that contained content held by the feed result service, without forcing the feed preview page to itself be privileged. 

Comment 8

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

I'm not really familiar with the entities involved in the previous comment.

Is what you want to do simply putting content in the webpage view, but the code that generates that content needs to have higher level privileges than what normal web pages do?

If so, could you simply generate all the content in chrome-level script and just pass the result to the document in the content area which can then insert it where it wants?

Comment 9

[Mike Schroepfer]

Ben - is this in progress?  We'll need to fix this for ff2 for sure..

Comment 10

[Ben Goodger (use ben at mozilla dot org for email)]

Schrep, it's not in progress because it's not clear what the solution is. 

It's not done in the chrome and then relayed to the client because I deemed it architecturally dodgy. At present, the entire feed preview generation fits inside the flow of content load. Moving to the chrome would require a major split in code for this feature. It's theoretically possible to move the call to write() to a load handler in the browser chrome, but I don't think that would really fix the problem, since it would still be writing content into an untrusted context from a trusted one. 

I would prefer to hide BrowserFeedWriter from all pages.

Comment 11

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

maybe the solution is to hide it from all pages and then let chrome code create it and pass it to the page.

To remove it from all pages you simply have to kill the code that initializes the global constructor here:

http://lxr.mozilla.org/mozilla/source/browser/components/feeds/src/FeedWriter.js#915

Then let chrome create it by calling createInstance or some such.

Comment 12

[Ben Goodger (use ben at mozilla dot org for email)]

OK, so how do you identify when the feed page is being shown, since the URI of the feed page is always that of the feed, not the feed page.

Comment 13

[Asaf Romano (gone)]

Oy, there are some weird issues with XPConnect/XPCNativeWrapper here

I couldn't figure out whether the passed object is wrapped. When a real window object is passed to nsIFeedWriter:wrtie, window.toString() is

[object XPCNativeWrapper [object Window @ 0x35d1000 (native @ 0x35cf768)]]

For the first testcase here, window.toString is:
[xpconnect wrapped nsIDOMWindow @ 0x37cc298 (native @ 0x44f36e8)]

However, (XPCNativeWrapper(window) == window) is always true.

Now, even if I explicitly wrap the passed object, both (window instanceof Foo) and window.QueryInterface are accessing the QueryInteface method of the untrusted object.

Brendan, Boris, any thoughts?

Comment 14

[Boris Zbarsky [:bzbarsky]]

> However, (XPCNativeWrapper(window) == window) is always true.

Yes.  The == operator is hacked to report this.  If you really want to test object identity, use ===.

> even if I explicitly wrap the passed object, both (window instanceof Foo)
> and window.QueryInterface are accessing the QueryInteface method of the
> untrusted object

Yes.  That's a method defined in an XPIDL interface and therefore known to XPConnect, so it gets called...  :(

I'll need to think a little bit about how you can get around XPConnect being helpful here.

Comment 15

[timeless]

i thought xpconnect would ask caps to run a security check on the QI call, but it sounds like your object (the caller) is chrome and therefore doesn't fail that check?

is it possible to temporarily adopt a different security priviledge (Components.utils.Sandbox ?)?

Comment 16

[Asaf Romano (gone)]

(In reply to comment #14)
> > However, (XPCNativeWrapper(window) == window) is always true.
> 
> Yes.  The == operator is hacked to report this.  If you really want to test
> object identity, use ===.

If that works (wouldn't that just add a type of check? They're both "object" iirc...), I guess we could use this as a hack for now.

Comment 17

[Asaf Romano (gone)]

"typeof" check i mean.

Comment 18

[Asaf Romano (gone)]

...which wouldn't work because of the wrapper lifetime, doh.

Comment 19

[Asaf Romano (gone)]

(In reply to comment #15)
> i thought xpconnect would ask caps to run a security check on the QI call, but
> it sounds like your object (the caller) is chrome and therefore doesn't fail
> that check?
> 
> is it possible to temporarily adopt a different security priviledge
> (Components.utils.Sandbox ?)?
> 

That wouldn't work, we still need to call getInterface with chrome privs, see _getOriginalURI.

Comment 20

[Asaf Romano (gone)]

Attachment: patch

Many thanks to josh for his help.

Comment 21

[Asaf Romano (gone)]

Attachment: patch

oops, wrong file.

Comment 22

[Asaf Romano (gone)]

Attachment: also fix nsIObserver

Comment 23

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

Isn't XPCNativeWrapper supposed to *not* call script implementations of functions but rather go directly to the C++ code? Or is QI excepted from that? If so that is probably something that we need to fix.

However, can you really use XPCNativeWrapper for pure JS objects? Though that would be good if you could.

Comment 24

[Mike Connor [:mconnor]]

Comment on attachment 238506 [details] [diff] [review]
also fix nsIObserver

looks good, and seems to work effectively

would like another pair or three of eyes on this though

Comment 25

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

Is it enough to verify that the window is an interfacerequestor? There are plenty of objects that are. Couldn't an evil user just grab any other object that is an interfacerequestor that doesn't have a .write function, and then add a .write function on it?

Comment 26

[Asaf Romano (gone)]

(In reply to comment #25)
> Is it enough to verify that the window is an interfacerequestor? There are
> plenty of objects that are. Couldn't an evil user just grab any other object
> that is an interfacerequestor that doesn't have a .write function, and then add
> a .write function on it?
> 

Er? write is part of nsIFeedWriter, not of the passed argument (which is in an nsIDOMWindow).

Checking for interfacerequestor is enough, I think. |write| would throw before setting this._window if the object doesn't return something useful for getInterface(Ci.nsIWebNavigation).
 

Comment 27

[Jesse Ruderman]

> content-defined methods (e.g. x.QueryInterface, etc.) can be called by chrome
> script.  Thus, content can run arbitrary code with chrome privileges by using
> Array.prototype methods

Is there a bug filed on Array.prototype methods turning "chrome function calls a content function" bugs into "arbitrary code execution" bugs?

Comment 28

[Brendan Eich [:brendan]]

(In reply to comment #27)
> Is there a bug filed on Array.prototype methods turning "chrome function calls
> a content function" bugs into "arbitrary code execution" bugs?

See bug 344495.

/be

Comment 29

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

err, sorry, change 'write' to whatever function you're using on the window.

Jst informed me though that XPCNativeWrapper should protect against calling a function that's not defined in any interface though, so my suggested attack wouldn't really work.


Jst has done more investigation, but i'll let him comment.

Comment 30

[Asaf Romano (gone)]

(In reply to comment #29)
> err, sorry, change 'write' to whatever function you're using on the window.
> 

Again, see _getOriginalURI which is called before this._window is set.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 238506 [details] [diff] [review]
also fix nsIObserver

Hmm...  I wish this stuff were implemented in C++ where we could check for things safely.  :(  Or that we could change our subject principal in chrome JS...

I do think this should work, though...

Comment 32

[Mike Schroepfer]

Comment on attachment 238506 [details] [diff] [review]
also fix nsIObserver

a=schrep

Comment 33

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

Don't you need to wrap the window in an XPCNativeWrapper still, even after making sure that it's a real window. Otherwise script could override any methods on it.

Comment 34

[Asaf Romano (gone)]

When a window is passed, it's already XPCNative-wrapped.

Comment 35

[Asaf Romano (gone)]

1.8 branch: mozilla/browser/components/feeds/src/FeedWriter.js 1.2.2.21

Comment 36

[Asaf Romano (gone)]

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

Comment 37

[moz_bug_r_a4]

It's still exploitable.

  var o = {
    QueryInterface : function() { return this; },
    canCreateWrapper : function() { return "AllAccess"; }
  };
  (new BrowserFeedWriter()).write(o);

In this case, _isTrustedWindow() returns true.

Comment 38

[moz_bug_r_a4]

Attachment: testcase 4 - exploit

Comment 39

[moz_bug_r_a4]

Attachment: testcase 5 - shows content-defined functions that can be called by chrome

Comment 40

[Boris Zbarsky [:bzbarsky]]

OK.  :(

So here's what's going on in general in the various cases here, just so we're all straight on it:

When the page does |new BrowserFeedWriter()| it gets an XPCWrappedNative around the nsXPCWrappedJS that wraps the JSObject that's created by the GenericComponentFactory(FeedWriter) thing.

Normal code flow:  Page passes a Window object to write().  XPConnect sees that this method takes an nsIDOMWindow and tries to get one from the JSObject. Since the JSObject is an XPCWrappedNative for some nsISupports, XPConnect QIs the nsISupports to nsIDOMWindow (which succeeds, since the object is an nsGlobalWindow) and passes that to the write() method of the feed writer's nsXPCWrappedJS.  Since that's going to call out into JS, we wrap the nsIDOMWindow in an XPCWrappedNative, and since its scope (which is just the window's scope) differs from the callee scope (chrome) we wrap it in XPCNativeWrapper.

Exploit code flow:  Page passes a random JSObject to write().  XPConnect tries to get an nsIDOMWindow from the JSObject and fails.  It then creates an nsXPCWrappedJS which implements nsIDOMWindow and passes that to the write() method of the feed writer's nsXPCWrappedJS.  The nsXPCWrappedJS doesn't have a scope of its own, so no XPCNativeWrapper is created when wrapping the nsXPCWrappedJS in an XPCWrapppedNative.  But even if one were (or if the calee creates one manually), it doesn't matter.  The problem is that you're not looking at the sort of object that XPCNativeWrapper can protect you from -- an XPCWrappedNative for some object, with random stuff set on it.  In this case the XPCWrappedNative is a completely vanilla one.  The "native" it's wrapping, however is an nsXPCWrappedJS, not an nsGlobalWindow.

What moz_bug_r_a4 discovered is that nsISecurityCheckedComponent is scriptable...  so the page can just implement it, and bypass the attempted patch for this bug.

So the real question is how one tells apart, in JS,  an XPCWrappedNative for an nsXPCWrappedJS from an XPCWrappedNative for an nsGlobalWindow.  I don't think there's a way to do it...  In C++ I would try QI to a noscript interface; preferably one of the ones implemented by nsXPCWrappedJS itself (nsIXPConnectJSObjectHolder comes to mind).  But any interface that can be QIed to in JS can be "implemented" in the way attachment 238573 [details] demonstrates.

So some options I can think of:

1)  Move some of the feed code into C++ where it can safely perform the
    security checks it wants to perform here.
2)  Add a method to Components.utils that takes an nsISupports and returns
    whether it's a nsXPCWrappedJS (or possibly even whether it's an
    nsXPCWrappedJS for a non-chrome object).
3)  Prevent creation of nsXPCWrappedJS for non-chrome objects (or possibly just
    callers without UniversalXPConnect?) altogether.

Long-term, option 3 may be nice if we can do it -- it would allow possible removal of various existing defensive DOM code (for example, we could assume that being nsIDOMNode means that QI to nsINode should succeed).  But for the release, option 2 is probably the way to go...

Comment 41

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

What me and jst talked was that we should make it impossible to create an XPCNativeWrapper around a JS object (at least an untrusted one) and use it. We should either throw at every function call, or at some point during the wrapping.

Comment 42

[Boris Zbarsky [:bzbarsky]]

> we should make it impossible to create an XPCNativeWrapper around a JS object

And then have this code manually create an XPCNativeWrapper?

Comment 43

[Johnny Stenback (:jst)]

Attachment: Make XPCNativeWrapper refuse to wrap a double-wrapped JS object.

This fixes all testcases in this bug, including the two new ones.

The change here is to make it so that you can't create an XPCNativeWrapper from script for a double-wrapped JS object, and with that change the _isTrustedWindow function goes away and it's caller is simply replaced by an explicit wrapping of the given window. If that throws, we bail, if it succeeds then we know it's a native object and we should be safe from there on.

Note that this patch also prevents implicit XPCNativeWrapper wrapping of double-wrapped JS objects, and while that's not required for fixing this bug it seems like the sane thing to do that at the same time.

Comment 44

[Johnny Stenback (:jst)]

And for the record, I didn't invent that variable name, I'm simply reusing existing naming for identical code :)

Comment 45

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 238596 [details] [diff] [review]
Make XPCNativeWrapper refuse to wrap a double-wrapped JS object.

>+++ b/browser/components/feeds/src/FeedWriter.js
>+    window = new XPCNativeWrapper(window)

Toss in a semicolon?

The rest looks ok, though perhaps s/underware/xpcwrappedjs/ ?  ;)

Comment 46

[Johnny Stenback (:jst)]

Yup, the semicolon somehow got lost. It's back in my tree.

Comment 47

[Asaf Romano (gone)]

So, why is nsISecurityCheckedComponent scriptable. FWIW, I don't see any in tree scripts which use it directly.

Comment 48

[Boris Zbarsky [:bzbarsky]]

> So, why is nsISecurityCheckedComponent scriptable.

So you can use it in JS components that want to allow access to parts of themselves to untrusted script (or disallow such access, as the case may be).

Comment 49

[Johnny Stenback (:jst)]

Attachment: Patch that was checked in on the trunk.

Comment 50

[Johnny Stenback (:jst)]

Attachment: Patch that was checked in on the trunk.

Comment 51

[Johnny Stenback (:jst)]

Fixed on trunk.

Comment 52

[Mike Schroepfer]

Comment on attachment 238596 [details] [diff] [review]
Make XPCNativeWrapper refuse to wrap a double-wrapped JS object.

a=schrep

Comment 53

[Johnny Stenback (:jst)]

Fixed on the 1.8 branch.

Comment 54

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

I've checked in the following vc6 bustage fix on the 1.8 branch and trunk:

@@ -1405,7 +1405,7 @@ XPCNativeWrapper::GetNewOrUsed(JSContext
   if (xpcwrappedjs) {
     XPCThrower::Throw(NS_ERROR_INVALID_ARG, cx);

-    return JS_FALSE;
+    return nsnull;
   }

Comment 55

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

A few questions:

1. Why do you need the explicit XPCNativeWrapper wrapping? Shouldn't that happen
   implicitly? Could we make it such that if the implicit wrapping fails (due to
   trying to wrap a double-wrapped) then the functioncall fails?

2. Could we make it so that nsISecurityCheckedComponent can not be implemented by
   untrusted script?

Comment 56

[Boris Zbarsky [:bzbarsky]]

> Shouldn't that happen implicitly?

Not in this case.  See comment 40.

Comment 57

[Johnny Stenback (:jst)]

(In reply to comment #54)
> I've checked in the following vc6 bustage fix on the 1.8 branch and trunk:

Thanks!

Comment 58

[Asaf Romano (gone)]

(In reply to comment #55)

> 2. Could we make it so that nsISecurityCheckedComponent can not be implemented
> by
>    untrusted script?
> 

filed bug 352882.

Comment 59

[[e-mail]]

(In reply to comment #43)
>Make XPCNativeWrapper refuse to wrap a double-wrapped JS object.
Shouldn't XPCNativeWrapper refuse to wrap all non-"DOM" objects? By that I mean objects that don't have expando properties that allow attributes and methods to be overridden by JS don't need XPCNativeWrappers, do they? Or can you double-wrap a JS object in such a way as to give the wrapped object expando properties too?

Comment 60

[Boris Zbarsky [:bzbarsky]]

> By that I mean objects that don't have expando properties

There are no such objects.

Comment 61

[[e-mail]]

(In reply to comment #60)
>>By that I mean objects that don't have expando properties
>There are no such objects.
Maybe I used the wrong term. I'm referring to objects where trying to set arbitrary properties results in Error: uncaught exception: [Exception... "Cannot modify properties of a WrappedNative"  nsresult: "0x80570034 (NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)"  location: "JS frame :: javascript: foo.foo = 0 :: <TOP_LEVEL> :: line 1"  data: no]

Comment 62

[moz_bug_r_a4]

Please see Bug 353117.