Last Comment Bug 531364 - (CVE-2010-0171) Fix for bug 380474 does not work with security wrappers
(CVE-2010-0171)
: Fix for bug 380474 does not work with security wrappers
Status: VERIFIED FIXED
[sg:high][3.6.x]
: verified1.9.0.18, verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla1.9.3a1
Assigned To: Blake Kaplan (:mrbkap)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-11-27 02:01 PST by moz_bug_r_a4
Modified: 2010-05-09 15:50 PDT (History)
13 users (show)
benjamin: blocking1.9.2-
dveditz: blocking1.9.0.18+
dveditz: wanted1.9.0.x+
hskupin: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.1+
.1-fixed
.8+
.8-fixed


Attachments
Start of a patch (5.53 KB, patch)
2009-12-16 10:35 PST, Peter Van der Beken [:peterv]
no flags Details | Diff | Splinter Review
Patch (1.27 KB, patch)
2009-12-17 16:23 PST, Blake Kaplan (:mrbkap)
peterv: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
For 1.9.1 (1.69 KB, patch)
2009-12-22 16:58 PST, Blake Kaplan (:mrbkap)
dveditz: approval1.9.1.8+
Details | Diff | Splinter Review
Fix for that too (906 bytes, patch)
2009-12-23 13:46 PST, Blake Kaplan (:mrbkap)
peterv: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
For 1.9.2 (2.38 KB, patch)
2010-01-20 16:17 PST, Blake Kaplan (:mrbkap)
dveditz: approval1.9.2.1+
Details | Diff | Splinter Review
For 1.9.1 (2.67 KB, patch)
2010-01-20 16:19 PST, Blake Kaplan (:mrbkap)
dveditz: approval1.9.1.8+
dveditz: approval1.9.0.18+
Details | Diff | Splinter Review
Re-fix that (2.16 KB, patch)
2010-01-28 17:08 PST, Blake Kaplan (:mrbkap)
jst: review+
dveditz: approval1.9.1.8+
dveditz: approval1.9.0.18+
Details | Diff | Splinter Review
Fix for 1.8.0 (2.66 KB, patch)
2010-02-01 06:24 PST, Jan Horak
no flags Details | Diff | Splinter Review

Description moz_bug_r_a4 2009-11-27 02:01:19 PST
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.
Comment 5 Peter Van der Beken [:peterv] 2009-11-27 04:40:58 PST
(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>.".
Comment 6 moz_bug_r_a4 2009-11-27 07:47:09 PST
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).
Comment 7 Peter Van der Beken [:peterv] 2009-11-28 03:56:40 PST
(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?
Comment 8 moz_bug_r_a4 2009-11-29 00:28:29 PST
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).
Comment 9 Daniel Veditz [:dveditz] 2009-11-30 14:51:58 PST
Nominating for 1.9.2 in the absense of a multistate flag.
Comment 10 Benjamin Smedberg [:bsmedberg] 2009-12-01 11:01:35 PST
not a blocker per mrbkap
Comment 11 Peter Van der Beken [:peterv] 2009-12-16 10:35:31 PST
Created attachment 417954 [details] [diff] [review]
Start of a patch

