Bug 504021 (CVE-2010-0179)

Arbitrary code execution with Firebug XMLHttpRequestSpy

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
Security
P1
normal
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: moz_bug_r_a4, Assigned: mrbkap)

Tracking

({testcase, verified1.9.0.19, verified1.9.1})

Trunk
mozilla1.9.3a1
testcase, verified1.9.0.19, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +
blocking1.9.1.1 -
blocking1.9.0.18 +
blocking1.9.0.19 +
wanted1.9.0.x +
in-testsuite ?

Firefox Tracking Flags

(status1.9.2 beta4-fixed, blocking1.9.1 .8+, status1.9.1 .8-fixed)

Details

(Whiteboard: [sg:critical][firebug-blocks] hold advisory until 3.0.19)

Attachments

(7 attachments, 2 obsolete attachments)

(Reporter)

Description

8 years ago
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.
Flags: blocking1.9.1.1?
Keywords: testcase
Whiteboard: [sg:critical]
(Assignee)

Comment 2

8 years ago
I'll take a look at this.
Assignee: nobody → mrbkap
ugh. thanks for cc'ing me, ss.
cc'ing honza as he's familiar with the spy.js code
blocking1.9.1: --- → needed
status1.9.1: --- → wanted
Flags: blocking1.9.1.1? → blocking1.9.1.1-
(Assignee)

Comment 5

8 years ago
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?
> 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?
(Assignee)

Comment 7

8 years ago
Created attachment 388767 [details] [diff] [review]
wip

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

Comment 8

8 years ago
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.
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?
(Assignee)

Comment 10

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

Comment 11

8 years ago
(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.
(Assignee)

Comment 12

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

Comment 13

8 years ago
(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.
> 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?
(Assignee)

Comment 16

8 years ago
(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.
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?
(Assignee)

Comment 18

8 years ago
Created attachment 395437 [details] [diff] [review]
wip 2

This won't fix this bug, but it's almost there!
Attachment #388767 - Attachment is obsolete: true
(Assignee)

Comment 19

8 years ago
Created attachment 395462 [details] [diff] [review]
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).
Attachment #395437 - Attachment is obsolete: true
Attachment #395462 - Flags: superreview?
Attachment #395462 - Flags: review?(jst)
Attachment #395462 - Flags: review?(bzbarsky)
(Assignee)

Updated

8 years ago
Attachment #395462 - Flags: superreview? → superreview?(dveditz)
(Assignee)

Comment 20

8 years ago
Oh, still to come: tests for XPCSafeJSObjectWrapper.
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.  ;)
Attachment #395462 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 22

8 years ago
(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.
> Oops, I meant:

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

Updated

8 years ago
Blocks: 516911
This one's been asleep for a month or so - Johnny, Dan - little review love?
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+
Attachment #395462 - Flags: superreview?(dveditz) → superreview+
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
Attachment #395462 - Flags: review?(jst) → review+
(Assignee)

Comment 27

8 years ago
http://hg.mozilla.org/mozilla-central/rev/5c503628b3ed
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
blocking1.9.1: needed → .5+
Flags: blocking1.9.2?
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P1
any approval love for this for 1.9.2?
Whiteboard: [sg:critical] → [sg:critical][firebug-blocks][need-approval-1.9.2]
needs checkin. Johnath clarified the flagging process.
Whiteboard: [sg:critical][firebug-blocks][need-approval-1.9.2] → [sg:critical][firebug-blocks][need-checkin]
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)...
Attachment #395462 - Flags: approval1.9.2?

Updated

8 years ago
Attachment #395462 - Flags: approval1.9.2?
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?
(Assignee)

Comment 32

8 years ago
Created attachment 410788 [details] [diff] [review]
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.
Attachment #410788 - Flags: review?(jst)
(Assignee)

Comment 33

8 years ago
Created attachment 410791 [details] [diff] [review]
As promised
Attachment #410791 - Flags: review?(jst)

Updated

8 years ago
Attachment #410788 - Flags: review?(jst) → review+

Updated

8 years ago
Attachment #410791 - Flags: review?(jst) → review+
(Assignee)

Comment 34

8 years ago
Created attachment 410808 [details] [diff] [review]
Applying to the 1.9.1 branch -- but untested

This is the same deal as before...
Attachment #410808 - Flags: review?(jst)
(Assignee)

Comment 35

8 years ago
Created attachment 410809 [details] [diff] [review]
Ibid
Attachment #410809 - Flags: review?(jst)
(Assignee)

Comment 36

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

Updated

8 years ago
Depends on: 503926
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).
Blocks: 293973
blocking1.9.1: .6+ → .7+
(Assignee)

