436741 - (CVE-2008-5014) "Assertion failure: OBJ_IS_NATIVE(obj)" with __proto__ mangling

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes or hangs Firefox when loaded)

Loading the testcase kills Firefox.

Debug:
Assertion failure: OBJ_IS_NATIVE(obj), at /Users/jruderman/central/mozilla/js/src/jslock.cpp:1187

Opt:
Hang.

Security-sensitive for now because I don't know whether this is a memory safety bug in opt builds.

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Brendan Eich [:brendan]]

Blake, can you take this one?

/be

Comment 3

[Jeff Walden [:Waldo]]

mrbkap's out for a couple, few more weeks still, isn't he?

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Guess

This seems like an ancient bug dating back to bug 72354 (2001!). The code currently always locks obj2 when it is returned from a newresolve hook. Furthermore, when it finds out that obj2 is not native, it unlocks it. But both js_LockObj and js_UnlockObj (called directly via JS_{UN,}LOCK_OBJ) assert that the given object is native!

The fix proposed here is to not try to lock (or unlock) a non-native object. Note that in this case, the non-native object is a shavarray, but could just as easily be a liveconnect object.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 326322 [details] [diff] [review]
Guess

Yeah, this makes sense. Although for liveconnect do we have thread safety issues? For shavarrays my hope is to convert 'em to sparse upon crossing a thread boundary.

/be

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Better

Looking further down the loop shows that if the non-native object fails to resolve the id, we'll try to unlock the non-native obj2.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 326444 [details] [diff] [review]
Better

Glad someone's looking!

/be

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Fix pushed as changeset 8eac0738eaab.

Comment 9

[Mike Schroepfer]

Want for branch blake?

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 326444 [details] [diff] [review]
Better

Yeah. I think this is necessary for the array prototype functions getting "this" wrong bug.

Comment 11

[Samuel Sidler (old account; do not CC)]

Comment on attachment 326444 [details] [diff] [review]
Better

Approved for 1.9.0.2. Please land in CVS. a=ss

Comment 12

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/extensions/regress-436741.js

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.9 branch.

Comment 14

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0/trunk linux/mac/win.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 326444 [details] [diff] [review]
Better

This applies cleanly to the 1.8 branch.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 326444 [details] [diff] [review]
Better

Approved for 1.8.1.18, a=dveditz for release-drivers

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Fixed on the 1.8 branch too (although it isn't showing up in bonsai, probably because I hit ctl+c in the middle of checking in).

Comment 18

[Bob Clary [:bc] (inactive)]

v 1.8.1.18

Comment 19

[Alexander Sack]

Comment on attachment 326444 [details] [diff] [review]
Better

a=asac for 1.8.0

Comment 20

[Bob Clary [:bc] (inactive)]

test landed http://hg.mozilla.org/mozilla-central/rev/dcddc30c2960 and cvs