502630 - Invalid pointer into hash table in jsatom

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Luke Wagner [:luke]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.11) Gecko/2009060308 Ubuntu/9.04 (jaunty) Firefox/3.0.11
Build Identifier: 

This is another instance of the mistake behind bug 501834.

The error is in JSAtomList::add, line 1118 (on tracemonkey).  Basically, after a JS_HashTableRawAdd, the given JSHashEntry** ('hep') can be invalidated when it points to the pointer in the internal table and the table is resized.

One fix would be to re-lookup the hep immediately after the Add.  However, looking inside the impl of RawAdd, this is already being done on resize.  Thus, a more efficient strategy would be for RawAdd to take 'hep' by reference.  Thus, hep would be kept valid and the comment on line 1123 would be correct.  In theory, this could mess with callers that previously assumed hep was passed by value, but except for this usage and the soon-to-be-removed use in jsarray, noone uses hep after a call to RawAdd because of the invalidation.

Reproducible: Always

Comment 1

[Brendan Eich [:brendan]]

Testsuite that botches an assertion in the patch I'm about to attach coming up.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: Luke's suggested fix, plus assertion to catch bug

Hurray for references.

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: test .js file, load in shell and do not assert-botch -> PASS

Comment 4

[Brendan Eich [:brendan]]

I'd say this is wanted1.9.1.x.

/be

Comment 5

[Graydon Hoare :graydon]

Harping on this point due to discussion of the recent 0-day: here we now have an invalid-pointer bug with a minimal .js file that triggers it, in a public bug. 

Take private?

(Likewise the linked bug 501834)

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #5)
> Harping on this point due to discussion of the recent 0-day: here we now have
> an invalid-pointer bug with a minimal .js file that triggers it, in a public
> bug. 
> 
> Take private?

AFAIK this bug is not exploitable. It stores a valid pointer into memory that was just freed.

/be

Comment 7

[Daniel Veditz [:dveditz]]

This use in jsatom appears to have been added with the upvar landing (bug 452598) and this problem doesn't occur on the 1.9.0 branch.

> It stores a valid pointer into memory that was just freed.

There's a chance that memory got immediately reused and you just corrupted something else. I'll grant that would be a pretty tight race and unlikely in this spot, but bugs like this have been exploited in the past
http://www.phreedom.org/presentations/heap-feng-shui/

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> There's a chance that memory got immediately reused and you just corrupted
> something else. I'll grant that would be a pretty tight race and unlikely in
> this spot, but bugs like this have been exploited in the past
> http://www.phreedom.org/presentations/heap-feng-shui/

You mean between threads? Not with jemalloc or the Mac malloc, AFAIK (again, please correct me if I'm wrong). There's no single-threaded reallocation hazard in this code.

/be

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> You mean between threads? Not with jemalloc or the Mac malloc, AFAIK (again,
> please correct me if I'm wrong). There's no single-threaded reallocation hazard
> in this code.

I mean the freed (oldbuckets) vector is not reallocated before the store into it via the stale hep.

C'mon, someone make a specific case for exploitability. We shouldn't have to restrict access to every non-NPE crash bug.

/be

Comment 10

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/7fdda31ac1fa (patch as reviewed)
http://hg.mozilla.org/tracemonkey/rev/f580b9ccafa5 (hide C++ from C includers)

/be

Comment 11

[Martijn Wargers (dead)]

(In reply to comment #7)
> This use in jsatom appears to have been added with the upvar landing (bug
> 452598) and this problem doesn't occur on the 1.9.0 branch.

Wrong bug number, it's not bug 452598.

Comment 12

[Robert Sayre]

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

Comment 13

[Daniel Veditz [:dveditz]]

request approval on the patch when you're ready to/want to land this on the 1.9.1 branch