455633 - XPCNativeWrapper Crash [@ XPCNativeWrapper::GetNewOrUsed]

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Gregory Fleischer]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1.16) Gecko/20080702 Firefox/2.0.0.16
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1.16) Gecko/20080702 Firefox/2.0.0.16

Calls to XPCNativeWrapper crash when invoked on Components object in Sandbox.

Firefox 3 example:
<link rel="stylesheet" type="text/css" href="javascript:XPCNativeWrapper(Components);"></link>

Firefox 2 example:
<img alt="" src="javascript:XPCNativeWrapper(Components);"></img>

Appears to be a basic null-pointer dereference, but I occasionally noticed subsequent invalid memory accesses so there may be additional corruption occurring.  At a minimum, this would be an annoying denial of service.

This may also be Bug 352134.


Reproducible: Always




Tested with user-agents:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b1pre) Gecko/20080916020338 Minefield/3.1b1pre

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.18pre) Gecko/20080915 BonEcho/2.0.0.18pre

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.17) Gecko/20080827 Firefox/2.0.0.17

Comment 1

[Gregory Fleischer]

Attachment: Generate crash using img tag

Test case to crash browser using img tag.

Comment 2

[Gregory Fleischer]

Attachment: Generate crash using link tag

Test case to cause browser crash using link tag

Comment 3

[Lucas Adamski [:ladamski]]

Confirmed that at least the second attachment (crash via link tag) crashes for me on 3.0.1 / OS X 10.5

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

It's also possible to hit this crash via PAC.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

As far as I can see, we have two options here:
- Null check when creating XPCNativeWrappers (taken here)
- Make the sandbox global object be a wrapped native

I think the second option is unnecessarily complicated, though I don't have terribly strong feelings on the matter. Boris, if you'd prefer, I'll implement the second option.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 340640 [details] [diff] [review]
Proposed fix

Hmm....  I _think_ this should be fine...  Is there any danger in chrome using a sandbox and then touching the polluted global and being screwed?

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #6)
> Hmm....  I _think_ this should be fine...  Is there any danger in chrome using
> a sandbox and then touching the polluted global and being screwed?

Once an untrusted script has been evaluated in a sandbox, the only safe way to touch the sandbox object anyway is to use XPCSafeJSObjectWrapper. So this patch doesn't really that question either way.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

By the way, I don't see how this crash could be anything but a null dereference crash... I'm not sure how we could try to read or write to any addresses after trying to deref null.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/7de3a1cdeb25

Comment 11

[Daniel Veditz [:dveditz]]

agree w/Gregory and mrbkap that this should always be a null-deref

> but I occasionally noticed subsequent invalid memory accesses so
> there may be additional corruption occurring.

If we find a testcase showing that it should be moved to a new bug, it's not likely to be related to the wrapper problem fixed here.

Comment 12

[Peter Van der Beken [:peterv]]

This caused bug 494445, so if it's ever landed on branches that patch should be taken too.

Comment 13

[Daniel Veditz [:dveditz]]

We probably don't need this on the 1.9.0 branch, the incidence of crashes at XPCNativeWrapper::GetNewOrUsed is very low (4 in the last week).

XPCWrappedNative::GetNewOrUsed is another story, 520 crashes in the last week on the 1.9.0 branch.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: 1.9.0 patch

Here's a rolled up patch for 1.9.0.

Comment 15

[Daniel Veditz [:dveditz]]

Blocking 1.9.0.12 for tracking purposes -- fixes regression bug 502458

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 386940 [details] [diff] [review]
1.9.0 patch

Approved for 1.9.0.12, a=dveditz

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Checking in js/src/xpconnect/src/XPCNativeWrapper.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/XPCNativeWrapper.cpp,v  <--  XPCNativeWrapper.cpp
new revision: 1.82; previous revision: 1.81
done

Comment 18

[Al Billings [:abillings - ex-MoCo]]

Verified fixed in 1.9.0.12 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.12pre) Gecko/2009070606 GranParadiso/3.0.12pre (.NET CLR 3.5.30729).

Comment 19

[Martin Stránský [:stransky] (ni? me)]

affects even 1.8.0.

Comment 20

[Jan Horak [:jhorak]]

Attachment: Patch for 1.8.0 version

Attached patch for 1.8.0 version. Thanks for your review in advance.

Comment 21

[Jan Horak [:jhorak]]

Attachment: Patch for 1.8.0 version, rev2

Fix also issue in #494445. Please can you check it? TIA.