Integer overflow when constructing Error.stack

VERIFIED FIXED in mozilla1.8.1

Status

()

Core
JavaScript Engine
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({verified1.8.0.7, verified1.8.1})

Trunk
mozilla1.8.1
verified1.8.0.7, verified1.8.1
Points:
---
Bug Flags:
blocking1.8.1 +
blocking1.8.0.7 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] if you have enough memory?)

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
Created attachment 233473 [details] [diff] [review]
Fix

The fix just limit the stack property length to 1MB.
Assignee: general → igor.bukanov
Status: NEW → ASSIGNED
Attachment #233473 - Flags: review?(brendan)
Attachment #233473 - Flags: approval1.8.1?
Attachment #233473 - Flags: approval1.8.0.7?
(Assignee)

Updated

11 years ago
Flags: blocking1.8.1?
Flags: blocking1.8.0.7?
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
Attachment #233473 - Flags: review?(brendan) → review+
(Assignee)

Comment 3

11 years ago
(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.
(Assignee)

Comment 4

11 years ago
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);
Another thought: why not trim strings and other overlong representations to be legible, say end with ... -- or something unique (requiring new lexical form?).

/be
(Assignee)

Comment 6

11 years ago
I committed the patch from comment 1 to the trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 7

11 years ago
(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.
(Assignee)

Comment 8

11 years ago
I took away the previous commit as it broke windows builds: the patch introduced if before "void *ptr".
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 9

11 years ago
Created attachment 233547 [details] [diff] [review]
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.
Attachment #233473 - Attachment is obsolete: true
Attachment #233547 - Flags: review?(brendan)
Attachment #233473 - Flags: approval1.8.1?
Attachment #233473 - Flags: approval1.8.0.7?

Updated

11 years ago
Flags: blocking1.8.1? → blocking1.8.1+
Comment on attachment 233547 [details] [diff] [review]
Fix v2

Red-faced second r+.

/be
Attachment #233547 - Flags: review?(brendan) → review+
Flags: blocking1.8.0.7? → blocking1.8.0.7+
(Assignee)

Comment 11

11 years ago
I committed the patch from comment 9 to the trunk.
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Comment 12

11 years ago
Created attachment 233759 [details]
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

Updated

11 years ago
Flags: in-testsuite+
(Assignee)

Comment 13

11 years ago
(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

11 years ago
(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!
(Assignee)

Comment 15

11 years ago
(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

11 years ago
(In reply to comment #15)

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

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

Comment 17

11 years ago
(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

11 years ago
(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;
    }

(Assignee)

Comment 19

11 years ago
(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

11 years ago
Ready for branch approval?
Target Milestone: --- → mozilla1.8.1
(Assignee)

Updated

11 years ago
Attachment #233547 - Flags: approval1.8.1?
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.
Attachment #233547 - Flags: approval1.8.1? → approval1.8.1+

Comment 22

11 years ago
verified fixed 1.9 20060815 windows/mac(ppc|tel)/linux
Status: RESOLVED → VERIFIED
(Assignee)

Comment 23

11 years ago
I committed the patch from comment 9 to MOZILLA_1_8_BRANCH.
Keywords: fixed1.8.1
(Assignee)

Comment 24

11 years ago
Comment on attachment 233547 [details] [diff] [review]
Fix v2

The patch applies to 1.8.0 branch as is.
Attachment #233547 - Flags: approval1.8.0.7?
Comment on attachment 233547 [details] [diff] [review]
Fix v2

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #233547 - Flags: approval1.8.0.7? → approval1.8.0.7+
(Assignee)

Comment 26

11 years ago
I committed the patch from comment 9 to MOZILLA_1_8_0_BRANCH.
Keywords: fixed1.8.0.7

Comment 27

11 years ago
verified fixed 1.8 20060818 windows/mac*/linux
Keywords: fixed1.8.1 → verified1.8.1

Comment 28

11 years ago
verified fixed 1.8.0.7 20060824 windows/mac*/linux
Keywords: fixed1.8.0.7 → verified1.8.0.7
Whiteboard: [sg:critical?] if you have enough memory?
Group: security

Comment 29

11 years ago
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

9 years ago
fwiw, bug 462265 http://hg.mozilla.org/mozilla-central/rev/85aa13a05d43 fixed the too much recursion error on js1_5/GC/regress-348532.js
You need to log in before you can comment on or make changes to this bug.