303213 - integer overflow in js

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[georgi - hopefully not receiving bugspam]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6

there are 2 integer overflows in jsstr.c js_str_escape.

there is chance of exploitability with favorable heap layout or eventually if
possible race with another thread. expoloitation is not trivial.

(gdb) break jsstr.c:366
Breakpoint 1 at 0xb7fbe6ee: file /opt/firefox-cvs/mozilla/js/src/jsstr.c, line 366.
(gdb) cont
Breakpoint 1, js_str_escape (cx=0x92e1770, obj=0x9012d68, argc=1, 
    argv=0x94808d4, rval=0x73f46008)
    at /opt/firefox-cvs/mozilla/js/src/jsstr.c:366
366         newchars = (jschar *) JS_malloc(cx, (newlength + 1) * sizeof(jschar));
(gdb) p/x length
$1 = 0x15800000
(gdb) p/x newlength
$2 = 0x81000000
(gdb) p/x (newlength + 1) * sizeof(jschar)
$3 = 0x2000002
(gdb) cont
Continuing.

Program received signal SIGSEGV, Segmentation fault.
0xb7fbe7d2 in js_str_escape (cx=0x92e1770, obj=0x9012d68, argc=1, 
    argv=0xa1f33008, rval=0x30) at /opt/firefox-cvs/mozilla/js/src/jsstr.c:383
383                 newchars[ni++] = digits[ch >> 12];

