Bug 531364 (CVE-2010-0171)

Fix for bug 380474 does not work with security wrappers

VERIFIED FIXED in mozilla1.9.3a1

Status

()

Core
Security
--
critical
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: moz_bug_r_a4, Assigned: mrbkap)

Tracking

({verified1.9.0.18, verified1.9.1, verified1.9.2})

Trunk
mozilla1.9.3a1
verified1.9.0.18, verified1.9.1, verified1.9.2
Points:
---
Bug Flags:
blocking1.9.2 -
blocking1.9.0.18 +
wanted1.9.0.x +
in-testsuite ?

Firefox Tracking Flags

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

Details

(Whiteboard: [sg:high][3.6.x])

Attachments

(6 attachments, 2 obsolete attachments)

(Reporter)

Description

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

Comment 6

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

Comment 8

8 years ago
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).
blocking1.9.1: --- → ?
Flags: wanted1.9.0.x?
Whiteboard: [sg:high]
Nominating for 1.9.2 in the absense of a multistate flag.
blocking1.9.1: ? → needed
status1.9.1: --- → wanted
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.2?
Whiteboard: [sg:high] → [sg:high][3.6.x]
not a blocker per mrbkap
Flags: blocking1.9.2? → blocking1.9.2-
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.
(Assignee)

Comment 12

8 years ago
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.
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Attachment #418295 - Flags: superreview?(bzbarsky)
Attachment #418295 - Flags: review?(peterv)
Attachment #418295 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 418295 [details] [diff] [review]
Patch

File a followup on removing the dead code?
Attachment #418295 - Flags: review?(peterv) → review+
(Assignee)

Comment 14

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

Comment 15

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

I filed bug 536480 for pursuing this patch.
Attachment #417954 - Attachment is obsolete: true
(Assignee)

Comment 16

8 years ago
http://hg.mozilla.org/mozilla-central/rev/ce29e942a14d
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 17

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

Comment 18

8 years ago
Created attachment 418939 [details] [diff] [review]
For 1.9.1

The backport was trivial.
Attachment #418939 - Flags: approval1.9.1.8?
(Reporter)

Comment 19

8 years ago
The problem in nsXPCComponents_Utils::LookupMethod is not fixed (the patch in
comment 12 fixes testcase 3 but does not fix testcase 4).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 20

8 years ago
Created attachment 419048 [details] [diff] [review]
Fix for that too

Sorry, I missed that there were two different bugs here, somehow :-/.
Attachment #419048 - Flags: superreview?(bzbarsky)
Attachment #419048 - Flags: review?(peterv)
Attachment #419048 - Flags: superreview?(bzbarsky) → superreview+
Attachment #419048 - Flags: review?(peterv) → review+
(Assignee)

Comment 21

8 years ago
http://hg.mozilla.org/mozilla-central/rev/2f4c8116f515
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
Comment on attachment 418939 [details] [diff] [review]
For 1.9.1

Approved for 1.9.1.8, a=dveditz for release-drivers
Attachment #418939 - Flags: approval1.9.1.8? → approval1.9.1.8+
blocking1.9.1: needed → .8+
Whiteboard: [sg:high][3.6.x] → [sg:high][3.6.x][needs 1.9.1 landing]
Do we need branch approvals or a different branch patch for "Fix for that too" (attachment 419048 [details] [diff] [review])?
blocking1.9.2: --- → ?
(Assignee)

Updated

7 years ago
Attachment #418295 - Flags: approval1.9.2.1?
(Assignee)

Comment 24

7 years ago
Created attachment 422653 [details] [diff] [review]
For 1.9.2
Attachment #418939 - Attachment is obsolete: true
Attachment #422653 - Flags: approval1.9.2.1?
(Assignee)

Comment 25

7 years ago
Created attachment 422654 [details] [diff] [review]
For 1.9.1
Attachment #422654 - Flags: approval1.9.1.8?
(Assignee)

Comment 26

7 years ago
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.
status1.9.1: wanted → .8-fixed
a192=beltzner
blocking1.9.2: ? → .1+
Attachment #422653 - Flags: approval1.9.2.1? → approval1.9.2.1+
(Assignee)

Comment 28

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8e1fcf8f8b15
status1.9.2: --- → .1-fixed
(Assignee)

Updated

7 years ago
Whiteboard: [sg:high][3.6.x][needs 1.9.1 landing] → [sg:high][3.6.x]
Does the 1.9.1 patch work for 1.9.0?
Flags: blocking1.9.0.18+
Comment on attachment 422654 [details] [diff] [review]
For 1.9.1

Approved for 1.9.1.8, a=dveditz for release-drivers
Attachment #422654 - Flags: approval1.9.1.8? → approval1.9.1.8+
(Assignee)

Updated

7 years ago
Attachment #422654 - Flags: approval1.9.0.18?
Comment on attachment 422654 [details] [diff] [review]
For 1.9.1

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

Comment 32

7 years ago
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
Keywords: fixed1.9.0.18
(Reporter)

Comment 33

7 years ago
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.
Severity: normal → critical
Status: RESOLVED → VERIFIED
status1.9.1: .8-fixed → ?
Keywords: fixed1.9.0.18 → verified1.9.2
Target Milestone: --- → mozilla1.9.3a1
Version: unspecified → Trunk
Flags: in-testsuite?

Comment 35

7 years ago
At least testcase 3 is still positive for 1.8.0 with applied patch.
(Reporter)

Comment 36

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

Comment 37

7 years ago
Created attachment 424159 [details] [diff] [review]
Re-fix that
Attachment #424159 - Flags: review?(jst)

Comment 38

7 years ago
Created attachment 424580 [details] [diff] [review]
Fix for 1.8.0

Based on https://bugzilla.mozilla.org/attachment.cgi?id=424159
Whiteboard: [sg:high][3.6.x] → [sg:high][3.6.x][needs 1.9.1 r=jst]
status1.9.1: ? → wanted

Updated

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

Updated

7 years ago
Attachment #424159 - Flags: approval1.9.1.8?
Attachment #424159 - Flags: approval1.9.0.18?
Attachment #424159 - Flags: approval1.9.1.8?
Attachment #424159 - Flags: approval1.9.1.8+
Attachment #424159 - Flags: approval1.9.0.18?
Attachment #424159 - Flags: approval1.9.0.18+
Comment on attachment 424159 [details] [diff] [review]
Re-fix that

Approved for 1.9.1.8 and 1.9.0.18, a=dveditz
Whiteboard: [sg:high][3.6.x][needs 1.9.1 r=jst] → [sg:high][3.6.x][needs 1.9.1/1.9.0 landing]
(Assignee)

Comment 40

7 years ago
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
status1.9.1: wanted → .8-fixed
Keywords: fixed1.9.0.18
Whiteboard: [sg:high][3.6.x][needs 1.9.1/1.9.0 landing] → [sg:high][3.6.x]
Attachment #422653 - Flags: approval1.9.2.2+ → approval1.9.2.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
Keywords: verified1.9.1
OS: Windows XP → All
Hardware: x86 → All
Verified for 1.9.0.18 on all platforms as well using attached testcases. All fixed now.
Keywords: fixed1.9.0.18 → verified1.9.0.18
Alias: CVE-2010-0171
Group: core-security
You need to log in before you can comment on or make changes to this bug.