531364 - (CVE-2010-0171) Fix for bug 380474 does not work with security wrappers

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[moz_bug_r_a4]

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]]

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

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]]

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

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]]

Nominating for 1.9.2 in the absense of a multistate flag.

Comment 10

[Benjamin Smedberg]

not a blocker per mrbkap

Comment 11

[Peter Van der Beken [:peterv]]

Attachment: 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) (inactive)]

Attachment: 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 [:bzbarsky]]

Comment on attachment 418295 [details] [diff] [review]
Patch

File a followup on removing the dead code?

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

(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) (inactive)]

Comment on attachment 417954 [details] [diff] [review]
Start of a patch

I filed bug 536480 for pursuing this patch.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/ce29e942a14d

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

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) (inactive)]

Attachment: For 1.9.1

The backport was trivial.

Comment 19

[moz_bug_r_a4]

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) (inactive)]

Attachment: Fix for that too

Sorry, I missed that there were two different bugs here, somehow :-/.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/2f4c8116f515

Comment 22

[Daniel Veditz [:dveditz]]

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]]

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) (inactive)]

Attachment: For 1.9.2

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: For 1.9.1

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

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]]

a192=beltzner

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8e1fcf8f8b15

Comment 29

[Daniel Veditz [:dveditz]]

Does the 1.9.1 patch work for 1.9.0?

Comment 30

[Daniel Veditz [:dveditz]]

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]]

Comment on attachment 422654 [details] [diff] [review]
For 1.9.1

Approved for 1.9.0.18, a=dveditz

Comment 32

[Blake Kaplan (:mrbkap) (inactive)]

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]

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][⌚️UTC+1]]

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 [:jhorak]]

At least testcase 3 is still positive for 1.8.0 with applied patch.

Comment 36

[moz_bug_r_a4]

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) (inactive)]

Attachment: Re-fix that

Comment 38

[Jan Horak [:jhorak]]

Attachment: Fix for 1.8.0

Based on https://bugzilla.mozilla.org/attachment.cgi?id=424159

Comment 39

[Daniel Veditz [:dveditz]]

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) (inactive)]

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][⌚️UTC+1]]

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 - ex-MoCo]]

Verified for 1.9.0.18 on all platforms as well using attached testcases. All fixed now.