proposed patch:
-------------------------
--- jsstr.c.orig        2005-08-03 11:45:17.000000000 +0300
+++ jsstr.c     2005-08-03 11:49:56.000000000 +0300
@@ -352,6 +352,7 @@
 
     /* Take a first pass and see how big the result string will need to be. */
     for (i = 0; i < length; i++) {
+       if (newlength + 2 >= ((size_t) -1)/sizeof(jschar)) return JS_FALSE;   
         if ((ch = chars[i]) < 128 && IS_OK(ch, mask))
             continue;
         if (ch < 256) {
-------------------------

it is strange the coverity tool didn't discover this.



Reproducible: Always

Steps to Reproduce:
url

Actual Results:  
SEGV

Expected Results:  
bill to write GNU man pages for living and mitchell to discover "text/plain"

Comment 1

[Brendan Eich [:brendan]]

Attachment: fix

No need to check every time through the loop, check after instead.  Also, don't
make a silent failure, report OOM before returning false.

/be

Comment 2

[Brendan Eich [:brendan]]

mrbkap, can you check the fix in if it meets your review approval?

/be

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 191501 [details] [diff] [review]
fix

r=me, though I've foolishly left my ssh keys in CA, so someone else will have
to check this in.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Timeless checked the patch in, marking FIXED.

Comment 5

[georgi - hopefully not receiving bugspam]

i would like to reopen, brendan's fix is buggy.

brendan: while i debugged one of my exploits, i noticed that:

newlength += 5

may also overflow "newlength" caz of "int too big problem".

try with length >= 4096M/6

i didn't place the check in the loop on vain, though i am dumb.

do you want a testcase for the overflow in the loop?

btw, my fix may be optimized a little to speed things.

Comment 6

[georgi - hopefully not receiving bugspam]

overflow of "newlength" has nothing to do with the multiplication with sizeof()
caz the overflow in a different manner.

btw, the movie "the corporation" is good.

Comment 7

[Daniel Veditz [:dveditz]]

Seems like this should be fixed on the branches, too. Especially since 1.7.x is
the EOL stable branch for that product.

Comment 8

[Brendan Eich [:brendan]]

Georgi: you may be dumb, but that makes me dumber -- thanks for reopening.

I'll try again.

/be

Comment 9

[Brendan Eich [:brendan]]

Georgi: yes, please attach a testcase and set the testcase+ flag.  Saves us from
having to retrace your steps.

/be

Comment 10

[Brendan Eich [:brendan]]

Attachment: real fix

mrbkap must redeem himself too.  Georgi's review welcome of course.

Ob. political response: the problem with "the corporation" is that it lets "the
government" off the hook.  Governments are the biggest, baddest, legally lethal
monopoly powers on the planet, and they make the laws that define, constrain,
and help corporations.	Mozilla Corporation is mainly about dealing with the
U.S. federal government's tax laws, it doesn't change much else.  So don't hate
us cuz we're "corporate" now (as if!).

/be

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 191518 [details] [diff] [review]
real fix

>+        ni = newlength;

Can't you just use |length| for this purpose?

Making:
>+        if (newlength < ni) {

if (newlength < length)

which is slightly clearer, IMO, and equally as correct.

>+            JS_ReportOutOfMemory(cx);
>+            return JS_FALSE;
>+        }
...
>         JS_ReportOutOfMemory(cx);
>         return JS_FALSE;

I also moved the duplicated error reporting code down into a label (overflow:)
at the bottom of the function, though I wasn't sure how necessary that was.

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #11)
> (From update of attachment 191518 [details] [diff] [review] [edit])
> >+        ni = newlength;
> 
> Can't you just use |length| for this purpose?

Not without reloading length after this first for loop (since it is used in the
second for loop's condition) from JSSTRING_LENGTH(str), which is an avoidable
redundant load and computation.

> >+            JS_ReportOutOfMemory(cx);
> >+            return JS_FALSE;
> >+        }
> ...
> >         JS_ReportOutOfMemory(cx);
> >         return JS_FALSE;
> 
> I also moved the duplicated error reporting code down into a label (overflow:)
> at the bottom of the function, though I wasn't sure how necessary that was. 

Please don't.  Decent compilers will cope, and gotos to manually fuse such a
short common tail are unseemly, even in SpiderMonkey.

/be

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 191518 [details] [diff] [review] [edit] [edit])
> > >+        ni = newlength;
> > 
> > Can't you just use |length| for this purpose?

I misread mrbkap's suggestion, it's obviously best: just let length stay at its
initial value (JSSTRING_LENGTH(str)) and test whether the post-incremented value
of newlength < length.  Duh (2nd baby, same sleep deprivation/interruption as
with 1st, my IQ is shot!  Glad you guys are awake).

/be

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: my take

This is what we discussed over IRC. It has the best of both worlds.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 191531 [details] [diff] [review]
my take

If Shaver gets to this before Brendan does, he should review. Whoever does
review/approve should also check this in, since I'm currently unable to.

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 191531 [details] [diff] [review]
my take

>+        /* 
>+         * Note: this works because newlength can be incremented by at most 5 

Period at end of sentence, and if the */ fits on that line without entering
column 80, why not use a minor one-line comment here?

>+         */

r=me with that fixed.  Thanks again!

/be

Comment 17

[Brendan Eich [:brendan]]

Shaver's at OSCON, one review suffices for js*.[ch] in general, I say check it in.

/be

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Timeless checked the second fix in. Hopefully that is the last newlength
overflow possible. Marking FIXED for real this time.

Comment 19

[georgi - hopefully not receiving bugspam]

the fix "my take" seems good.

ironically my original proposed fix seems buggy on border cases :)

the original testcase is in the url of the bug.

the new testcase (have not tested it on unpatched, so it may be buggy) is here:
http://www.guninski.com/mozbugs/esc2.html

brendan's last fix seems to fix both of them.

when tested on opera, these testcases give js error "object too large" and don't
crash and don't consume much memory.

Comment 20

[georgi - hopefully not receiving bugspam]

*** Bug 305190 has been marked as a duplicate of this bug. ***

Comment 21

[Brendan Eich [:brendan]]

Attachment: roll-up patch for AVIARY_1_0_1_20050124_BRANCH

Noting reviewed status from trunk.

/be

Comment 22

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

I confirmed that attachment 195803 [details] [diff] [review] is equivalent to the differences between
revisions 3.106 and 3.109, except for a one-character whitespace change in
another part of the file.

Comment 23

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

Checked in to MOZILLA_1_7_BRANCH and AVIARY_1_0_1_20050124_BRANCH.

Comment 24

[Bob Clary [:bc] (inactive)]

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

Comment 25

[Daniel Veditz [:dveditz]]

*** Bug 312102 has been marked as a duplicate of this bug. ***