Last Comment Bug 615657 - (CVE-2011-0054) Buffer overflow/Memory corruption: cg->upvarList.count <= cg->upvarMap.length
(CVE-2011-0054)
: Buffer overflow/Memory corruption: cg->upvarList.count <= cg->upvarMap.length
Status: RESOLVED FIXED
[sg:critical] fixed-in-tracemonkey
: verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Linux
: -- critical (vote)
: ---
Assigned To: Brendan Eich [:brendan]
:
Mentors:
Depends on:
Blocks: langfuzz
  Show dependency treegraph
 
Reported: 2010-11-30 12:21 PST by Christian Holler (:decoder)
Modified: 2014-09-04 09:39 PDT (History)
11 users (show)
rforbes: sec‑bounty+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
betaN+
.14+
.14-fixed
.17+
.17-fixed


Attachments
Script to reproduce described crash (740 bytes, application/javascript)
2010-11-30 12:22 PST, Christian Holler (:decoder)
no flags Details
Valgrind log for 32bit with 24 upVars on 1.9.2 (12.01 KB, text/plain)
2010-11-30 12:24 PST, Christian Holler (:decoder)
no flags Details
Valgrind log for 32bit with 32 upVars on 1.9.2 (4.72 KB, text/plain)
2010-11-30 12:24 PST, Christian Holler (:decoder)
no flags Details
Valgrind log for 32bit with 32 upVars on 2.0 (3.47 KB, text/plain)
2010-11-30 12:25 PST, Christian Holler (:decoder)
no flags Details
Valgrind log for 64bit with 32 upVars on 2.0 (7.59 KB, text/plain)
2010-11-30 12:26 PST, Christian Holler (:decoder)
no flags Details
testcase (click to crash) (843 bytes, text/html)
2010-12-08 11:29 PST, Daniel Veditz [:dveditz]
no flags Details
fix (1.61 KB, patch)
2010-12-08 14:34 PST, Brendan Eich [:brendan]
dmandelin: review+
Details | Diff | Review
191 patch (1.21 KB, patch)
2011-01-20 15:54 PST, Jeff Walden [:Waldo] (remove +bmo to email)
dveditz: approval1.9.1.17+
Details | Diff | Review
192 patch (1.22 KB, patch)
2011-01-20 15:54 PST, Jeff Walden [:Waldo] (remove +bmo to email)
dveditz: approval1.9.2.14+
Details | Diff | Review

Description Christian Holler (:decoder) 2010-11-30 12:21:22 PST
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.12) Gecko/20101027 Ubuntu/10.10 (maverick) Firefox/3.6.12
Build Identifier: 

The following problem affects all Spidermonkey versions I tested (1.9.2 and 2.0) and hence Firefox 3.5.x and Firefox 4. Under test were mozilla-2.0-297086a0fb61 and mozilla-1-9-2-053f07027a38:

There is a serious problem with the code that copies upvarMap.vector in jsscript.cpp. Using a combination of inner/outer functions with eval, we were able to trigger the following assertion (mozilla-2.0-297086a0fb61):

Assertion failure: cg->upvarList.count <= cg->upvarMap.length, at jsscript.cpp:1224

The respective code looks as follows:

if (cg->upvarList.count != 0) {
    JS_ASSERT(cg->upvarList.count <= cg->upvarMap.length);
    memcpy(script->upvars()->vector, cg->upvarMap.vector,
           cg->upvarList.count * sizeof(uint32));
    cg->upvarList.clear();
    cx->free(cg->upvarMap.vector);
    cg->upvarMap.vector = NULL;
}

In an optimized non-debug build, the memcpy simply either overruns the available space in script->upvars()->vector, or reads beyond cg->upvarMap.vector or both.

The attacker can control cg->upvarList.count arbitrarily here. We suspect the issue to be exploitable although it shouldn't be too easy (the attacker can hardly control what is actually written, only the amount).

I'll attach test.js to reproduce the problem and some valgrind logs showing different behavior depending on how large the upvar list gets. Using 24 additional upvars in the script triggers the assertion, using 32 crashes straight away without even asserting (I suspect two different problems here).

Reproducible: Always

Steps to Reproduce:
1. Run test.js
2. Observe crash/assertion
Actual Results:  
Crash/Assertion

Expected Results:  
Normal run.
Comment 1 Christian Holler (:decoder) 2010-11-30 12:22:57 PST
Created attachment 494087 [details]
Script to reproduce described crash

