446494 - Buffer overflow in Number.toLocaleString()

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Philip Taylor]

Using SpiderMonkey from CVS trunk:

$ LANG=C valgrind ./js -e '1e-10.toLocaleString()'

==3190== Invalid read of size 1
==3190==    at 0x80A5DD0: num_toLocaleString (jsnum.c:367)
==3190==    by 0x81226A4: js_Interpret (jsinterp.c:4835)
==3190==    by 0x80A0D96: js_Execute (jsinterp.c:1536)
==3190==    by 0x80620DB: JS_EvaluateUCScriptForPrincipals (jsapi.c:4999)
==3190==    by 0x8062049: JS_EvaluateUCScript (jsapi.c:4980)
==3190==    by 0x8061F53: JS_EvaluateScript (jsapi.c:4946)
==3190==    by 0x804A076: ProcessArgs (js.c:518)
==3190==    by 0x804F4D5: main (js.c:3931)
==3190==  Address 0x41cc0ce is 0 bytes after a block of size 6 alloc'd
==3190==    at 0x402291D: malloc (in /usr/lib/valgrind/x86-linux/vgpreload_memcheck.so)
==3190==    by 0x805BBE0: JS_malloc (jsapi.c:1791)
==3190==    by 0x80F6F62: js_DeflateString (jsstr.c:2927)
==3190==    by 0x80F7B08: js_GetStringBytes (jsstr.c:3232)
==3190==    by 0x80A5BDB: num_toLocaleString (jsnum.c:323)
==3190==    by 0x81226A4: js_Interpret (jsinterp.c:4835)
==3190==    by 0x80A0D96: js_Execute (jsinterp.c:1536)
==3190==    by 0x80620DB: JS_EvaluateUCScriptForPrincipals (jsapi.c:4999)
==3190==    by 0x8062049: JS_EvaluateUCScript (jsapi.c:4980)
==3190==    by 0x8061F53: JS_EvaluateScript (jsapi.c:4946)
==3190==    by 0x804A076: ProcessArgs (js.c:518)
==3190==    by 0x804F4D5: main (js.c:3931)
==3190==
==3190== Invalid write of size 1
==3190==    at 0x80A5E80: num_toLocaleString (jsnum.c:384)
==3190==    by 0x81226A4: js_Interpret (jsinterp.c:4835)
==3190==    by 0x80A0D96: js_Execute (jsinterp.c:1536)
==3190==    by 0x80620DB: JS_EvaluateUCScriptForPrincipals (jsapi.c:4999)
==3190==    by 0x8062049: JS_EvaluateUCScript (jsapi.c:4980)
==3190==    by 0x8061F53: JS_EvaluateScript (jsapi.c:4946)
==3190==    by 0x804A076: ProcessArgs (js.c:518)
==3190==    by 0x804F4D5: main (js.c:3931)
==3190==  Address 0x41cc146 is 0 bytes after a block of size 6 alloc'd
==3190==    at 0x402291D: malloc (in /usr/lib/valgrind/x86-linux/vgpreload_memcheck.so)
==3190==    by 0x805BBE0: JS_malloc (jsapi.c:1791)
==3190==    by 0x80A5D98: num_toLocaleString (jsnum.c:360)
==3190==    by 0x81226A4: js_Interpret (jsinterp.c:4835)
==3190==    by 0x80A0D96: js_Execute (jsinterp.c:1536)
==3190==    by 0x80620DB: JS_EvaluateUCScriptForPrincipals (jsapi.c:4999)
==3190==    by 0x8062049: JS_EvaluateUCScript (jsapi.c:4980)
==3190==    by 0x8061F53: JS_EvaluateScript (jsapi.c:4946)
==3190==    by 0x804A076: ProcessArgs (js.c:518)
==3190==    by 0x804F4D5: main (js.c:3931)

jsnum.c:367 is the line "while (*tmpSrc == '-' || remainder--)".
jsnum.c:384 is the line "*tmpDest++ = '\0';".

Comment 1

[Brian Crowder]

So we don't handle exponential notation in this routine at all.

Comment 2

[Philip Taylor]

Non-finite values aren't handled properly either -- Infinity.toLocaleString() == "In,fin,ity" in my default locale (en_GB).

Comment 3

[Brian Crowder]

Attachment: v1

Will ask for review after testing.

Comment 4

[Brian Crowder]

Attachment: v2

Comment 5