Comment 38

8 years ago
(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.
(Assignee)

Comment 39

8 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6ccaf1dd6553
status1.9.2: --- → final-fixed
(Assignee)

Updated

8 years ago
Whiteboard: [sg:critical][firebug-blocks][need-checkin] → [sg:critical][firebug-blocks]
Whiteboard: [sg:critical][firebug-blocks] → [sg:critical][firebug-blocks][need r=jst for branch patches]
Comment on attachment 410808 [details] [diff] [review]
Applying to the 1.9.1 branch -- but untested

Looks good.

r=jst
Attachment #410808 - Flags: review?(jst) → review+

Updated

7 years ago
Attachment #410809 - Flags: review?(jst) → review+
(Assignee)

Updated

7 years ago
Attachment #410808 - Flags: approval1.9.1.8?
Sorry, Blake: which of these patches are we supposed to be approving/landing? The one nominated, or the one marked ibid?
(Assignee)

Comment 42

7 years ago
(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 on attachment 410808 [details] [diff] [review]
Applying to the 1.9.1 branch -- but untested

a=beltzner for 191 branch
Attachment #410808 - Flags: approval1.9.1.8? → approval1.9.1.8+
Attachment #410809 - Flags: approval1.9.1.8+
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?
Flags: wanted1.9.0.x?
Flags: blocking1.9.0.18?
(Assignee)

Comment 45

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

Comment 46

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2ad3dc1dcf42
status1.9.1: wanted → .8-fixed
(Assignee)

Updated

7 years ago
Whiteboard: [sg:critical][firebug-blocks][need r=jst for branch patches] → [sg:critical][firebug-blocks]
(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.
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.18?
Flags: blocking1.9.0.18+

Updated

7 years ago
Depends on: 541997
(Assignee)

Comment 48

7 years ago
Created attachment 423691 [details] [diff] [review]
For 1.9.0

jst, want to give this a once-over?
Attachment #423691 - Flags: review?(jst)
Attachment #423691 - Flags: approval1.9.0.18?

Updated

7 years ago
Attachment #423691 - Flags: review?(jst) → review+
(Assignee)

Comment 49

7 years ago
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 on attachment 423691 [details] [diff] [review]
For 1.9.0

Approved for 1.9.0.18, a=dveditz
Attachment #423691 - Flags: approval1.9.0.18? → approval1.9.0.18+
(Assignee)

Comment 51

7 years ago
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
Keywords: fixed1.9.0.18
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?
Target Milestone: --- → mozilla1.9.3a1
Version: unspecified → Trunk
(Reporter)

Comment 53

7 years ago
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
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
Flags: in-testsuite?
Keywords: verified1.9.1
OS: Windows XP → All
Hardware: x86 → All
(Reporter)

Comment 55

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

Comment 56

7 years ago
Created attachment 425570 [details] [diff] [review]
Fix for comment 55

I missed this because it wasn't needed on the newer branches -- the added code was belt-and-braces. *sigh*.
Attachment #425570 - Flags: review?(jst)

Updated

7 years ago
Attachment #425570 - Flags: review?(jst) → review+
We'll need to include Blake's update for 1.9.0.19. 

Dan Veditz and Beltzner, do you agree?
Flags: blocking1.9.0.19?
I suppose that I should CC Beltzner if I want a comment on comment 57 soon.
Removing fixed1.9.0.18 flag, marking as blocking.
Flags: blocking1.9.0.19? → blocking1.9.0.19+
Keywords: fixed1.9.0.18
Whiteboard: [sg:critical][firebug-blocks] → [sg:critical][firebug-blocks] hold advisory until 3.0.19
Comment on attachment 425570 [details] [diff] [review]
Fix for comment 55

Is this ready to land for 1.9.0.19?
Attachment #425570 - Flags: approval1.9.0.19?
(Assignee)

Comment 61

7 years ago
Yes.
Comment on attachment 425570 [details] [diff] [review]
Fix for comment 55

a=beltzner for 1.9.0 branch
Attachment #425570 - Flags: approval1.9.0.19? → approval1.9.0.19+
We can haz branch landing?
mrbkap, are we holding off on landing this for any reason?
(Assignee)

Comment 65

7 years ago
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
Keywords: fixed1.9.0.19
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.
Keywords: fixed1.9.0.19 → verified1.9.0.19
Alias: CVE-2010-0179
Group: core-security
You need to log in before you can comment on or make changes to this bug.