Closed Bug 479560 (CVE-2009-1841) Opened 15 years ago Closed 15 years ago

Arbitrary code execution using sidebar

Categories

(Core :: Security, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: moz_bug_r_a4, Assigned: mrbkap)

References

Details

(4 keywords, Whiteboard: [sg:critical])

Attachments

(2 files)

In nsSidebar.prototype.getInterfaces(), count is an object created in content,
thus it's unsafe to access count.value.
Whiteboard: [sg:critical]
Flags: blocking1.9.1?
Flags: blocking1.9.0.8?
Flags: blocking1.8.0.next?
Keywords: testcase
OS: Windows XP → All
Hardware: x86 → All
Assignee: nobody → mrbkap
Flags: wanted1.9.0.x+
Flags: wanted1.8.1.x+
Flags: blocking1.9.2?
Flags: blocking1.9.0.8?
Flags: blocking1.9.0.8+
Flags: blocking1.8.1.next?
Flags: blocking1.8.0.next?
For what it's worth, I have no idea how we're going to fix this in a generic way on the 1.8 branch. This will be fixed on trunk and 1.9.1 by XPCUnsafeJSObjectWrapper (name still TBD).
Blake, is there a bug for XPCUnsafeJSObjectWrapper?  And, can you comment on whether or not we should block 1.9.1 for this?
(In reply to comment #3)
> Blake, is there a bug for XPCUnsafeJSObjectWrapper?  And, can you comment on
> whether or not we should block 1.9.1 for this?

I talked this over with jst, and we decided that this should block. The situation we have right now (whack-a-mole) will require about as much work as adding the new wrapper and will fix problems we don't even know about. I'll file a new bug on adding the new wrapper type.
Depends on: cow
Please cc me.
Sorry, thought I did when I filed it -- fixed now.
Blocking, this could be bad. And this will likely be fixed by bug 480205.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Target Milestone: --- → mozilla1.9.1
Flags: blocking1.9.0.8+ → blocking1.9.0.9+
Attached patch FixSplinter Review
I don't know what to say.
Attachment #373256 - Flags: superreview?(jst)
Attachment #373256 - Flags: review?(jst)
Comment on attachment 373256 [details] [diff] [review]
Fix

Yeah, I hear ya :)
Attachment #373256 - Flags: superreview?(jst)
Attachment #373256 - Flags: superreview+
Attachment #373256 - Flags: review?(jst)
Attachment #373256 - Flags: review+
Landed: http://hg.mozilla.org/mozilla-central/rev/e31e971c0846
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9.1 → mozilla1.9.2a1
This is P1 now, as it's the actual required fix for the P1 bug 454363
Priority: P2 → P1
Target Milestone: mozilla1.9.2a1 → mozilla1.9.1
Comment on attachment 373256 [details] [diff] [review]
Fix

