778603 - (CVE-2012-4204) Out of bounds read in str_unescape

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Scott Bell]

I ran into this issue while fuzzing.  ASAN reports this as a heap overflow.  I wasn't able to symbolize the ASAN output for some unknown reason.  However, I see it crashing at strange locations on different runs on optimized builds.

In debug builds, it hits this assertion and crashes @ NULL: 
Assertion failure: mLength + 1 <= mReserved, at ./../../dist/include/js/Vector.h:788

Windbg references this code: mozjs!str_unescape+0x23e [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\js\src\jsstr.cpp @ 352]

I have attached some logs from both debug and optimized builds.

Comment 1

[Scott Bell]

Attachment: ASAN Debug Build Output

Comment 2

[Scott Bell]

Attachment: ASAN Optimized Build Output

Comment 3

[Scott Bell]

Attachment: Windbg Debug Build Output

Comment 4

[Scott Bell]

Attachment: Windbg Optimized Build Output

Comment 5

[Scott Bell]

Attachment: Repro File

Comment 6

[Luke Wagner [:luke]]

Thanks for reporting!  The test-case reproduces in the shell.  A debug build asserts early reporting a call to infallibleAppend that has not reserved enough space.

Bug 708873 seems to be at fault; unfortunately the author is gone and the reviewer is on vacation.  I guess I'll look at it.

(cc'ing Jesse for fuzzer fodder.)

Comment 7

[Luke Wagner [:luke]]

Attachment: fix

Oh my, the bug is much simpler than that: there is an unsigned underflow in the expression (k > length - 6) when length < 6.

Comment 8

[Luke Wagner [:luke]]

Reduced testcase:
  unescape("0%u0000".substr(0,2))

Unfortunately, bug 708873 went out with FF11.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Per luke's request, I landed this fix on m-c inside an inbound merge commit rather than with its own changeset. You can confirm that the fix is present there.
https://hg.mozilla.org/mozilla-central/rev/61d7f15ea6a7

Comment 10

[Daniel Veditz [:dveditz]]

This bug was marked sec-high early before we understood the nature of the vulnerability. Understanding what the bug is now (out of bounds read), is that still appropriate? Will any part of this code write into an area assuming there's enough space because of the bogus length or it is only the read path that goes wrong?

Comment 11

[Luke Wagner [:luke]]

IIRC, there was the possibility of writing past the end of the buffer (infallibleAppend is being used), so this sec-high sounds appropriate.

Comment 13

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old