335535 - potential int overflow in jsstr.c tagify().

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[georgi - hopefully not receiving bugspam]

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

Comment 1

[georgi - hopefully not receiving bugspam]

Attachment: int overflow for x86_64 with 32 bit firefox

int overflow for x86_64 with 32 bit firefox

Comment 2

[Brendan Eich [:brendan]]

Attachment: 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

Comment 3

[georgi - hopefully not receiving bugspam]

(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.

Comment 4

[georgi - hopefully not receiving bugspam]

ooops, i flamed lamely.

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

i apologize.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

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

I think we want this on the branches.

Comment 7

[georgi - hopefully not receiving bugspam]

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 8

[Daniel Veditz [:dveditz]]

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

approved for 1.8.0 branch, a=dveditz for drivers

Comment 9

[Daniel Veditz [:dveditz]]

Please also remember to land on the 1.8 branch

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.8 branches.

Comment 11

[Bob Clary [:bc] (inactive)]

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.

Comment 12

[georgi - hopefully not receiving bugspam]

(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