XPCNativeWrapper can be used to circumvent security checks

VERIFIED FIXED

Status

()

Core
DOM
--
critical
VERIFIED FIXED
13 years ago
11 years ago

People

(Reporter: shutdown, Assigned: jst)

Tracking

({verified1.8})

Trunk
verified1.8
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.9 -
blocking-aviary1.0.5 -
blocking1.8b5 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical])

Attachments

(2 attachments)

(Reporter)

Description

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

Comment 1

13 years ago
Created attachment 188022 [details]
testcase

arbitrary code execution testcase.
Flags: blocking1.8b4+
Flags: blocking1.8b3?
Flags: blocking-aviary1.1?
Blocks: 256195
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

Comment 4

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

Comment 5

13 years ago
Created attachment 188359 [details] [diff] [review]
Prevent ever setting __proto__ on an XPCNativeWrapper.

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+
(Assignee)

Comment 8

13 years ago
Fixed.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Updated

13 years ago
Flags: blocking1.8b3?
Flags: blocking-aviary1.1?
(Assignee)

Comment 9

12 years ago
Fixed on the trunk before we branched for 1.8.
Keywords: fixed1.8
Whiteboard: [sg:critical]

Updated

12 years ago
Status: RESOLVED → VERIFIED
Keywords: fixed1.8 → verified1.8

Updated

12 years ago
Flags: testcase+
Group: security

Updated

11 years ago
Flags: in-testsuite+ → in-testsuite?
You need to log in before you can comment on or make changes to this bug.