Integer overflow when constructing Error.stack

VERIFIED FIXED in mozilla1.8.1

Status

()

defect
VERIFIED FIXED
13 years ago
11 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

({verified1.8.0.7, verified1.8.1})

Trunk
mozilla1.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

13 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

13 years ago
Posted patch Fix (obsolete) — Splinter Review
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

13 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

13 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

13 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

13 years ago
I committed the patch from comment 1 to the trunk.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Assignee

Comment 7

13 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

13 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

13 years ago
Posted patch Fix v2Splinter Review
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

13 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

13 years ago
I committed the patch from comment 9 to the trunk.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED

Comment 12

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

13 years ago
Flags: in-testsuite+
Assignee

Comment 13

13 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

13 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

13 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

13 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

13 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

13 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

13 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

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

Updated

13 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

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

Comment 23

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

Comment 24

13 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

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

Comment 27

13 years ago
verified fixed 1.8 20060818 windows/mac*/linux

Comment 28

13 years ago
verified fixed 1.8.0.7 20060824 windows/mac*/linux
Whiteboard: [sg:critical?] if you have enough memory?
Group: security

Comment 29

13 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

11 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.