integer overflow in js

VERIFIED FIXED in mozilla1.8beta4

Status

()

Core
JavaScript Engine
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: georgi - hopefully not receiving bugspam, Assigned: brendan)

Tracking

({fixed-aviary1.0.7, fixed1.7.12, js1.5})

Trunk
mozilla1.8beta4
x86
Linux
fixed-aviary1.0.7, fixed1.7.12, js1.5
Points:
---
Bug Flags:
blocking1.7.12 +
blocking-aviary1.0.7 +
blocking-aviary1.5 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix], URL)

Attachments

(2 attachments, 2 obsolete attachments)

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"
(Assignee)

Comment 1

12 years ago
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
Attachment #191501 - Flags: review?(mrbkap)
Attachment #191501 - Flags: approval1.8b4+
(Assignee)

Comment 2

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

/be
Status: UNCONFIRMED → NEW
Component: General → JavaScript Engine
Ever confirmed: true
Flags: review?(mrbkap) → blocking-aviary1.5+
Keywords: js1.5
Product: Firefox → Core
Target Milestone: --- → mozilla1.8beta4
Version: unspecified → 1.0 Branch
(Assignee)

Updated

12 years ago
Version: 1.0 Branch → Trunk
(Assignee)

Updated

12 years ago
Attachment #191501 - Flags: review?(mrbkap)
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.
Attachment #191501 - Flags: review?(mrbkap) → review+

Updated

12 years ago
Assignee: nobody → brendan
Timeless checked the patch in, marking FIXED.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
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.
(Reporter)

Updated

12 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.

Seems like this should be fixed on the branches, too. Especially since 1.7.x is
the EOL stable branch for that product.
Flags: blocking1.7.12?
Flags: blocking-aviary1.0.7?
Whiteboard: [sg:fix]
(Assignee)

Comment 8

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

I'll try again.

/be
Status: REOPENED → NEW
(Assignee)

Comment 9

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

/be
Flags: testcase?
(Assignee)

Comment 10

12 years ago
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
Attachment #191501 - Attachment is obsolete: true
Attachment #191518 - Flags: review?(mrbkap)
Attachment #191518 - Flags: approval1.8b4+
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.
(Assignee)

Comment 12

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

Comment 13

12 years ago
(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
Created attachment 191531 [details] [diff] [review]
my take

This is what we discussed over IRC. It has the best of both worlds.
Attachment #191518 - Attachment is obsolete: true
Attachment #191531 - Flags: review?(brendan)
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.
Attachment #191531 - Flags: review?(shaver)
(Assignee)

Comment 16

12 years ago
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
Attachment #191531 - Flags: review?(brendan)
Attachment #191531 - Flags: review+
Attachment #191531 - Flags: approval1.8b4+
(Assignee)

Comment 17

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

/be
Timeless checked the second fix in. Hopefully that is the last newlength
overflow possible. Marking FIXED for real this time.
Status: NEW → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Attachment #191531 - Flags: review?(shaver)

Updated

12 years ago
Attachment #191518 - Flags: review?(mrbkap)
Attachment #191518 - Flags: approval1.8b4+
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.
*** Bug 305190 has been marked as a duplicate of this bug. ***
Flags: blocking1.7.12?
Flags: blocking1.7.12+
Flags: blocking-aviary1.0.7?
Flags: blocking-aviary1.0.7+
Flags: blocking1.7.12?
Flags: blocking-aviary1.0.7?

Updated

12 years ago
Flags: blocking-aviary1.0.7? → blocking-aviary1.0.7+
(Assignee)

Comment 21

12 years ago
Created attachment 195803 [details] [diff] [review]
roll-up patch for AVIARY_1_0_1_20050124_BRANCH

Noting reviewed status from trunk.

/be
Attachment #195803 - Flags: review+
Attachment #195803 - Flags: approval1.7.12?
Attachment #195803 - Flags: approval-aviary1.0.7?
Flags: blocking1.7.12? → blocking1.7.12+
Attachment #195803 - Flags: approval1.7.12?
Attachment #195803 - Flags: approval1.7.12+
Attachment #195803 - Flags: approval-aviary1.0.7?
Attachment #195803 - Flags: approval-aviary1.0.7+
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.
Checked in to MOZILLA_1_7_BRANCH and AVIARY_1_0_1_20050124_BRANCH.
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8+
Keywords: fixed-aviary1.0.7, fixed1.7.12
(Assignee)

Updated

12 years ago
Status: RESOLVED → VERIFIED
Group: security

Comment 24

12 years ago
Checking in regress-303213.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-303213.js,v  <--  regress-303213.js
initial revision: 1.1
done
Flags: testcase? → testcase+
*** Bug 312102 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.