Last Comment Bug 453310 - XPCNativeWrapper pollution using top-level statement of chrome JS
: XPCNativeWrapper pollution using top-level statement of chrome JS
Status: VERIFIED FIXED
: [sg:critical]
: testcase, verified1.8.1.19, verified1.9.0.5
Product: Core
Classification: Components
Component: XPConnect
: unspecified
: All All
: P2 normal (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap)
: xpconnect
:
:
: 441087
  Show dependency treegraph
 
Reported: 2008-09-02 07:57 PDT by moz_bug_r_a4
Modified: 2010-02-24 06:23 PST (History)
9 users (show)
jst: blocking1.9.1-
samuel.sidler+old: blocking1.9.0.5+
dveditz: wanted1.9.0.x+
samuel.sidler+old: blocking1.8.1.19+
dveditz: wanted1.8.1.x+
asac: blocking1.8.0.next+
See Also:
Crash Signature:


Attachments
Proposed fix (5.74 KB, patch)
2008-10-21 15:32 PDT, Blake Kaplan (:mrbkap)
brendan: review+
brendan: superreview+
dveditz: approval1.9.0.5+
Details | Diff | Splinter Review
Patch for the 1.8 branch (5.53 KB, patch)
2008-11-17 16:48 PST, Blake Kaplan (:mrbkap)
jst: review+
jst: superreview+
dveditz: approval1.8.1.19+
asac: approval1.8.0.next+
Details | Diff | Splinter Review

Summon comment box

Description moz_bug_r_a4 2008-09-02 07:57:39 PDT
User-Agent:       
Build Identifier: 

Please see bug 444077 and bug 441087.

When a top-level statement is executed, fp->callee is null.  Thus, it's
possible to circumvent the fix in bug 441087.


Reproducible: Always
Comment 1 moz_bug_r_a4 2008-09-02 07:58:54 PDT
Created attachment 336487 [details]
testcase 1

This works on trunk and fx3.0.x.
Comment 2 moz_bug_r_a4 2008-09-02 08:00:11 PDT
Created attachment 336488 [details]
testcase 2

This works on fx2.0.0.x.
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2008-09-24 16:47:57 PDT
Not going to block 1.9.1 on this after all, but Blake, please do investigate!
Comment 4 Blake Kaplan (:mrbkap) 2008-10-21 15:32:44 PDT
Created attachment 344195 [details] [review]
Proposed fix

I'm a little sad that nsScriptSecurityManager is peeking into xpcconvert here but avoiding it is more work than it's worth.

The core of the fix is the else branch just before the call to GetNewOrUsed where we pluck the script principal out if we didn't get a function object callee.
Comment 5 Blake Kaplan (:mrbkap) 2008-10-22 13:24:31 PDT
http://hg.mozilla.org/mozilla-central/rev/b91e44d05452
Comment 6 Blake Kaplan (:mrbkap) 2008-10-23 15:39:33 PDT
Since we're apparently opposed to taking bug 459906 on the 1.8 branch, there is no reason that this bug should block 1.8.1.18. I'll write up the backport patch for this bug soon, though, so it's sure to make the 1.8.1.19 and 1.9.0.5 releases.
Comment 7 Samuel Sidler (old account; do not CC) 2008-10-24 10:21:54 PDT
Yeah, we talked and took bug 459906 instead of this one for this release. This bug blocks 1.8.1.19/1.9.0.5 however (so don't stop working on it!).
Comment 8 Blake Kaplan (:mrbkap) 2008-11-12 18:33:05 PST
Comment on attachment 344195 [details] [review]
Proposed fix

This applies as-is to the 1.9 branch.
Comment 9 Daniel Veditz 2008-11-13 10:46:13 PST
Comment on attachment 344195 [details] [review]
Proposed fix

Approved for 1.9.0.5, a=dveditz for release-drivers 

We need a separate 1.8.1.x patch, right?
Comment 10 Blake Kaplan (:mrbkap) 2008-11-17 16:48:16 PST
Created attachment 348685 [details] [review]
Patch for the 1.8 branch

This fix is different from the one checked in on trunk. In particular, on the branch, it is much more expensive to get one's hands on an nsIScriptSecurityManager so it makes sense to do a little bit more work in XPCNativeWrapper since it has a script security manager already.

I was trying to avoid this on trunk, since IMO this signature is ugly and harder to use.
Comment 11 Blake Kaplan (:mrbkap) 2008-11-17 16:49:50 PST
Comment on attachment 348685 [details] [review]
Patch for the 1.8 branch

Oops, this is a patch for the 1.8 branch.
Comment 12 Blake Kaplan (:mrbkap) 2008-11-17 21:04:07 PST
Fixed on the 1.9 branch.
Comment 13 Daniel Veditz 2008-11-17 22:16:15 PST
Comment on attachment 348685 [details] [review]
Patch for the 1.8 branch

Approved for 1.8.1.19, a=dveditz for release-drivers
Comment 14 Blake Kaplan (:mrbkap) 2008-11-18 16:08:19 PST
Fixed on the 1.8 branch.
Comment 15 Al Billings [:abillings] 2008-11-25 15:53:41 PST
Verified for 1.8.1.19 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.19pre) Gecko/2008112503 BonEcho/2.0.0.19pre.

Verified for 1.9.0.5 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5pre) Gecko/2008112505 GranParadiso/3.0.5pre.
Comment 16 Josh Bressers 2008-12-11 05:24:26 PST
I think there is a problem with this patch.  I'm not sure if this has any security implication though.

If you load either of the attached testcases in the 3.0.5 beta, then double click in the page, when you bring up the context menu (right click), it's been corrupted, containing only: copy, select all, this frame >, and selection source.

There are other odd things going on as well, as if you try to say highlight some text on the page, you just end up trying to drag and drop the iframe.
Comment 17 moz_bug_r_a4 2008-12-11 21:58:45 PST
On fx3.0.x, when I double click in the page, the browser selects an empty
<html> element and an <iframe> element in the subframe.  It seems that the
behavior you mentioned is the right behavior in this situation.  (Even when
JavaScript is disabled, the behavior is the same.)
Comment 18 Alexander Sack 2008-12-16 00:59:13 PST
Comment on attachment 348685 [details] [review]
Patch for the 1.8 branch

a=asac for 1.8.0

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