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.

I'll take a look at this.

ugh. thanks for cc'ing me, ss.

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

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?

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

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?

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

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

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

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

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

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

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

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

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

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+

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

any approval love for this for 1.9.2?

needs checkin. Johnath clarified the flagging process.

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

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?

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.

This is the same deal as before...

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

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

(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 on attachment 410808 [details] [diff] [review] Applying to the 1.9.1 branch -- but untested Looks good. r=jst

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

(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

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?

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

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

jst, want to give this a once-over?

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

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

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?

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

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.

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

We'll need to include Blake's update for 1.9.0.19. Dan Veditz and Beltzner, do you agree?

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.

Comment on attachment 425570 [details] [diff] [review] Fix for comment 55 Is this ready to land for 1.9.0.19?

Yes.

Comment on attachment 425570 [details] [diff] [review] Fix for comment 55 a=beltzner for 1.9.0 branch

We can haz branch landing?

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

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

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.