504021 - (CVE-2010-0179) Arbitrary code execution with Firebug XMLHttpRequestSpy

Status: RESOLVED FIXED

(Core :: Security, defect, P1)

Description

[moz_bug_r_a4]

spy.js:
top.XMLHttpRequestSpy.prototype =
{
    attach: function()
    {
        ...
        this.onreadystatechange = this.xhrRequest.onreadystatechange;

When accessing this.xhrRequest.onreadystatechange, content functions 
(QueryInterface, getInterfaces, etc.) can be called.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

I'll take a look at this.

Comment 3

[Rob Campbell [:rc] (:robcee)]

ugh. thanks for cc'ing me, ss.

Comment 4

[Rob Campbell [:rc] (:robcee)]

cc'ing honza as he's familiar with the spy.js code

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

This is kind of rough. The call stack (as moz_bug_r_a4 probably already knows is):

<firebug JS>
XPC_NW_GetProperty(onreadystatechange)
...
C++2JSObject(XPCWrappedJS(moz_bug_r_a4's evil object))
...
XPCWrappedNativeProto::GetNewOrUsed
...
XPCWrappedJS::CallMethod("getInterfaces")
...
nsLocation::SetHref

So, even with bug 503926, we'd still have to deal with this. We could go one further in that bug and disable any sort of nsIClassInfo probing on content objects while double-wrapping them and that *would* stop this. I'm not sure how much I like that solution, though.

My other option would be, in nsXPCWrappedJSClass::CallMethod, try to pin caps' conception of the subject principal to the principal of the function object we're calling (whether or not it's a native function). I *think* that with the wrapperization we do now, with the exception of document.domain stuff, the function object in question is guaranteed to have the right principal.

Thoughts?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> try to pin caps' conception of the subject principal to the principal of the
> function object we're calling

That would also fix other issues we've run into, no?  With JS-implemented components not being callable into from C++ if there's content JS on the stack?

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: wip

This works, but I'm really worried about performance.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 388767 [details] [diff] [review]
wip

One thing I probably want to do is unwrap aJSObj so we don't get SJOW(SJOW(...)) or something.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I'm not sure we have too many really perf-sensitive uses of XPCWrappedJS, do we? I certainly hope not!

That said, how would this patch help?

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #9)
> I'm not sure we have too many really perf-sensitive uses of XPCWrappedJS, do
> we? I certainly hope not!

Event handlers spring to mind. smaug did a bunch of work in bug 461861 to reduce the perf impact of firing off events.

> That said, how would this patch help?

Setting onreadystatechange wraps the function (or event handler, whichever) in an nsXPCWrappedJS. The bug that moz_bug_r_a4 has found is that when Firebug tries to get a reference to onreadystatechange, we call getInterfaces on the existing handler (through the nsXPCWrappedJS). So if that is a native function, we have a bug 344495-style attack. The SJOW created here sits between the Firebug code and the code that is wrapping the handler, preventing the attack.

Another reason I'm not terribly happy with this patch is that, with the patch, when we actually return the double-wrapped object, we end up with SJOW(XPCWrappedNative(nsXPCWrappedJS(SJOW(actual object)))) which is really, really excessive.

This is technically a regression from bug 479560 since before that bug, the page had no way of controlling the argument to the native function.

Comment 11

[moz_bug_r_a4]

(In reply to comment #5)
> <firebug JS>
> XPC_NW_GetProperty(onreadystatechange)
> ...
> C++2JSObject(XPCWrappedJS(moz_bug_r_a4's evil object))
> ...
> XPCWrappedNativeProto::GetNewOrUsed
> ...
> XPCWrappedJS::CallMethod("getInterfaces")
> ...
> nsLocation::SetHref
> 
> So, even with bug 503926, we'd still have to deal with this. We could go one
> further in that bug and disable any sort of nsIClassInfo probing on content
> objects while double-wrapping them and that *would* stop this. I'm not sure how
> much I like that solution, though.

For what it's worth, the testcase depends on the fact that my evil object can
be QIed to nsIClassInfo.  If my evil object does not have QueryInterface() then
getInterfaces() is not called.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #11)
> For what it's worth, the testcase depends on the fact that my evil object can
> be QIed to nsIClassInfo.  If my evil object does not have QueryInterface() then
> getInterfaces() is not called.

Ah, true...

Comment 13

[moz_bug_r_a4]

(In reply to comment #10)
> This is technically a regression from bug 479560 since before that bug, the
> page had no way of controlling the argument to the native function.

I'll attach another testcase that does not depend on the fix for bug 479560.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Event handlers spring to mind.

Sure.  The current cost of XPCWrappedJS dispatch is already a large fraction of that cost, though.  Does this really significantly slow down that part?  It's not particularly pretty as it is....

> The SJOW created here sits between the Firebug code and the code that is
> wrapping the handler

I guess I'm not clear on where it sits exactly.  Does it sit between the XPCWrappedJS and the untrusted JSObject?

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #15)
> Sure.  The current cost of XPCWrappedJS dispatch is already a large fraction of
> that cost, though.  Does this really significantly slow down that part?  It's
> not particularly pretty as it is....

Well, I might actually test and find that it doesn't matter, I was just worried. Doing anything through a SJOW requires compiling JS, which isn't the fastest thing in the world.

> I guess I'm not clear on where it sits exactly.  Does it sit between the
> XPCWrappedJS and the untrusted JSObject?

Yes.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Ah, I see.  OK, I see why it fixes the bug now.  ;)

Given that change, can we stop doing the explicit security checks XPCWrappedJS does now?  Or not so much?

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: wip 2

This won't fix this bug, but it's almost there!

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

This patch fixes "testcase" if I back the patch for bug 503926 out of my tree. "testcase 2" still works because it goes directly through nsXPCWrappedJSClass::CallQueryInterfaceOnJSObject and I didn't bother patching that function precisely because of the patch for bug 503926.

I think we probably want to be using the new nsIScriptSecurityManager API in more places. In particular, places that we push contexts and set aside frame chains (in part because there's a weird interaction between setting aside a frame chain and the API added here, see the comment in nsScriptSecurityManager::GetPrincipalAndFrame).

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Oh, still to come: tests for XPCSafeJSObjectWrapper.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 395462 [details] [diff] [review]
patch v1

>+        ContextPrincipal *next;
>+        JSContext *cx;
>+        JSStackFrame *fp;
>+        nsCOMPtr<nsIPrincipal> principal;

mMember, please.

>+++ b/caps/src/nsScriptSecurityManager.cpp
>+nsScriptSecurityManager::PopContextPrincipal(JSContext *cx)

Why do we even want the cx arg here?

What's the frame that GetPrincipalAndFrame returns used for?  Why do we want the innermost frame in the clamped case?

Why do we need that debug noise printf?  should it be DEBUG_mrbkap or something?

The last looks fine.  Will make those annoying security error messages in the manually compiled functions go away too.  ;)

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #21)
> mMember, please.

Ew. But OK.

> >+nsScriptSecurityManager::PopContextPrincipal(JSContext *cx)
> 
> Why do we even want the cx arg here?

I sort of like having the sanity check that we're properly stack-like in pushing and popping. I can remove it if you don't think it's worth it.

> What's the frame that GetPrincipalAndFrame returns used for?  Why do we want
> the innermost frame in the clamped case?

Oops, I meant:

            if (fp && fp == target)
            {
                *frameResult = fp;
            }
            else
            {
                JSStackFrame *inner = nsnull;
                *frameResult = JS_FrameIterator(cx, &inner);
            }

> Why do we need that debug noise printf?  should it be DEBUG_mrbkap or
> something?

Nah, I nuked it. I was a little worried that the list would get very long. Some light testing that I did showed that it doesn't.

> The last looks fine.  Will make those annoying security error messages in the
> manually compiled functions go away too.  ;)

Hmm, the error messages won't be misleading, but we're not going to have source coordinates anymore (because we set aside the frame chain). I don't think this is any worse than what we have now, but I don't think it's better.

Comment 23

[Boris Zbarsky [:bzbarsky]]

> Oops, I meant:

Ah, ok.  That makes a lot more sense.  ;)

Comment 24

[Johnathan Nightingale [:johnath]]

This one's been asleep for a month or so - Johnny, Dan - little review love?

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 395462 [details] [diff] [review]
patch v1

>+    [noscript] void pushContextPrincipal(in JSContextPtr cx,
>+    [noscript] void popContextPrincipal(in JSContextPtr cx);

We should add new functions to the end of the interface, not in the middle please.

sr=dveditz on the caps stuff. I briefly looked at the xpconnect part but not closely enough to call a sr+

Comment 26

[Johnny Stenback (:jst)]

Comment on attachment 395462 [details] [diff] [review]
patch v1

- In nsScriptSecurityManager.h:

-    static nsIPrincipal*
+    nsIPrincipal*
     GetSubjectPrincipal(JSContext* cx, nsresult* rv);

I wonder if this performs better, worse, or the same, as leaving these static and making the members they now need also static? Performance used to matter a lot here, but probably matters way less nowadays.

- In nsScriptSecurityManager::GetPrincipalAndFrame():

         // Get principals from innermost frame of JavaScript or Java.

Java, really? :) Wanna fix that comment while you're here?

r=jst

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/5c503628b3ed

Comment 28

[Rob Campbell [:rc] (:robcee)]

any approval love for this for 1.9.2?

Comment 29

[Rob Campbell [:rc] (:robcee)]

needs checkin. Johnath clarified the flagging process.

Comment 30

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 395462 [details] [diff] [review]
patch v1

I still don't see any checkin notices in the bug. Rerequesting approval (this time actually setting the flag)...

Comment 31

[Samuel Sidler (old account; do not CC)]

Note that this changes an interface, which I'm not sure we can do for 1.9.2 now?

Blake: Where are we on getting this landed on 1.9.2 and getting a 1.9.1 patch ready?

Comment 32

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Applying to the 1.9.2 branch

I think this is a proper backport of this patch to the 1.9.2 branch. I'll do some testing in a second. Note: DO NOT LAND THIS ON THE 1.9.2 BRANCH. I'm going to attach another patch, on top of this one that makes this safe to land on a branch where we're not allowed to change nsIScriptSecurityManager.

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: As promised

Comment 34

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Applying to the 1.9.1 branch -- but untested

This is the same deal as before...

Comment 35

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Ibid

Comment 36

[Blake Kaplan (:mrbkap) (inactive)]

Note that the 1.9.1 branch patch probably accidentally contains the fix for bug 508752, which isn't really correct.

Comment 37

[Daniel Veditz [:dveditz]]

And the 1.9.1 patch does NOT contain the fixes for dependencies and regressions bug 503926, bug 502959 (by way of bug 506752), and bug 509824. That's a lot of patching to do with Blake not around :-(

I think we've got to wait on this one on the older branches.

Also you've changed the IID of an interface that says not to change it without coordinating with a new JEP update, and we had to go back to using the JEP on 1.9.2,  and we definitely don't want to be doing it that way on a stable branch (hello _MOZILLA_1.9.1_BRANCH interface ugliness).

Comment 38

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #37)
> And the 1.9.1 patch does NOT contain the fixes for dependencies and regressions
> bug 503926, bug 502959 (by way of bug 506752), and bug 509824. That's a lot of
> patching to do with Blake not around :-(

bug 503926 isn't a regression from this bug. That bug happened to fix this exploit, leading to a "depends on" relationship. I don't know whether we want to take it on the 1.9.1 branch. We can decide if we want to take that fix separately from this one (thanks for letting us make the distinction, bugzilla!).

> Also you've changed the IID of an interface

The first patch changes the IID and the second one (Ibid) changes it back. I did it that way to maintain my own sanity when dealing with these patch as well as to make reviewing the patches a little easier.

Comment 39

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6ccaf1dd6553

Comment 40

[Johnny Stenback (:jst)]

Comment on attachment 410808 [details] [diff] [review]
Applying to the 1.9.1 branch -- but untested

Looks good.

r=jst

Comment 41

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

Sorry, Blake: which of these patches are we supposed to be approving/landing? The one nominated, or the one marked ibid?

Comment 42

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #41)
> Sorry, Blake: which of these patches are we supposed to be approving/landing?
> The one nominated, or the one marked ibid?

They have to land together: attachment 410808 [details] [diff] [review] is a direct backport of the patch that landed on trunk to the 1.9.1 branch and attachment 410809 [details] [diff] [review] makes it safe to land on the old branch, where we're not allowed to change interfaces.

Comment 43

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

Comment on attachment 410808 [details] [diff] [review]
Applying to the 1.9.1 branch -- but untested

a=beltzner for 191 branch

Comment 44

[Daniel Veditz [:dveditz]]

Is the 1.9.0 branch immune, or will we be 0-daying ourselves by releasing this fix on the 1.9.1/1.9.2 branches?

Comment 45

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #44)
> Is the 1.9.0 branch immune, or will we be 0-daying ourselves by releasing this
> fix on the 1.9.1/1.9.2 branches?

Yeah, this affects the 1.9.0 branch. However, on that branch, I think that we should just take bug 503926, which fixes this bug with far less code churn (though, admittedly, less thoroughly).

Comment 46

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2ad3dc1dcf42

Comment 47

[Daniel Veditz [:dveditz]]

(In reply to comment #45)
> Yeah, this affects the 1.9.0 branch. However, on that branch, I think that we
> should just take bug 503926, which fixes this bug with far less code churn

Fine, marked that bug and its regressions as blocking 1.9.0.18. Still going to make this a blocker because this is the security hole you claim is fixed by that other one.

Comment 48

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: For 1.9.0

jst, want to give this a once-over?

Comment 49

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ca3f3f6b9846 backs out the SJOW stuff on 1.9.1. Hopefully that'll fix the fallout from that changeset on the 1.9.1 branch.

Comment 50

[Daniel Veditz [:dveditz]]

Comment on attachment 423691 [details] [diff] [review]
For 1.9.0

Approved for 1.9.0.18, a=dveditz

Comment 51

[Blake Kaplan (:mrbkap) (inactive)]

Checking in caps/idl/nsIScriptSecurityManager.idl;
/cvsroot/mozilla/caps/idl/nsIScriptSecurityManager.idl,v  <--  nsIScriptSecurityManager.idl
new revision: 1.81; previous revision: 1.80
done
Checking in caps/include/nsScriptSecurityManager.h;
/cvsroot/mozilla/caps/include/nsScriptSecurityManager.h,v  <--  nsScriptSecurityManager.h
new revision: 1.118; previous revision: 1.117
done
Checking in caps/src/nsScriptSecurityManager.cpp;
/cvsroot/mozilla/caps/src/nsScriptSecurityManager.cpp,v  <--  nsScriptSecurityManager.cpp
new revision: 1.366; previous revision: 1.365
done
Checking in js/src/xpconnect/shell/xpcshell.cpp;
/cvsroot/mozilla/js/src/xpconnect/shell/xpcshell.cpp,v  <--  xpcshell.cpp
new revision: 1.113; previous revision: 1.112
done
Checking in js/src/xpconnect/src/XPCWrapper.h;
/cvsroot/mozilla/js/src/xpconnect/src/XPCWrapper.h,v  <--  XPCWrapper.h
new revision: 1.16; previous revision: 1.15
done
Checking in js/src/xpconnect/src/nsXPConnect.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/nsXPConnect.cpp,v  <--  nsXPConnect.cpp
new revision: 1.176; previous revision: 1.175
done
Checking in js/src/xpconnect/src/xpcwrappedjsclass.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp,v  <--  xpcwrappedjsclass.cpp
new revision: 1.119; previous revision: 1.118
done

Comment 52

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I have problems to reproduce this bug with Firefox 3.5.7. Means testcase 1 and 2 don't show an alert dialog for me when loading and Firebug's console enabled. Is there something else I would have to prepare?

Comment 53

[moz_bug_r_a4]

Are you using Firebug 1.5.0?  It seems that Firebug 1.5.0 distinguishes fx3.5
form fx3.6, and on fx3.5 it does not run code that triggers this bug.

I can reproduce testcase 1 & 2 on fx3.5.7 with Firebug 1.4.5.

https://addons.mozilla.org/en-US/firefox/addons/versions/1843#version-1.4.5

Comment 54

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Good hint! I had Firebug 1.5 installed. So I did a check with Firebug 1.4.5 again and I'm able to see the issue now.

By running both of the testcases with a recent Shiretoko build on all platforms doesn't bring up the alerts anymore. Marking as verified fixed with:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.8pre) Gecko/20100128 Shiretoko/3.5.8pre (.NET CLR 3.5.30729) ID:20100128044603

Comment 55

[moz_bug_r_a4]

testcase 2 still works on 1.9.0 with Firebug 1.4.5 (note: Firebug 1.5.0 does
not support fx3.0.*).

Patches for this bug can fix testcase 1 but cannot fix testcase 2 as described
in comment 19.  Actually both testcase 1 and 2 were fixed by bug 503926 on
trunk/1.9.2/1.9.1, but that bug is not fixed on 1.9.0 yet.

Comment 56

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix for comment 55

I missed this because it wasn't needed on the newer branches -- the added code was belt-and-braces. *sigh*.

Comment 57

[Al Billings [:abillings - ex-MoCo]]

We'll need to include Blake's update for 1.9.0.19. 

Dan Veditz and Beltzner, do you agree?

Comment 58

[Al Billings [:abillings - ex-MoCo]]

I suppose that I should CC Beltzner if I want a comment on comment 57 soon.

Comment 59

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

Removing fixed1.9.0.18 flag, marking as blocking.

Comment 60

[Daniel Veditz [:dveditz]]

Comment on attachment 425570 [details] [diff] [review]
Fix for comment 55

Is this ready to land for 1.9.0.19?

Comment 61

[Blake Kaplan (:mrbkap) (inactive)]

Yes.

Comment 62

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

Comment on attachment 425570 [details] [diff] [review]
Fix for comment 55

a=beltzner for 1.9.0 branch

Comment 63

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

We can haz branch landing?

Comment 64

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

mrbkap, are we holding off on landing this for any reason?

Comment 65

[Blake Kaplan (:mrbkap) (inactive)]

Checking in js/src/xpconnect/src/xpcwrappedjsclass.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp,v  <--  xpcwrappedjsclass.cpp
new revision: 1.120; previous revision: 1.119
done

Comment 66

[Al Billings [:abillings - ex-MoCo]]

Verified that testcase 2 is fixed in 1.9.0 now with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.19pre) Gecko/2010031106 GranParadiso/3.0.19pre (.NET CLR 3.5.30729) and Firebug 1.4.5.