Last Comment Bug 303213 - integer overflow in js
: integer overflow in js
Status: VERIFIED FIXED
[sg:fix]
: fixed-aviary1.0.7, fixed1.7.12, js1.5
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: mozilla1.8beta4
Assigned To: Brendan Eich [:brendan]
:
Mentors:
http://www.guninski.com/mozbugs/esc1....
: 305190 312102 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-08-03 02:56 PDT by georgi - hopefully not receiving bugspam
Modified: 2006-03-12 18:46 PST (History)
7 users (show)
dbaron: blocking1.7.12+
mtschrep: blocking‑aviary1.0.7+
brendan: blocking‑aviary1.5+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (1.08 KB, patch)
2005-08-03 10:04 PDT, Brendan Eich [:brendan]
mrbkap: review+
brendan: approval1.8b4+
Details | Diff | Splinter Review
real fix (1.32 KB, patch)
2005-08-03 13:26 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
my take (1.21 KB, patch)
2005-08-03 15:29 PDT, Blake Kaplan (:mrbkap)
brendan: review+
brendan: approval1.8b4+
Details | Diff | Splinter Review
roll-up patch for AVIARY_1_0_1_20050124_BRANCH (1.30 KB, patch)
2005-09-12 15:14 PDT, Brendan Eich [:brendan]
brendan: review+
dbaron: approval‑aviary1.0.7+
dbaron: approval1.7.12+
Details | Diff | Splinter Review

Description georgi - hopefully not receiving bugspam 2005-08-03 02:56:58 PDT
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] 2005-08-03 10:04:32 PDT
Created attachment 191501 [details] [diff] [review]
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] 2005-08-03 10:07:30 PDT
mrbkap, can you check the fix in if it meets your review approval?

/be
Comment 3 Blake Kaplan (:mrbkap) 2005-08-03 10:36:07 PDT
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) 2005-08-03 11:08:38 PDT
Timeless checked the patch in, marking FIXED.
Comment 5 georgi - hopefully not receiving bugspam 2005-08-03 12:10:28 PDT
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 2005-08-03 12:13:20 PDT
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] 2005-08-03 13:00:38 PDT
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] 2005-08-03 13:16:24 PDT
Georgi: you may be dumb, but that makes me dumber -- thanks for reopening.

I'll try again.

/be
Comment 9 Brendan Eich [:brendan] 2005-08-03 13:19:20 PDT
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] 2005-08-03 13:26:57 PDT
Created attachment 191518 [details] [diff] [review]
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) 2005-08-03 13:54:03 PDT
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] 2005-08-03 15:11:49 PDT
(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] 2005-08-03 15:27:59 PDT
(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) 2005-08-03 15:29:20 PDT
Created attachment 191531 [details] [diff] [review]
my take

This is what we discussed over IRC. It has the best of both worlds.
Comment 15 Blake Kaplan (:mrbkap) 2005-08-03 15:38:06 PDT
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] 2005-08-03 15:41:48 PDT
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] 2005-08-03 15:49:02 PDT
Shaver's at OSCON, one review suffices for js*.[ch] in general, I say check it in.

/be
Comment 18 Blake Kaplan (:mrbkap) 2005-08-03 19:36:26 PDT
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 2005-08-04 01:15:59 PDT
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 2005-08-19 04:10:34 PDT
*** Bug 305190 has been marked as a duplicate of this bug. ***
Comment 21 Brendan Eich [:brendan] 2005-09-12 15:14:35 PDT
Created attachment 195803 [details] [diff] [review]
roll-up patch for AVIARY_1_0_1_20050124_BRANCH

Noting reviewed status from trunk.

/be
Comment 22 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-09-12 18:32:56 PDT
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+2 (review requests must explain patch) 2005-09-12 18:35:47 PDT
Checked in to MOZILLA_1_7_BRANCH and AVIARY_1_0_1_20050124_BRANCH.
Comment 24 Bob Clary [:bc:] 2005-10-09 22:53:57 PDT
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] 2005-10-11 16:52:39 PDT
*** Bug 312102 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.