potential int overflow in jsstr.c tagify().

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
12 years ago
11 years ago

People

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

Tracking

({fixed1.8.0.4, fixed1.8.1})

Trunk
x86
Linux
fixed1.8.0.4, fixed1.8.1
Points:
---
Bug Flags:
blocking1.7.14 ?
blocking-aviary1.0.9 ?
blocking1.8.1 +
blocking1.8.0.4 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?])

Attachments

(2 attachments)

potential int overflow in jsstr.c tagify().

there is potential int overflow in jsstr.c tagify().

could only made it crash on a really perverted config: x86_64 linux with a
lot of virtual memory AND *32 bit compiled firefox*.

if it is possible to make a js string with length of 2^30-C with C small, this
may be exploitable in other configurations (personally can't make C less than
kilobytes on 32 bit linux yet).

testcase for 32 bit firefox for x64_64 linux to follow.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsstr.c&rev=3.125&mark=2162,2183,2187,2189#2162

in tagify:
2183 parlen = JSSTRING_LENGTH(param);
2187 taglen += JSSTRING_LENGTH(str) + 2 + endlen + 1;    /* 'str</end>' */
2189 tagbuf = (jschar *) JS_malloc(cx, (taglen + 1) * sizeof(jschar));

so if str == param and JSTRING_LENGTH == 2^30-1

(2^30-1+2^30-1+C)*sizeof(jschar) will overflow 2^32-1
Created attachment 219879 [details]
int overflow for x86_64 with 32 bit firefox

int overflow for x86_64 with 32 bit firefox
(Reporter)

Updated

12 years ago
Component: General → JavaScript Engine
Product: Firefox → Core
Assignee: nobody → mrbkap
Whiteboard: [sg:critical?]
(Assignee)

Comment 2

12 years ago
Created attachment 219918 [details] [diff] [review]
fix

Don't see how earlier sums could exceed 2^32-1, so all we need is a length test.

BTW, I extracted this by hand from a jsstr.c with other changes, so line numbers are off -- patch(1) will cope.

/be
Attachment #219918 - Flags: review?(mrbkap)
(In reply to comment #2)
> Created an attachment (id=219918) [edit]
> fix
> 
> Don't see how earlier sums could exceed 2^32-1, so all we need is a length
> test.
> 

brendan, let me explain that i crashed in the explained scenario.
according to you how much is:
2*2^30*sizeof(jschar)+C
C is small.


ooops, i flamed lamely.

my Comment #3 is buggy, so don't take it seriously.

i apologize.

Updated

12 years ago
Attachment #219918 - Flags: review?(mrbkap) → review+

Updated

12 years ago
Assignee: mrbkap → brendan
Fix checked into trunk.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Flags: blocking1.8.0.3?
Comment on attachment 219918 [details] [diff] [review]
fix

I think we want this on the branches.
Attachment #219918 - Flags: approval1.8.0.3?

Updated

12 years ago
Flags: blocking1.8.0.3? → blocking1.8.0.4?

Updated

12 years ago
Attachment #219918 - Flags: approval1.8.0.3? → approval1.8.0.4?
Flags: blocking1.8.0.4? → blocking1.8.0.4+
i suspect that strings/objects with byte size near 2GB may cause other int troubles on  32 bit systems.

what about patching JS_malloc/JS_realloc this way:

JS_PUBLIC_API(void *)
JS_malloc(JSContext *cx, size_t nbytes)
{
    void *p;

    JS_ASSERT(nbytes != 0);
    if (nbytes == 0)
        nbytes = 1;
+ if (nbytes > 50*1024*1024) return NULL;

the limit may be configurable and having in mind there is already check for 0 the performance hit probably will be small.

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

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #219918 - Flags: approval1.8.0.4? → approval1.8.0.4+
Please also remember to land on the 1.8 branch
Flags: blocking1.8.1+
Whiteboard: [sg:critical?] → [sg:critical?] needs branch landing
Fix checked into the 1.8 branches.
Keywords: fixed1.8.0.4, fixed1.8.1
Whiteboard: [sg:critical?] needs branch landing → [sg:critical?]

Comment 11

11 years ago
I don't know how to write this test for 32bit machines so that it doesn't run out of memory before finishing constructing the r string.
Flags: in-testsuite-
(In reply to comment #11)
> I don't know how to write this test for 32bit machines so that it doesn't run
> out of memory before finishing constructing the r string.
> 

i tried a lot of testcases with large strings, but with no luck on 32 bit machines. not sure about the reason for failure. the testcase should be very close to this one imho.

similar "issues" are Bug 336409 and Bug 336410
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
Group: security
You need to log in before you can comment on or make changes to this bug.