795352 - Potential integer overflow in jsscan.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[gorny]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.4 (KHTML, like Gecko) Chrome/22.0.1229.79 Safari/537.4

Steps to reproduce:

I was looking at some code and stumbled over jsscan.cpp.

There's a potential integer overflow there which, if triggerable at all, will only be triggerable on 32-bit platforms with a lot of memory (since the input line needs to reach UINT_MAX). I couldn't find references to places in the code where sizeof(jschar) > 1 and I'm not sure what the behavior of the allocation routine is when the integer overflow occurs and it tries to allocate 0 bytes.

I figured I file it anyway.

Code as of this moment at: http://hg.mozilla.org/tracemonkey/file/74b2b46fca7d/js/src/jsscan.cpp

   505     if (report.lineno == lineno) {
   506         size_t linelength = userbuf.findEOL() - linebase;  <-- comes from somewhere else
   507 
   508         linechars = (jschar *)cx->malloc_((linelength + 1) * sizeof(jschar));  <--- integer overflow
   509         if (!linechars) {
   510             warning = false;
   511             goto out;
   512         }
   513         memcpy(linechars, linebase, linelength * sizeof(jschar));  <-- memory corruption
   514         linechars[linelength] = 0;
   515         linebytes = DeflateString(cx, linechars, linelength);
   516         if (!linebytes) {
   517             warning = false;
   518             goto out;
   519         }


Actual results:

-


Expected results:

-

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

The tracemonkey repo fell into disuse over a year ago. The latest code is at http://hg.mozilla.org/mozilla-central. And it looks like that code uses a StringBuffer now:
http://hg.mozilla.org/mozilla-central/file/10279bd74ee0/js/src/frontend/TokenStream.cpp#l542

Comment 2

[Jeff Walden [:Waldo]]

Thanks for the report, looks like a pretty clear issue to me, albeit probably one tricky (impossible on Windows due to the 2GB memory limit?) to exploit, as you note.  In the future when you're reporting integer overflows and similar security issues, would you mind checking the security-sensitive checkbox when you file?  In this case the bullet's mostly dodged so there seems likely to be little harm, but that might not be the case the next time around.

This code got rewritten to use StringBuffer in bug 634444, which fix seems to have landed for 16, which should be out in a little over a week if memory serves.  Hiding for now, although I don't think there's anything we need to do here, just wait for the next release...

Comment 3

[Jeff Walden [:Waldo]]

CCing Nick primarily to feel gleefully vindicated about agitating for the rewrite (bug 634444 comment 40), secondarily because he may find this interesting.  :-)

Comment 4

[gorny]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Thanks for the report, looks like a pretty clear issue to me, albeit
> probably one tricky (impossible on Windows due to the 2GB memory limit?) to
> exploit, as you note.  In the future when you're reporting integer overflows
> and similar security issues, would you mind checking the security-sensitive
> checkbox when you file?  In this case the bullet's mostly dodged so there
> seems likely to be little harm, but that might not be the case the next time
> around.

I know; I saw it and didn't do it on purpose since I couldn't find any exploitable condition and discussed it with several folks and we deemed it a defense in depth issue. I found this during a source code audit for a customer of mine who use the JS engine in their own software and am an informatoin security consultant by trade. But my apologies nevertheless.

> This code got rewritten to use StringBuffer in bug 634444, which fix seems
> to have landed for 16, which should be out in a little over a week if memory
> serves.  Hiding for now, although I don't think there's anything we need to
> do here, just wait for the next release...

Sounds good. I'll let my customer know to keep an eye out for the update.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Also, I just wanted to point out that Luke fixed a bunch of these overflow possibilities recently by replacing them with pod_malloc<jschar>(len), which checks that the multiplication doesn't overflow.

Comment 6

[Jeff Walden [:Waldo]]

(In reply to gorny from comment #4)
Fair enough.  For me at least, when it comes to security, I'd rather be paranoid than wrong, and err on the side of caution; the loss from keeping a bug unnecessarily hidden is small compared to the possible gain should it actually be exploitable.  :-)  Thanks again for the report!

Comment 7

[Daniel Veditz [:dveditz]]

Since this is fixed on trunk should we mark this fixed, or leave it open and change it to the ESR-10 branch which still need the fix? Since ESR triage tends to wait for bugs to be fixed before they look at them I think I'll go with the former.

Looks possible to backport bug 634444 fairly easily, but it will almost certainly not get approved less than a week from release. We'll don't need the regression fixes bug 635164 and bug 635235 since those were against an earlier version of the bug 634444 landing that got backed out and fixed.

Comment 8

[bhavana bajaj [:bajaj]]

marking this "wontfix" as it is sec moderate

Comment 9

[Jan de Mooij [:jandem]]

Per whiteboard and multiple comments in this bug, this was fixed by bug 634444. 

Bug 634444 landed in Firefox 16 so I think we can close this now.

Comment 10

[David Chan [:dchan]]

Cleaning up list of security bugs for b2g18. This bug doesn't need to be backported either due to it affecting a later version of Fx or another reason.