615657 - (CVE-2011-0054) Buffer overflow/Memory corruption: cg->upvarList.count <= cg->upvarMap.length

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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)]

Attachment: Script to reproduce described crash

This script demonstrates the crash.

Comment 2

[Christian Holler (:decoder)]

Attachment: 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)]

Attachment: 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)]

Attachment: 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)]

Attachment: 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)]

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]]

Attachment: 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]]

Attachment: fix

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

/be

Comment 12

[David Mandelin [:dmandelin]]

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]]

http://hg.mozilla.org/tracemonkey/rev/fb3b0fd656bf

/be

Comment 14

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/fb3b0fd656bf

Comment 15

[Daniel Veditz [:dveditz]]

Does this bug affect FF3.5 or did upvars come in with 1.9.2?

Comment 16

[Brendan Eich [:brendan]]

This affects 3.5 too.

/be

Comment 17

[Brendan Eich [:brendan]]

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]]

Attachment: 191 patch

Pedestrian port to 191, nothing difficult to need re-review.

Comment 19

[Jeff Walden [:Waldo]]

Attachment: 192 patch

...also a trivial port.

Comment 20

[Daniel Veditz [:dveditz]]

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 21

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/83f326171713
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/28ce03bbd37c

Comment 22

[Al Billings [:abillings - ex-MoCo]]

(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)).