Invalid pointer into hash table in jsatom

RESOLVED FIXED in mozilla1.9.2a1

Status

()

P1
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: luke, Assigned: brendan)

Tracking

({regression})

unspecified
mozilla1.9.2a1
regression
Points:
---
Bug Flags:
wanted1.9.0.x -
in-testsuite ?

Firefox Tracking Flags

(status1.9.1 wanted)

Details

(Whiteboard: [sg:nse] fixed-in-tracemonkey)

Attachments

(2 attachments)

(Reporter)

Description

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

Updated

9 years ago
Assignee: general → brendan
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: wanted1.9.1.x?
(Assignee)

Updated

9 years ago
OS: Linux → All
Priority: -- → P1
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.2a1
(Assignee)

Comment 1

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

/be
Flags: in-testsuite?
(Assignee)

Comment 2

9 years ago
Created attachment 388845 [details] [diff] [review]
Luke's suggested fix, plus assertion to catch bug

Hurray for references.

/be
Attachment #388845 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 3

9 years ago
Created attachment 388846 [details]
test .js file, load in shell and do not assert-botch -> PASS
(Assignee)

Comment 4

9 years ago
I'd say this is wanted1.9.1.x.

/be
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)
(Assignee)

Comment 6

9 years ago
(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

Updated

9 years ago
Group: core-security
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/
Blocks: 452598
blocking1.9.1: --- → ?
status1.9.1: --- → wanted
Flags: wanted1.9.0.x-
Keywords: regression
Whiteboard: [sg:low] unhide bug 501834 comment 5 and 6 when this bug unhidden
(Assignee)

Comment 8

9 years ago
(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

Updated

9 years ago
Attachment #388845 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 9

9 years ago
(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
blocking1.9.1: ? → ---
status1.9.1: wanted → ---
(Assignee)

Comment 10

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

/be
Whiteboard: [sg:low] unhide bug 501834 comment 5 and 6 when this bug unhidden → [sg:low] unhide bug 501834 comment 5 and 6 when this bug unhidden fixed-in-tracemonkey
(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.
No longer blocks: 452598

Comment 12

9 years ago
http://hg.mozilla.org/mozilla-central/rev/7fdda31ac1fa
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
request approval on the patch when you're ready to/want to land this on the 1.9.1 branch
status1.9.1: --- → wanted
Flags: wanted1.9.1.x?
Group: core-security
Whiteboard: [sg:low] unhide bug 501834 comment 5 and 6 when this bug unhidden fixed-in-tracemonkey → [sg:nse] fixed-in-tracemonkey
You need to log in before you can comment on or make changes to this bug.