For mrbkap. I gave up after we realized we need to audit all callers of XPCNativeWrapper::GetWrappedNative.
Comment 12 Blake Kaplan (:mrbkap) 2009-12-17 16:23:30 PST
Created attachment 418295 [details] [diff] [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 13 Boris Zbarsky [:bz] (still a bit busy) 2009-12-17 16:34:41 PST
Comment on attachment 418295 [details] [diff] [review]
Patch

File a followup on removing the dead code?
Comment 14 Blake Kaplan (:mrbkap) 2009-12-22 16:31:48 PST
(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 15 Blake Kaplan (:mrbkap) 2009-12-22 16:32:12 PST
Comment on attachment 417954 [details] [diff] [review]
Start of a patch

I filed bug 536480 for pursuing this patch.
Comment 16 Blake Kaplan (:mrbkap) 2009-12-22 16:50:41 PST
http://hg.mozilla.org/mozilla-central/rev/ce29e942a14d
Comment 17 Blake Kaplan (:mrbkap) 2009-12-22 16:56:42 PST
Comment on attachment 418295 [details] [diff] [review]
Patch

I'm not entirely sure what release to nominate this for, but 1.9.2.1 seems like a decent enough choice.
Comment 18 Blake Kaplan (:mrbkap) 2009-12-22 16:58:33 PST
Created attachment 418939 [details] [diff] [review]
For 1.9.1

The backport was trivial.
Comment 19 moz_bug_r_a4 2009-12-23 06:51:04 PST
The problem in nsXPCComponents_Utils::LookupMethod is not fixed (the patch in
comment 12 fixes testcase 3 but does not fix testcase 4).
Comment 20 Blake Kaplan (:mrbkap) 2009-12-23 13:46:51 PST
Created attachment 419048 [details] [diff] [review]
Fix for that too

Sorry, I missed that there were two different bugs here, somehow :-/.
Comment 21 Blake Kaplan (:mrbkap) 2010-01-05 15:05:59 PST
http://hg.mozilla.org/mozilla-central/rev/2f4c8116f515
Comment 22 Daniel Veditz [:dveditz] 2010-01-08 13:55:20 PST
Comment on attachment 418939 [details] [diff] [review]
For 1.9.1

Approved for 1.9.1.8, a=dveditz for release-drivers
Comment 23 Daniel Veditz [:dveditz] 2010-01-20 15:01:22 PST
Do we need branch approvals or a different branch patch for "Fix for that too" (attachment 419048 [details] [diff] [review])?
Comment 24 Blake Kaplan (:mrbkap) 2010-01-20 16:17:16 PST
Created attachment 422653 [details] [diff] [review]
For 1.9.2
Comment 25 Blake Kaplan (:mrbkap) 2010-01-20 16:19:17 PST
Created attachment 422654 [details] [diff] [review]
For 1.9.1
Comment 26 Blake Kaplan (:mrbkap) 2010-01-20 16:26:09 PST
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.
Comment 27 Mike Beltzner [:beltzner, not reading bugmail] 2010-01-20 23:36:43 PST
a192=beltzner
Comment 28 Blake Kaplan (:mrbkap) 2010-01-21 13:32:03 PST
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8e1fcf8f8b15
Comment 29 Daniel Veditz [:dveditz] 2010-01-22 13:20:52 PST
Does the 1.9.1 patch work for 1.9.0?
Comment 30 Daniel Veditz [:dveditz] 2010-01-22 13:21:45 PST
Comment on attachment 422654 [details] [diff] [review]
For 1.9.1

Approved for 1.9.1.8, a=dveditz for release-drivers
Comment 31 Daniel Veditz [:dveditz] 2010-01-26 15:29:28 PST
Comment on attachment 422654 [details] [diff] [review]
For 1.9.1

Approved for 1.9.0.18, a=dveditz
Comment 32 Blake Kaplan (:mrbkap) 2010-01-26 15:34:50 PST
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
Comment 33 moz_bug_r_a4 2010-01-27 18:50:14 PST
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
Comment 34 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2010-01-28 07:39:12 PST
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.
Comment 35 Jan Horak 2010-01-28 11:44:39 PST
At least testcase 3 is still positive for 1.8.0 with applied patch.
Comment 36 moz_bug_r_a4 2010-01-28 15:48:14 PST
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.
Comment 37 Blake Kaplan (:mrbkap) 2010-01-28 17:08:17 PST
Created attachment 424159 [details] [diff] [review]
Re-fix that
Comment 38 Jan Horak 2010-02-01 06:24:39 PST
Created attachment 424580 [details] [diff] [review]
Fix for 1.8.0

Based on https://bugzilla.mozilla.org/attachment.cgi?id=424159
Comment 39 Daniel Veditz [:dveditz] 2010-02-01 18:03:23 PST
Comment on attachment 424159 [details] [diff] [review]
Re-fix that

Approved for 1.9.1.8 and 1.9.0.18, a=dveditz
Comment 40 Blake Kaplan (:mrbkap) 2010-02-02 11:28:32 PST
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
Comment 41 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2010-02-04 01:13:33 PST
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
Comment 42 Al Billings [:abillings] 2010-02-05 15:49:54 PST
Verified for 1.9.0.18 on all platforms as well using attached testcases. All fixed now.

Note You need to log in before you can comment on or make changes to this bug.