This script demonstrates the crash.
Comment 2 Christian Holler (:decoder) 2010-11-30 12:24:01 PST
Created attachment 494089 [details]
Valgrind log for 32bit with 24 upVars on 1.9.2

Valgrind log for 32bit with 24 upVars on 1.9.2
Comment 3 Christian Holler (:decoder) 2010-11-30 12:24:26 PST
Created attachment 494090 [details]
Valgrind log for 32bit with 32 upVars on 1.9.2

Valgrind log for 32bit with 32 upVars on 1.9.2
Comment 4 Christian Holler (:decoder) 2010-11-30 12:25:31 PST
Created attachment 494092 [details]
Valgrind log for 32bit with 32 upVars on 2.0

Valgrind log for 32bit with 32 upVars on 2.0
Comment 5 Christian Holler (:decoder) 2010-11-30 12:26:00 PST
Created attachment 494093 [details]
Valgrind log for 64bit with 32 upVars on 2.0

Valgrind log for 64bit with 32 upVars on 2.0
Comment 6 Johnny Stenback (:jst, jst@mozilla.com) 2010-11-30 13:18:02 PST
Brendan, this looks like upvar code, so handing to you to start with. Sounds like a critical bug to me, but please let me know if you disagree.
Comment 9 Daniel Veditz [:dveditz] 2010-12-08 11:29:44 PST
Created attachment 496209 [details]
testcase (click to crash)

Incorporated the PoC script into a loadable page, and increased the upvars to ensure a crash.  For some reason I hit the unresponsive script dialog twice just executing the loop 1000 times, which is odd because it doesn't do much except grow some strings. I've seen PoC's using tons more memory and loops that don't hit that warning.

I think the following crash happened when I closed the page after running the testcase, not directly when I clicked it.

bp-104c40b5-7653-4d6e-ac52-ff31e2101208
Comment 11 Brendan Eich [:brendan] 2010-12-08 14:34:14 PST
Created attachment 496287 [details] [diff] [review]
fix

I hate MakeUpvarForEval... (which means I hate SunSpider, which wants it -- but you knew that).

/be
Comment 12 David Mandelin [:dmandelin] 2010-12-08 16:41:25 PST
Comment on attachment 496287 [details] [diff] [review]
fix

It's cool how the assert basically documents what the other part of the patch is for. Made it super-easy to review.
Comment 13 Brendan Eich [:brendan] 2010-12-10 17:04:32 PST
http://hg.mozilla.org/tracemonkey/rev/fb3b0fd656bf

/be
Comment 15 Daniel Veditz [:dveditz] 2011-01-03 10:50:42 PST
Does this bug affect FF3.5 or did upvars come in with 1.9.2?
Comment 16 Brendan Eich [:brendan] 2011-01-03 16:44:11 PST
This affects 3.5 too.

/be
Comment 17 Brendan Eich [:brendan] 2011-01-04 10:37:55 PST
I don't know how to prove this can't be exploited. With enough work, it probably could be, although it is not as easy to exploit as sg:criticals involving free memory reads during virtual call instruction sequences.

/be
Comment 18 Jeff Walden [:Waldo] (remove +bmo to email) 2011-01-20 15:54:16 PST
Created attachment 505586 [details] [diff] [review]
191 patch

Pedestrian port to 191, nothing difficult to need re-review.
Comment 19 Jeff Walden [:Waldo] (remove +bmo to email) 2011-01-20 15:54:56 PST
Created attachment 505588 [details] [diff] [review]
192 patch

...also a trivial port.
Comment 20 Daniel Veditz [:dveditz] 2011-01-21 10:22:28 PST
Comment on attachment 505586 [details] [diff] [review]
191 patch

Approved for 1.9.2.14 and 1.9.1.17, a=dveditz for release-drivers
Comment 22 [On PTO until 6/29] 2011-01-25 15:17:48 PST
(In reply to comment #9)
> Created attachment 496209 [details]
> testcase (click to crash)
> 
> Incorporated the PoC script into a loadable page, and increased the upvars to
> ensure a crash.  For some reason I hit the unresponsive script dialog twice
> just executing the loop 1000 times, which is odd because it doesn't do much
> except grow some strings. I've seen PoC's using tons more memory and loops that
> don't hit that warning.


Verified for 1.9.2 using this testcase. Crashes in 1.9.2.13 and verified fixed in 1.9.14 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.14) Gecko/20110121 Firefox/3.6.14 ( .NET CLR 3.5.30729)).

Same with 1.9.1.16 and verified fixed in 1.9.17 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.17) Gecko/20110121 Firefox/3.5.17 ( .NET CLR 3.5.30729)).

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