assuming this code hasn't changed much, approved for 1.9.0.10, a=dveditz for release-drivers
Attachment #373256 - Flags: approval1.9.0.10+
Rob, we better test this in Firebug, it's the kind of thing that we break on.
(In reply to comment #16)
> Rob, we better test this in Firebug, it's the kind of thing that we break on.

We'll need to convert this testcase to something we can run in Firebug or, preferably FBTest. I can't even make sense of what this testcase is doing right now, but it's crazy.

Once we've got one, we'll have to verify it against Firefox ≤ 3.0.9, ≤ 3.5b3 and then vs. 3.0.10 and 3.5b4. I wish I'd seen this last month. :/

Also, this brings up the issue of how do we want to store our Firebug security tests? They could be bad-times if we just publish them in our usual tests repository. Probably not the ideal location for this discussion, but we don't have a Firebug SG either.
I guess the other possibility is that we just check for regressions in Firebug as a result of this checkin. Probably the easier alternative.
For what it's worth, the only way this patch could break Firebug is if it sets a setter on Object.prototype.value and then calls a double-wrapped function that takes an out parameter and expects the setter to be called...
(In reply to comment #18)
> I guess the other possibility is that we just check for regressions in Firebug
> as a result of this checkin. Probably the easier alternative.

Sorry, I meant: we should run FBTest on the nightly 3.0 before this patch ships.
(In reply to comment #20)
> (In reply to comment #18)
> > I guess the other possibility is that we just check for regressions in Firebug
> > as a result of this checkin. Probably the easier alternative.
> 
> Sorry, I meant: we should run FBTest on the nightly 3.0 before this patch
> ships.

ah, good call. Panic averted. :)
Checking in js/src/xpconnect/src/xpcprivate.h;
/cvsroot/mozilla/js/src/xpconnect/src/xpcprivate.h,v  <--  xpcprivate.h
new revision: 1.284; previous revision: 1.283
done
Checking in js/src/xpconnect/src/xpcwrappedjsclass.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp,v  <--  xpcwrappedjsclass.cpp
new revision: 1.118; previous revision: 1.117
done
Keywords: fixed1.9.0.10
(In reply to comment #17)
> Once we've got one, we'll have to verify it against Firefox ≤ 3.0.9, ≤ 3.5b3
> and then vs. 3.0.10 and 3.5b4. I wish I'd seen this last month. :/

Is anyone working on a new testcase so we can verify this patch?
Flags: in-testsuite?
Keywords: testcase-wanted
Target Milestone: mozilla1.9.1 → mozilla1.9.2a1
Version: unspecified → Trunk
(In reply to comment #23)
> Is anyone working on a new testcase so we can verify this patch?

You can verify this patch using the testcase here. Comment 17 is referring to creating a testcase that the Firebug guys can use.
I verified this fixed with the testcase here for 1.9.0.11 with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.11pre) Gecko/2009051104 GranParadiso/3.0.11pre. I verified the bad behavior with 1.9.0.10.
Attached patch 1.8.0 patchSplinter Review
It's the same as 1.9 patch + there's JS_GetGlobalForObject() function from 1.9 line
Flags: blocking1.8.1.next?
Does this bug actually depend on bug 480205 ? I don't think it does with the fix we used ...
No, it doesn't.
No longer depends on: cow
Flags: blocking1.8.1.next?
Flags: blocking1.8.1.next? → blocking1.8.1.next+
Comment on attachment 377155 [details] [diff] [review]
1.8.0 patch

Approved for 1.8.1.22, a=dveditz for release-drivers
Attachment #377155 - Flags: approval1.8.1.next+
Checking in js/src/jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.214.2.45; previous revision: 3.214.2.44
done
Checking in js/src/jsapi.h;
/cvsroot/mozilla/js/src/jsapi.h,v  <--  jsapi.h
new revision: 3.107.2.13; previous revision: 3.107.2.12
done
Checking in js/src/xpconnect/src/xpcprivate.h;
/cvsroot/mozilla/js/src/xpconnect/src/xpcprivate.h,v  <--  xpcprivate.h
new revision: 1.162.2.22; previous revision: 1.162.2.21
done
Checking in js/src/xpconnect/src/xpcwrappedjsclass.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp,v  <--  xpcwrappedjsclass.cpp
new revision: 1.84.4.9; previous revision: 1.84.4.8
done
Keywords: fixed1.8.1.22
There is no way to test this for 1.8. There are no Firefox 1.8 builds anymore and it doesn't reproduce in the 1.8 Seamonkey builds.
(In reply to comment #31)
> There is no way to test this for 1.8. There are no Firefox 1.8 builds anymore
> and it doesn't reproduce in the 1.8 Seamonkey builds.

I can reproduce the testcase on SeaMonkey-1.1.16.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.21) Gecko/20090403
SeaMonkey/1.1.16 -> Components.stack alert appears

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.22pre) Gecko/20090602
SeaMonkey/1.1.17pre -> no alert appears
Weird. My mistake then.

Marking as verified for 1.8.1.22.
Flags: blocking1.9.2?
Group: core-security
Alias: CVE-2009-1841
You need to log in before you can comment on or make changes to this bug.