348532 - Integer overflow when constructing Error.stack

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

The code to construct Error.stack property in InitExceptionObject from jsexn.c does not check for a possible integer overflow when reallocating stackbuf array. The overflow happens when a sufficiently huge string is present as argument multiple times on the stack. In this case the code would include its text multiple times. Since the buffer grows as a power of 2, at some point with sufficient memory (more than 2GB) stackmax will be increased from 2^30 to 2^31. 

Then on 32 bit CPU the line
JS_realloc(cx, stackbuf, (stackmax+1) * sizeof(jschar))
would allocate ((2^31+1) * sizeof(jschar)) % 2^32 or just 2 bytes. When realloc returns a pointer p to such chunk, the code would starting from at p+2GB address a user-controllable data.

Comment 1

[Igor Bukanov]

Attachment: Fix

The fix just limit the stack property length to 1MB.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 233473 [details] [diff] [review]
Fix

Looks good, my bad.  Looking at the code I wonder whether it's worth the trouble to avoid allocating power-of-two + 1 jschars.  Fodder for a future bug, if it matters.

/be

Comment 3

[Igor Bukanov]

(In reply to comment #2)
> Looking at the code I wonder whether it's worth the
> trouble to avoid allocating power-of-two + 1 jschars.  Fodder for a future bug,
> if it matters.

I filed bug 348545 to consolidate various realloc-to-build-string-buffer code.

Comment 4

[Igor Bukanov]

Here is a possible test case for the bug. Note that it needs to allocate more than 2 GB per 32 bit process so I could not test it as apparently Ubuntu kernel by default limits the user-space memory to 2GB.   

// construct string of 1<<23 characters
var s = Array((1<<23)+1).join('x');

var recursionDepth = 0;
function err() {
        if (++recursionDepth == 128)
                return new Error();
        return err.apply(this, arguments);
}

// The full stack trace in error would include 128*2 copies of s exceeding
//  2^23 * 256 or 2^31 in length
var error = err(s,s);

print(error.stack.length);

Comment 5

[Brendan Eich [:brendan]]

Another thought: why not trim strings and other overlong representations to be legible, say end with ... -- or something unique (requiring new lexical form?).

/be

Comment 6

[Igor Bukanov]

I committed the patch from comment 1 to the trunk.

Comment 7

[Igor Bukanov]

(In reply to comment #5)
> Another thought: why not trim strings and other overlong representations to be
> legible, say end with ... -- or something unique (requiring new lexical form?).

To get prettier looking traces it would be better besides limiting the overall stack length to limit the length of each argument's string to, say, 50 chars or so. But that is for another bug.

Comment 8

[Igor Bukanov]

I took away the previous commit as it broke windows builds: the patch introduced if before "void *ptr".

Comment 9

[Igor Bukanov]

Attachment: Fix v2

A fixed patch. I also replaced extra checks for stackmax exceeding STACK_LENGTH_LIMIT after calculating new buffer capacity by a simple check for length_ >= STACK_LENGTH_LIMIT - stacklen.

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 233547 [details] [diff] [review]
Fix v2

Red-faced second r+.

/be

Comment 11

[Igor Bukanov]

I committed the patch from comment 9 to the trunk.

Comment 12

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/GC/regress-348532.js

I'm not getting abnormal exit codes before or after the stack with the test case, but at least after the patch, the test outputs the stack length. The reported stack is kind of lame though:

Error()@:0
err(

length 15

Comment 13

[Igor Bukanov]

(In reply to comment #12)
> Created an attachment (id=233759) [edit]
> js1_5/GC/regress-348532.js
> 
> I'm not getting abnormal exit codes before or after the stack with the test
> case,  

This is expected unless you have a system where 32 bit process can allocate more than 2 GB of memory and when the bug is exposed. AFAIK this is not possible with 32-bits Windows XP, only 64-bit XP running 32 bits processes (or possibly 32 bit Windows Server) allow that.

> but at least after the patch, the test outputs the stack length. 

Without the patch the test case should fail with out-of-memory exception.

> The
> reported stack is kind of lame though:
> 
> Error()@:0
> err(

This is also OK since the patch aims for minimality, not pretty printing and simply cuts off too long arguments and everything after that. To get pretty printing, you may apply the patch for bug 348545.

Comment 14

[Bob Clary [:bc] (inactive)]

(In reply to comment #13)

> 
> Without the patch the test case should fail with out-of-memory exception.
> 

It didn't appear to use much more than 1G during my shell tests and I didn't get out of memory on my 4G box. I'll change the test to pass on either 0 or 3 exit codes though. 

Thanks!

Comment 15

[Igor Bukanov]

(In reply to comment #14)
> It didn't appear to use much more than 1G during my shell tests

That is strange, perhaps I miscalculated the size of allocated strings in the test.

> and I didn't get out of memory on my 4G box.

Do you mean 4G *32 bit* box? 

Comment 16

[Bob Clary [:bc] (inactive)]

(In reply to comment #15)

> Do you mean 4G *32 bit* box? 

Yes, and it is actually 3.25G usable. ;-)

Comment 17

[Igor Bukanov]

(In reply to comment #16)
> (In reply to comment #15)
> 
> > Do you mean 4G *32 bit* box? 
> 
> Yes, and it is actually 3.25G usable. ;-)

My knowledge about Windows is outdated ;)

In any case, since you got such a nice box, could you run a debugger to see what is the value of stackmax when InitExceptionObject from js/src/jsexn.c is about to return when running in the shell without the patch? I do not see what is wrong with the test case :( 



> 

Comment 18

[Bob Clary [:bc] (inactive)]

(In reply to comment #17)

> In any case, since you got such a nice box, could you run a debugger to see

paid for out of pocket! :-)

> what is the value of stackmax when InitExceptionObject from js/src/jsexn.c is
> about to return when running in the shell without the patch? 

without patch

+		argsrc	0x0036a8d0 {length=8388610 chars=0x206c0020 ""xxxx..." }	JSString *
		stackmax	536870912	unsigned int
		stacklen	260048111	unsigned int
+		stack	0x0000001a {length=??? chars=??? }	JSString *


done:
    if (ok)
        JS_RestoreExceptionState(cx, state);
    else
        JS_DropExceptionState(cx, state);
    JS_SetErrorReporter(cx, older);

    if (!ok) {
        JS_free(cx, stackbuf);
=>        return JS_FALSE;
    }

Comment 19

[Igor Bukanov]

(In reply to comment #18)
> without patch
> 
> +               argsrc  0x0036a8d0 {length=8388610 chars=0x206c0020 ""xxxx..."
> }        JSString *
>                 stackmax        536870912       unsigned int
>                 stacklen        260048111       unsigned int
> +               stack   0x0000001a {length=??? chars=??? }      JSString *
> 

It means that realloc() failed when realloc tried to allocate 2*(536870912 + 1) = 1GB + 2 bytes. Which is far from the necessary target of a successful 2 GB allocation. Which also tells that the bug is exploitable only in very exotic cases.

Comment 20

[Mike Schroepfer]

Ready for branch approval?

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 233547 [details] [diff] [review]
Fix v2

a=dbaron on behalf of drivers.  Please land on MOZILLA_1_8_BRANCH and add the fixed1.8.1 keyword once you have done so.

Comment 22

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 20060815 windows/mac(ppc|tel)/linux

Comment 23

[Igor Bukanov]

I committed the patch from comment 9 to MOZILLA_1_8_BRANCH.

Comment 24

[Igor Bukanov]

Comment on attachment 233547 [details] [diff] [review]
Fix v2

The patch applies to 1.8.0 branch as is.

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 233547 [details] [diff] [review]
Fix v2

approved for 1.8.0 branch, a=dveditz for drivers

Comment 26

[Igor Bukanov]

I committed the patch from comment 9 to MOZILLA_1_8_0_BRANCH.

Comment 27

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 20060818 windows/mac*/linux

Comment 28

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.0.7 20060824 windows/mac*/linux

Comment 29

[Bob Clary [:bc] (inactive)]

Checking in js1_5/GC/regress-348532.js;
/cvsroot/mozilla/js/tests/js1_5/GC/regress-348532.js,v  <--  regress-348532.js
initial revision: 1.1
done

Comment 30

[Bob Clary [:bc] (inactive)]

fwiw, bug 462265 http://hg.mozilla.org/mozilla-central/rev/85aa13a05d43 fixed the too much recursion error on js1_5/GC/regress-348532.js