Bypassing XPCNativeWrapper by redefining XPCNativeWrapper

VERIFIED FIXED

Status

()

P1
normal
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: moz_bug_r_a4, Assigned: mrbkap)

Tracking

({fixed1.8.0.15, verified1.8.1.12})

unspecified
fixed1.8.0.15, verified1.8.1.12
Points:
---
Bug Flags:
blocking1.9 +
blocking1.8.1.12 +
wanted1.8.1.x +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical])

Attachments

(4 attachments)

(Reporter)

Description

11 years ago
When creating an XPCNativeWrapper object, we get the prototype object for it
from content global scope.  But, content can override the prototype object by
redefining XPCNativeWrapper.

XPCNativeWrapper = function() {};
XPCNativeWrapper.prototype = x;
(Reporter)

Comment 1

11 years ago
Created attachment 284282 [details]
testcase - arbitrary code execution
Flags: blocking1.9?
Flags: blocking1.8.1.9?
Flags: blocking1.8.1.8?
OS: Windows XP → All
Hardware: PC → All
Whiteboard: [sg:critical]
We wouldn't want to rush a fix for this into 1.8.1.8 at this point.
Assignee: dveditz → mrbkap
Flags: blocking1.8.1.8?
Created attachment 286119 [details] [diff] [review]
Potential fix

So, the problem is that when we create the XPCNativeWrapper for chrome, its parent comes from content, so we'll search in the content window's scope to see what prototype we should give it. Properties on the prototype are then able to override the real properties. I'm fixing this by not giving any XPCNativeWrappers any prototype at all and moving the one function on the current real prototype directly on the wrapper itself. Some possible side-effects would be that other Object.prototype properties will disappear (like __define[GS]etter__), but I'm not convinced that we need to expose those through XPCNativeWrapper.
Attachment #286119 - Flags: superreview?(brendan)
Attachment #286119 - Flags: review?
Attachment #286119 - Flags: review? → review?(jst)
Comment on attachment 286119 [details] [diff] [review]
Potential fix

r=jst
Attachment #286119 - Flags: review?(jst) → review+

Updated

11 years ago
Attachment #286119 - Flags: superreview?(brendan) → superreview+

Updated

11 years ago
Flags: blocking1.9? → blocking1.9+

Comment 5

11 years ago
did this one land on the trunk, or is it waiting for post m9?
Priority: -- → P1

Comment 6

11 years ago
checkin needed?

Updated

11 years ago
Keywords: checkin-needed

Comment 7

11 years ago
Ping - Blake?  BKap?  Where are you... 
Pong! Checked in.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Flags: in-testsuite?

Comment 9

11 years ago
(In reply to comment #8)
> Pong! Checked in.
> 

Sweet - thanks!
(Reporter)

Comment 10

11 years ago
Explicit XPCNativeWrappers still use a prototype that comes from content.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 11

11 years ago
Created attachment 291176 [details]
testcase 2

Actual: xxx
Expected: undefined
Created attachment 291233 [details] [diff] [review]
I could have sworn I did this before...
Attachment #291233 - Flags: superreview?(jst)
Attachment #291233 - Flags: review?(jst)
Attachment #291233 - Flags: approval1.9?
Er, please ignore the parser/htmlparser stuff in that patch...
Status: REOPENED → ASSIGNED
Comment on attachment 291233 [details] [diff] [review]
I could have sworn I did this before...

re-request 1.9 approval once reviews are completed.  Thanks, bkap!
Attachment #291233 - Flags: approval1.9?
Comment on attachment 291233 [details] [diff] [review]
I could have sworn I did this before...

r+sr=jst for the XPCNativeWrapper.cpp changes, but the nsHTMLTokens.cpp changes are unrelated to this bug, right?
Attachment #291233 - Flags: superreview?(jst)
Attachment #291233 - Flags: superreview+
Attachment #291233 - Flags: review?(jst)
Attachment #291233 - Flags: review+
Also, Blake, no approval request is actually needed to land this as it's a P1 beta 2 blocker.
Fixed again.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.12?
Flags: blocking1.8.1.12+
Created attachment 294508 [details] [diff] [review]
1.8 branch patch

The only hand merging here was trivial (EnsureLegalActivity doesn't exist on the 1.8 branch and probably never should). This includes the patch for bug 407289 as well.
Attachment #294508 - Flags: review+
Attachment #294508 - Flags: approval1.8.1.12?
Comment on attachment 294508 [details] [diff] [review]
1.8 branch patch

approved for 1.8.1.12, a=dveditz for release-drivers
Attachment #294508 - Flags: approval1.8.1.12? → approval1.8.1.12+
Fixed on the 1.8 branch.
Keywords: fixed1.8.1.12
Verified in Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.12) Gecko/2008012822 Firefox/2.0.0.12. Also verified testcase repro in 2.0.0.11.
Keywords: fixed1.8.1.12 → verified1.8.1.12
Verified for trunk with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2008012804 Minefield/3.0b3pre.
Status: RESOLVED → VERIFIED
Group: security

Comment 23

11 years ago
Created attachment 310222 [details] [diff] [review]
1.8.0 patch (includes fix from cvs)

caillon, please sign off.
Attachment #310222 - Flags: approval1.8.0.15?
Comment on attachment 310222 [details] [diff] [review]
1.8.0 patch (includes fix from cvs)

a=caillon for 1.8.0.15
Attachment #310222 - Flags: approval1.8.0.15? → approval1.8.0.15+
MOZILLA_1_8_0_BRANCH:

Checking in js/src/xpconnect/src/XPCNativeWrapper.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/XPCNativeWrapper.cpp,v  <--  XPCNativeWrapper.cpp
new revision: 1.31.2.4.2.7; previous revision: 1.31.2.4.2.6
done
Keywords: fixed1.8.0.15
You need to log in before you can comment on or make changes to this bug.