Bugzilla@Mozilla – Bug 531364
Fix for bug 380474 does not work with security wrappers
Last modified: 2010-05-09 15:50:18 PDT
Summon comment box
Bug 380474 was fixed by innerizing a window early to force an event listener to be attached to the old inner window. But, that fix does not deal with security wrappers (OBJ_TO_INNER_OBJECT(cx, obj) does nothing if obj is a security wrapper). And, we need a new fix once a patch in bug 428229 removes nsEventReceiverSH::AddEventListenerHelper. On 1.9.1/1.9.0 branches, it's possible to perform an XSS attack. On trunk and 1.9.2 branch, due to the fix for bug 504021, it's not possible to run code with the privileges of a target site, but e.g. it's possible to sniff keystrokes on a target site.
(In reply to comment #0) > And, we need a new fix once a patch in bug 428229 removes > nsEventReceiverSH::AddEventListenerHelper. In a build with the patches for bug 428229 the two testcases from bug 380474 don't work. The "Components.lookupMethod(frames[1].location, "href")" throws, and if I add a line to dump frames[1].location I get "Permission denied for <file://> to call method Location.toString on <http://www.mozilla.com>.".
Yes. In the past, Components.lookupMethod(crossOriginWindow.location, "href") did not throw, but now it throws because a Location object is wrapped in an XOW. In a build with the patches for bug 428229, testcase 3 & 4 in this bug work (and testcase 1 & 2 can attach event listeners to the target site).
(In reply to comment #6) > In a build with the patches for bug 428229, testcase 3 & 4 in this bug work > (and testcase 1 & 2 can attach event listeners to the target site). Ok, just to be clear about what you're saying, we'll need a new fix for this bug after the patches for bug 428229 land, but it doesn't regress bug 380474?
What I meant to say is that removing nsEventReceiverSH::AddEventListenerHelper regresses bug 380474 (the problem fixed in bug 380474 is that an attacker can attach an event listener to a target site). Bug 380474 was fixed by innerizing a window in nsEventReceiverSH::AddEventListenerHelper (and nsXPCComponents_Utils::LookupMethod), thus if we remove nsEventReceiverSH::AddEventListenerHelper then we need to innerize a window somewhere or fix the problem in a different way (e.g. doing security check in nsGlobalWindow::AddEventListener).
Nominating for 1.9.2 in the absense of a multistate flag.
not a blocker per mrbkap
Created attachment 417954 [details] [review] Start of a patch For mrbkap. I gave up after we realized we need to audit all callers of XPCNativeWrapper::GetWrappedNative.
Created attachment 418295 [details] [review] Patch As a note: the CheckPropertyAccess calls are now obsolete because nothing can happen between the call to GetJSObjectOfWrapper (which actually does a security check as well as unwraps security wrappers) to change the principal of the passed-in object, but we can keep them for now. This patch just makes sure that we unwrap sooner, basically.
Comment on attachment 418295 [details] [review] Patch File a followup on removing the dead code?
(In reply to comment #13) > File a followup on removing the dead code? There isn't much of a point since the code is going away in bug 428229.
Comment on attachment 417954 [details] [review] Start of a patch I filed bug 536480 for pursuing this patch.
http://hg.mozilla.org/mozilla-central/rev/ce29e942a14d
Comment on attachment 418295 [details] [review] Patch I'm not entirely sure what release to nominate this for, but 1.9.2.1 seems like a decent enough choice.
Created attachment 418939 [details] [review] For 1.9.1 The backport was trivial.
The problem in nsXPCComponents_Utils::LookupMethod is not fixed (the patch in comment 12 fixes testcase 3 but does not fix testcase 4).
Created attachment 419048 [details] [review] Fix for that too Sorry, I missed that there were two different bugs here, somehow :-/.
http://hg.mozilla.org/mozilla-central/rev/2f4c8116f515
Comment on attachment 418939 [details] [review] For 1.9.1 Approved for 1.9.1.8, a=dveditz for release-drivers
Do we need branch approvals or a different branch patch for "Fix for that too" (attachment 419048 [details] [review])?
Created attachment 422653 [details] [review] For 1.9.2
Created attachment 422654 [details] [review] For 1.9.1
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2ce89e85b342 includes both patches. I'll leave the approval request for an ex-post-facto approval.
a192=beltzner
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8e1fcf8f8b15
Does the 1.9.1 patch work for 1.9.0?
Comment on attachment 422654 [details] [review] For 1.9.1 Approved for 1.9.1.8, a=dveditz for release-drivers
Comment on attachment 422654 [details] [review] For 1.9.1 Approved for 1.9.0.18, a=dveditz
Checking in dom/src/base/nsDOMClassInfo.cpp; /cvsroot/mozilla/dom/src/base/nsDOMClassInfo.cpp,v <-- nsDOMClassInfo.cpp new revision: 1.557; previous revision: 1.556 done Checking in js/src/xpconnect/src/xpccomponents.cpp; /cvsroot/mozilla/js/src/xpconnect/src/xpccomponents.cpp,v <-- xpccomponents.cpp new revision: 1.142; previous revision: 1.141 done
The fix for AddEventListenerHelper in 1.9.1/1.9.0 does not work. After innerizing obj, wrapper is still the outer window, but we get eventTarget from wrapper. http://mxr.mozilla.org/mozilla1.9.1/source/dom/src/base/nsDOMClassInfo.cpp#7245
Verified fixed with builds on trunk and 1.9.2: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20100126 Minefield/3.7a1pre (.NET CLR 3.5.30729) ID:20100126052630 Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.2pre) Gecko/20100127 Namoroka/3.6.2pre (.NET CLR 3.5.30729) ID:20100127045624 (In reply to comment #33) > The fix for AddEventListenerHelper in 1.9.1/1.9.0 does not work. After > innerizing obj, wrapper is still the outer window, but we get eventTarget from > wrapper. I believe you mean testcase 3? When testing with recent builds of Shiretoko and Gran Paradiso this test fails for me. As result I will remove the fixed flags.
At least testcase 3 is still positive for 1.8.0 with applied patch.
1.9.1 patch fixes testcase 2 & 4 but does not fix testcase 1 & 3. The reason testcase 1 no longer works is that a patch in bug 504021 fixed part of testcase 1 & 2.
Created attachment 424159 [details] [review] Re-fix that
Created attachment 424580 [details] [review] Fix for 1.8.0 Based on https://bugzilla.mozilla.org/attachment.cgi?id=424159
Comment on attachment 424159 [details] [review] Re-fix that Approved for 1.9.1.8 and 1.9.0.18, a=dveditz
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d8ca06471009 Checking in dom/src/base/nsDOMClassInfo.cpp; /cvsroot/mozilla/dom/src/base/nsDOMClassInfo.cpp,v <-- nsDOMClassInfo.cpp new revision: 1.558; previous revision: 1.557 done Checking in js/src/xpconnect/src/xpccomponents.cpp; /cvsroot/mozilla/js/src/xpconnect/src/xpccomponents.cpp,v <-- xpccomponents.cpp new revision: 1.143; previous revision: 1.142 done
Verified fixed on 1.9.1 with builds on all platforms like Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.8) Gecko/20100202 Firefox/3.5.8 (.NET CLR 3.5.30729) ID:20100202153512
Verified for 1.9.0.18 on all platforms as well using attached testcases. All fixed now.