Last Comment Bug 778603 - (CVE-2012-4204) Out of bounds read in str_unescape
(CVE-2012-4204)
: Out of bounds read in str_unescape
Status: RESOLVED FIXED
[adv-track-main17+]
: crash, regression, sec-high
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: mozilla17
Assigned To: Luke Wagner [:luke]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 708873 779863
  Show dependency treegraph
 
Reported: 2012-07-29 18:33 PDT by Scott Bell
Modified: 2014-07-24 13:44 PDT (History)
9 users (show)
dveditz: sec‑bounty+
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
affected
affected
fixed
unaffected


Attachments
ASAN Debug Build Output (7.41 KB, text/plain)
2012-07-29 18:34 PDT, Scott Bell
no flags Details
ASAN Optimized Build Output (2.29 KB, text/plain)
2012-07-29 18:35 PDT, Scott Bell
no flags Details
Windbg Debug Build Output (6.13 KB, text/plain)
2012-07-29 18:36 PDT, Scott Bell
no flags Details
Windbg Optimized Build Output (2.82 KB, text/plain)
2012-07-29 18:36 PDT, Scott Bell
no flags Details
Repro File (112 bytes, text/plain)
2012-07-29 18:43 PDT, Scott Bell
no flags Details
fix (1.17 KB, patch)
2012-08-01 11:08 PDT, Luke Wagner [:luke]
dmandelin: review+
Details | Diff | Splinter Review

Description Scott Bell 2012-07-29 18:33:55 PDT
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 2012-07-29 18:34:35 PDT
Created attachment 647033 [details]
ASAN Debug Build Output
Comment 2 Scott Bell 2012-07-29 18:35:34 PDT
Created attachment 647034 [details]
ASAN Optimized Build Output
Comment 3 Scott Bell 2012-07-29 18:36:13 PDT
Created attachment 647035 [details]
Windbg Debug Build Output
Comment 4 Scott Bell 2012-07-29 18:36:35 PDT
Created attachment 647036 [details]
Windbg Optimized Build Output
Comment 5 Scott Bell 2012-07-29 18:43:58 PDT
Created attachment 647038 [details]
Repro File
Comment 6 Luke Wagner [:luke] 2012-08-01 10:43:43 PDT
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] 2012-08-01 11:08:35 PDT
Created attachment 648015 [details] [diff] [review]
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] 2012-08-01 11:22:21 PDT
Reduced testcase:
  unescape("0%u0000".substr(0,2))

Unfortunately, bug 708873 went out with FF11.
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-08-01 19:22:44 PDT
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] 2013-02-04 15:19:45 PST
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] 2013-02-04 15:23:42 PST
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] 2014-01-10 10:41:57 PST
mass remove verifyme requests greater than 4 months old

Note You need to log in before you can comment on or make changes to this bug.