Arbitrary code execution using FeedWriter object

RESOLVED FIXED in Firefox 2

Status

()

Firefox
RSS Discovery and Preview
P1
normal
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: moz_bug_r_a4, Assigned: mano)

Tracking

({fixed1.8.1})

unspecified
Firefox 2
fixed1.8.1
Points:
---
Bug Flags:
blocking-firefox2 +
blocking1.8.0.8 -

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 3

12 years ago
Created attachment 237715 [details]
testcase 3

This uses feedWriter.write and document.write.

This does not work on trunk.
Assignee: nobody → bugs
Flags: blocking-firefox2?
Whiteboard: [sg:critical]
Why is BrowserFeedWriter exposed to any and all web pages in the first place?
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.
Flags: blocking-firefox2? → blocking-firefox2+
Target Milestone: --- → Firefox 2
> 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.
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. 
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

12 years ago
Ben - is this in progress?  We'll need to fix this for ff2 for sure..
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.
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.
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.
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?
> 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

12 years ago
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 ?)?
(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.
...which wouldn't work because of the wrapper lifetime, doh.
(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.
Assignee: bugs → mano
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Status: NEW → ASSIGNED
Created attachment 238491 [details] [diff] [review]
patch

Many thanks to josh for his help.
Attachment #238491 - Flags: review?(mconnor)
Created attachment 238492 [details] [diff] [review]
patch

oops, wrong file.
Attachment #238491 - Attachment is obsolete: true
Attachment #238492 - Flags: review?(mconnor)
Attachment #238491 - Flags: review?(mconnor)
Created attachment 238506 [details] [diff] [review]
also fix nsIObserver
Attachment #238506 - Flags: review?(mconnor)
Attachment #238506 - Flags: approval1.8.1?
Attachment #238492 - Attachment is obsolete: true
Attachment #238492 - Flags: review?(mconnor)
Attachment #238492 - Flags: approval1.8.1?
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 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
Attachment #238506 - Flags: superreview?(bugmail)
Attachment #238506 - Flags: review?(mconnor)
Attachment #238506 - Flags: review+
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?
(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

12 years ago
> 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?
(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
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.
(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 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...
Attachment #238506 - Flags: superreview?(bugmail) → superreview+

Comment 32

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

a=schrep
Attachment #238506 - Flags: approval1.8.1? → approval1.8.1+
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.
When a window is passed, it's already XPCNative-wrapped.
1.8 branch: mozilla/browser/components/feeds/src/FeedWriter.js 1.2.2.21
Keywords: fixed1.8.1
trunk: mozilla/browser/components/feeds/src/FeedWriter.js  1.16
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Reporter)

Comment 37

12 years ago
It's still exploitable.

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

In this case, _isTrustedWindow() returns true.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 38

12 years ago
Created attachment 238573 [details]
testcase 4 - exploit
(Reporter)

Comment 39

12 years ago
Created attachment 238574 [details]
testcase 5 - shows content-defined functions that can be called by chrome

Updated

12 years ago
Keywords: fixed1.8.1
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...
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.
> we should make it impossible to create an XPCNativeWrapper around a JS object

And then have this code manually create an XPCNativeWrapper?
Created attachment 238596 [details] [diff] [review]
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.
Attachment #238596 - Flags: superreview?(brendan)
Attachment #238596 - Flags: review?(bzbarsky)
And for the record, I didn't invent that variable name, I'm simply reusing existing naming for identical code :)
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/ ?  ;)
Attachment #238596 - Flags: review?(bzbarsky) → review+
Yup, the semicolon somehow got lost. It's back in my tree.

Updated

12 years ago
Attachment #238596 - Flags: superreview?(brendan) → superreview+
So, why is nsISecurityCheckedComponent scriptable. FWIW, I don't see any in tree scripts which use it directly.
> 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).
Created attachment 238634 [details] [diff] [review]
Patch that was checked in on the trunk.
Created attachment 238635 [details] [diff] [review]
Patch that was checked in on the trunk.

Updated

12 years ago
Attachment #238634 - Attachment is obsolete: true
Fixed on trunk.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Attachment #238596 - Flags: approval1.8.1?

Comment 52

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

a=schrep
Attachment #238596 - Flags: approval1.8.1? → approval1.8.1+

Updated

12 years ago
Whiteboard: [sg:critical] → [checkin needed (1.8 branch)]
Whiteboard: [checkin needed (1.8 branch)] → [sg:critical][checkin needed (1.8 branch)]
Fixed on the 1.8 branch.
Keywords: fixed1.8.1
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;
   }
Whiteboard: [sg:critical][checkin needed (1.8 branch)] → [sg:critical]
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?
> Shouldn't that happen implicitly?

Not in this case.  See comment 40.

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

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

12 years ago
(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?
> By that I mean objects that don't have expando properties

There are no such objects.

Comment 61

12 years ago
(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]
(Reporter)

Comment 62

12 years ago
Please see Bug 353117.
Flags: blocking1.8.0.8-
Whiteboard: [sg:critical] → [sg:critical] post FF1.5
Whiteboard: [sg:critical] post FF1.5 → [sg:critical] post FF1.5, testcase similar to 344494
Group: core-security
You need to log in before you can comment on or make changes to this bug.