[Brian Crowder]

Attachment: v3, leave room for '\0'

Since we use strcpy() later, we need the '\0'.  It's semi-extraneous, since we use JS_NewString at the end (ie., we don't need the terminator), but the string routines are convenient and clean, so I think it's a worthwhile tradeoff.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 330794 [details] [diff] [review]
v3, leave room for '\0'

>--- mozilla.7c4f66de2dee/js/src/jsnum.cpp	2008-07-22 10:24:36.000000000 -0700

>+    while(*nint >= '0' && *nint <= '9')
>+        nint++;

Nit: space after while. r=mrbkap with that.

Comment 7

[Brian Crowder]

changeset:   16181:c37a070f8397

Comment 8

[Brian Crowder]

Attachment: 1.9 branch patch (jsnum.cpp => jsnum.c

Comment 9

[Brian Crowder]

Attachment: 1.8 branch patch

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #6)
> (From update of attachment 330794 [details] [diff] [review])
> >--- mozilla.7c4f66de2dee/js/src/jsnum.cpp	2008-07-22 10:24:36.000000000 -0700
> 
> >+    while(*nint >= '0' && *nint <= '9')
> >+        nint++;
> 
> Nit: space after while. r=mrbkap with that.

Uber-nit: usually SpiderMonkey style uses number-line order: '0' <= c && c <= '9'. Could also rearrange (check that gcc doesn't do this) to avoid a branch:

(uintN)(c - '0') <= '9' - '0'

or

(uintN)(c - '0') < 10

/be

Comment 11

[Samuel Sidler (old account; do not CC)]

Can we get an automated test for this before landing on the 1.9 branch?

Comment 12

[Daniel Veditz [:dveditz]]

QA doesn't want to take on the 1.8 branch without a testcase, and not on the 1.9.0 branch without an automated one (which was probably a requirement for mozilla-central, right?)

Comment 13

[Brian Crowder]

The original bug will be difficult to write a testcase for, but the In,fin,ity one and similar bugs may not be.  The reason for difficulty in writing a case for the original bug is that the overflow is caused by an off-by-one in the buffer math, which means we usually don't overflow the space provided by malloc().  I'll see if I can figure out something more consistent/reliable.

Comment 14

[Bob Clary [:bc] (inactive)]

Attachment: ecma_3/Number/15.7.4.3-02.js

Even if we can't get a reliable crash for the overrun, we can still detect this using valgrind. I'll check this test in later today.

Comment 15

[Brian Crowder]

dveditz:  Am I cleared-to-land for branches, once the testcase is landed?

Comment 16

[Samuel Sidler (old account; do not CC)]

Comment on attachment 331172 [details] [diff] [review]
1.9 branch patch (jsnum.cpp => jsnum.c

Approved for 1.9.0.2. Please land in CVS. a=ss

(After the test lands.)

Comment 17

[Samuel Sidler (old account; do not CC)]

Comment on attachment 331175 [details] [diff] [review]
1.8 branch patch

Approved for 1.8.1.17. Please land in CVS. a=ss

(After the test lands.)

Comment 18

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/ecma_3/Number/15.7.4.3-02.js,v  <--  15.7.4.3-02.js
initial revision: 1.1

mozilla-central: changeset:   16381:779beff502f0

Typically I don't check in tests for outstanding security bugs, but here it is.

Comment 19

[Brian Crowder]

Yeah, I confess I'm not sure I understand the procedure here, and why we went this route.  Fortunately, the test doesn't -look- like an obvious security bug (or so it seems to me), but it still seems odd that we're landing a test for it before the fix has shipped in all relevant browsers.

Comment 20

[Reed Loden [:reed]]

Maybe we need to set some type of policy for tests in relation to security fixes?

Comment 21

[Samuel Sidler (old account; do not CC)]

We have a policy and the policy is to land the test after fixed releases are out. This was my bad for not noticing the s-s of this bug and pushing to get the test landed.

Comment 22

[Brian Crowder]

> or
> (uintN)(c - '0') < 10

For what it's worth, gcc -does- do this optimization if -fif-conversion is enabled (which it is for -0 and up).

Comment 23

[Bob Clary [:bc] (inactive)]

verified 1.8.1, 1.9.0, 1.9.1 shell/browser linux

Comment 24

[Alexander Sack]

Attachment: for 1.8.0 (just context changes in backport)

a=asac for 1.8.0.15