Closed Bug 299450 Opened 19 years ago Closed 19 years ago

XPCNativeWrapper can be used to circumvent security checks

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: sync2d, Assigned: jst)

References

Details

(Keywords: verified1.8, Whiteboard: [sg:critical])

Attachments

(2 files)

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1)
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050701 Firefox/1.0+

XPC_NW_CheckAccess() always returns JS_TRUE for explicitly created
XPCNativeWrapper-s, so it can be used to circumvent security checks.
http://lxr.mozilla.org/seamonkey/ident?i=XPC_NW_CheckAccess

Chrome XBL method's arbitrary code execution problem has returned from hell.

see: bug 296397, bug 296902

Reproducible: Always

Steps to Reproduce:
1. load the testcase.
2. follow "invoke an exploit" link.
Actual Results:  
JavaScript object access check can be circumvented.

Expected Results:  
XPC_NW_CheckAccess() does appropriate security checks.
Attached file testcase
arbitrary code execution testcase.
Flags: blocking1.8b4+
Flags: blocking1.8b3?
Flags: blocking-aviary1.1?
Blocks: sbb?
Flags: blocking1.7.9+
Flags: blocking-aviary1.0.5+
This might be naive, but: why do we even allow content to construct an
XPCWrappedNative?
Because there's no real reason not to, and disallowing it would not solve the
problem -- chrome can end up passing XPCNativeWrappers to content by accident,
and we shouldn't let those cases be security holes.

In any case, the testcase shows the problem quite nicely; confirming.

I'm not sure why this is blocking the branch releases, unless we're planning to
port XPCNativeWrapper to the branch...
Blocks: 296902
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
We don't think this is a problem on the branch.  If anyone disagrees, please
renominate.

Assigning to jst.
Assignee: general → jst
Status: ASSIGNED → NEW
Flags: blocking1.7.9-
Flags: blocking1.7.9+
Flags: blocking-aviary1.0.5-
Flags: blocking-aviary1.0.5+
This fixes this exploit by preventing anyone from ever setting
wrapper.__proto__, and also makes us do a proper security check in our
checkAccess hook (like the engine does if we don't have a checkAccess hook),
just to be on the safe side.
Attachment #188359 - Flags: superreview?(brendan)
Attachment #188359 - Flags: review?(bzbarsky)
Comment on attachment 188359 [details] [diff] [review]
Prevent ever setting __proto__ on an XPCNativeWrapper.

>Index: js/src/xpconnect/src/XPCNativeWrapper.cpp
>+  if ((mode & JSACC_WATCH) == JSACC_PROTO && mode & JSACC_WRITE) {

Parens around the |mode & JSACC_WRITE| please, for clarity.

r=bzbarsky with that.
Attachment #188359 - Flags: review?(bzbarsky) → review+
Comment on attachment 188359 [details] [diff] [review]
Prevent ever setting __proto__ on an XPCNativeWrapper.

Looks good, sr+a=me -- thanks.

/be
Attachment #188359 - Flags: superreview?(brendan)
Attachment #188359 - Flags: superreview+
Attachment #188359 - Flags: approval1.8b3+
Fixed.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Flags: blocking1.8b3?
Flags: blocking-aviary1.1?
Fixed on the trunk before we branched for 1.8.
Keywords: fixed1.8
Whiteboard: [sg:critical]
Status: RESOLVED → VERIFIED
Keywords: fixed1.8verified1.8
Flags: testcase+
Group: security
Flags: in-testsuite+ → in-testsuite?
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: