516396 - (CVE-2009-0689) Array indexing error in NSPR's Balloc() leads to floating point memory vulnerability (SA36711)

Status: VERIFIED FIXED

(NSPR :: NSPR, defect)

Description

[Reed Loden [:reed]]

From Secunia to security@:
---------------------------
Secunia Research has discovered a vulnerability in Mozilla Firefox,
which can be exploited by malicious people to compromise a user's
system.

The vulnerability is caused due to an array indexing error while
allocating space for floating point numbers. This can be exploited to
trigger a memory corruption via a specially crafted floating point
number.

Successful exploitation may allow execution of arbitrary code.

The vulnerability is confirmed in version 3.0.14 and 3.5.3. Other
versions may also be affected.

Vulnerability Details:
----------------------

The vulnerability is caused due to an error when converting strings to
floating point numbers in nsprpub/pr/src/misc/prdtoa.c.

The s2b() function takes the total number of digits and determines the
first number K for which : 1 << K >= (numdigits + 8)/9.

K is then passed to Balloc() to allocate memory. Balloc() dereferences
the static "freelist" buffer of 16 elements using K as an index. If K is
above 15, malformed pointers following the freelist array will be
returned from Balloc().

***
#define Kmax 15
...
static Bigint *freelist[Kmax+1];
...
Balloc ..(k)..
...
if (rv = freelist[k]) {	<-- out of bounds
    freelist[k] = rv->next;
}
...
return rv;
***

For e.g. K = 17, a pointer to a limited heap buffer is returned from
Balloc(), and used to hold the converted big number. This results in a
heap-based buffer overflow, followed by a call to a function grabbed
from a corrupted pointer to a virtual function table. This results in
the execution of an arbitrary address when paired with heap spraying.

Closing comments:
-----------------

We have assigned this vulnerability Secunia advisory SA36711 and CVE
identifier CVE-2009-1563.

Credits should go to:
Alin Rad Pop, Secunia Research.
------------------------

Comment 1

[Reed Loden [:reed]]

PoC has been requested from Secunia.

The vulnerability can also be reproduced by creating an HTML file
containing JavaScript code initializing a variable with 0.<1179649 '1'
digits> .

Comment 2

[Reed Loden [:reed]]

Attachment: Testcase for JavaScript based on comment #1

My simple testcase based on comment #1. Crashes most adequately.

Tested on Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20090912 Minefield/3.7a1pre.

bp-0d6034fa-6df3-43e2-8a5b-b7f802090914
bp-ec99a893-f615-4db5-bb71-ef0b62090914

Comment 3

[Reed Loden [:reed]]

Attachment: Secunia's PoC

Comment 4

[Wan-Teh Chang]

Thank you for this bug report.

dtoa.c is also used in Mozilla's, WebKit's, and Chromium's
JavaScript implementations, and in Ruby.  Has Secunia or
CVE notified the other users of dtoa.c?  We should also
notify dtoa.c's author, David M. Gay.

The obvious fix of comparing the index 'k' with Kmax in
Balloc is incomplete because we need a corresponding
change to Bfree:

Index: prdtoa.c
===================================================================
RCS file: /cvsroot/mozilla/nsprpub/pr/src/misc/prdtoa.c,v
retrieving revision 4.7
diff -u -u -r4.7 prdtoa.c
--- prdtoa.c	20 Mar 2009 03:41:21 -0000	4.7
+++ prdtoa.c	14 Sep 2009 17:46:14 -0000
@@ -581,7 +581,7 @@
 #endif
 
 	ACQUIRE_DTOA_LOCK(0);
-	if (rv = freelist[k]) {
+	if (k <= Kmax && rv = freelist[k]) {
 		freelist[k] = rv->next;
 		}
 	else {

It seems that we'll have to either require that k <= Kmax,
or to reallocate freelist when it's not big enough.

Comment 5

[Wan-Teh Chang]

After chatting with Chris Evans of the Google Security Team,
I found that this bug is already fixed in the dtoa.c upstream:
http://www.netlib.org/fp/dtoa.c

Since dtoa.c's author only publishes the latest revision of
dtoa.c, it's not easy to extract the fix for this bug, but
I believe the fix is entirely within the Balloc and Bfree
functions.  See also this OpenBSD patch for gdtoa, which is
the next generation of dtoa.c:
http://www.openbsd.org/cgi-bin/cvsweb/src/lib/libc/gdtoa/misc.c.diff?r1=1.1;r2=1.2;f=h

The code looks very similar to dtoa.c.

Comment 6

[Jeff Walden [:Waldo]]

(adding SpiderMonkey people since JS is affected)

Comment 7

[Andreas Gal :gal]

Webkit has a clean dtoa implementation. Its missing some piece we need, but otherwise we should be able to take it.

Comment 8

[Wan-Teh Chang]

Attachment: Patch from dtoa.c upstream

Note that the value of Kmax is also reduced from 15 to 7.
I don't know why.

I believe this entry in the "changes" file
(http://www.netlib.org/fp/changes) describes this
change:

Sun Mar  1 20:57:22 MST 2009
  dtoa.c and gdtoa/gdtoaimp.h and gdtoa/misc.c:  reduce Kmax, and use
MALLOC and FREE or free for huge blocks, which are possible only in
pathological cases, such as dtoa calls in mode 3 with thousands of
digits requested, or strtod() calls with thousand of digits.  For the
latter case, I have an alternate approach that runs much faster
and uses less memory, but finding time to get it ready for distribution
may take a while.

Comment 9

[Reed Loden [:reed]]

Comment on attachment 400623 [details] [diff] [review]
Patch from dtoa.c upstream

er, I clicked submit too early...

I'm not an NSPR peer, so tossing to ted for review. My initial look through the checks seems ok, though, and it's pretty similar to what gdtoa did to fix this problem.

However, I did have a random question... this new code adds the use of free(), which was not used directly before in prdtoa. Do we need to do anything special because of it (such as use PR_Free() instead or something like that)?

Comment 10

[Wan-Teh Chang]

Comment on attachment 400623 [details] [diff] [review]
Patch from dtoa.c upstream

The best person to review this patch is a JS developer
familiar with jsdtoa.cpp.  Any volunteer?  Is Brian
Crowder still working on Mozilla?

Reed, thanks for the review.  We don't need to do
anything special about the new FREE macro.  If FREE
is not defined, the code uses free(), which matches
malloc().  (The code defines MALLOC as malloc by
default.  It should also define FREE as free by
default.)

Comment 11

[Brian Crowder]

Comment on attachment 400623 [details] [diff] [review]
Patch from dtoa.c upstream

Does js/src/dtoa.c have the same bug?

Comment 12

[Wan-Teh Chang]

Yes, js/src/dtoa.c has the same bug.  We should open
a JS bug for that.

Comment 13

[Andreas Gal :gal]

Why was Kmax reduced to 7?

Comment 14

[Wan-Teh Chang]

It's explained in the "changes" file entry (see comment 8).
We don't want to add huge blocks to 'freelist'.  It's
possible that dtoa.c's author fixed this bug unknowingly
when he reduced Kmax to 7.

Comment 15

[Wan-Teh Chang]

Comment on attachment 400623 [details] [diff] [review]
Patch from dtoa.c upstream

I checked this in on the NSPR trunk (NSPR 4.8.1).

Checking in pr/src/misc/prdtoa.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prdtoa.c,v  <--  prdtoa.c
new revision: 4.8; previous revision: 4.7
done

Comment 16

[Reed Loden [:reed]]

(In reply to comment #15)
> (From update of attachment 400623 [details] [diff] [review])
> I checked this in on the NSPR trunk (NSPR 4.8.1).

We'll need to get NSPR tag(s) with the fix created for the various branches (trunk/1.9.2, 1.9.1, 1.9.0). Can you assist with that?

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 400623 [details] [diff] [review]
Patch from dtoa.c upstream

Approved for 1.9.1.4, a=dveditz

I guess for the branches using hg (trunk,1.9.2,1.9.1) you just check those in there, but for 1.9.0.x we'll need an updated NSPR tag and corresponding client.mk change.

Comment 18

[Benjamin Smedberg]

The hg branches should also use a tag: unless things have gone very strange we update hg to NSPR/NSS tags using client.py.

Comment 19

[Reed Loden [:reed]]

Comment on attachment 400623 [details] [diff] [review]
Patch from dtoa.c upstream

This is blocking1.9.2+, so approval1.9.2 not needed.

Comment 20

[Wan-Teh Chang]

I will work on a new NSPR release this weekend and next Monday.
Does any of the 1.9.0.15-based Mozilla products need to support
Windows 9x or Mac OS X 10.2 and 10.3?

Comment 21

[Daniel Veditz [:dveditz]]

The 1.9.0 branch already exclude those OS versions.
http://www.mozilla.com/en-US/firefox/system-requirements-v3.html

Comment 22

[Al Billings [:abillings - ex-MoCo]]

(In reply to comment #2)
> Created an attachment (id=400490) [details]
> Testcase based on comment #1
> 
> My simple testcase based on comment #1. Crashes most adequately.
> 
> Tested on Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20090912
> Minefield/3.7a1pre.

This does crash 1.9.1.3 cleanly on Linux. When I run this testcase on last night's 1.9.1.4pre build (Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.4pre) Gecko/20090921 Shiretoko/3.5.4pre), Firefox just hangs completely instead of crashing. Status1.9.1 says that this is fixed so this is either not fixed after all or the status was changed before the NSPR made it into the build so it isn't fixed yet.

Can someone comment?

Comment 23

[Daniel Veditz [:dveditz]]

The status of this bug has not been fixed on any Firefox build. The fix has been checked into the NSPR trunk (comment 15).

Comment 24

[Al Billings [:abillings - ex-MoCo]]

oops, I was off by one field. Thanks.

This is showing up in my "unverified for 1.9.1.4" query from the Tinderbox though for some reason.

It's strange that if it isn't fixed, it isn't crashing in the same way as 1.9.1.3 though.

Comment 25

[Wan-Teh Chang]

I pushed the fix to mozilla-central in changeset 7ec23b6b3611:
http://hg.mozilla.org/mozilla-central/rev/7ec23b6b3611

I pushed the fix to mozilla-1.9.2 in changeset fb2192ebeff0:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/fb2192ebeff0

Comment 26

[Wan-Teh Chang]

I pushed the fix as part of the NSPR_4_8_1_BETA1 CVS tag
to mozilla-1.9.1 in changeset ae5cf3fd2e69:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ae5cf3fd2e69

I will push the NSPR_4_8_1_RTM tag to mozilla-1.9.1 after
two days of testing.  Please let me know before you do your
first 1.9.1.4 candidate build.

Firefox 3 is using NSPR_4_7_5_RTM, so the upgrade to NSPR
4.8.1 will be a minor version upgrade.  But I'm confident
it'll be uneventful.

dveditz: Thunderbird 2 doesn't need this fix, right?

Comment 27

[Daniel Veditz [:dveditz]]

Thunderbird 2 probably can get away without it (and certainly isn't going to be happy with an upgrade to nspr 4.8.1) but SeaMonkey 1.1.x definitely needs it.

I think the testcases in this bug are testing bug 516862 -- the behavior on 1.9.0.15pre definitely changed even though this fix hasn't landed on the 1.9.0 branch yet (but that one has).  I think we'd need an SVG testcase to trigger the NSPR version of this function. I'm not sure the fix is correct, though, since it still crashes (differently): see bug 516862 comment 16

Comment 28

[Wan-Teh Chang]

Attachment: NSPR test case

Does SeaMonkey 1.1.x need to support Windows 9x or Mac OS X 10.2 and 10.3?

I am considering adding an input length check to PR_strtod to prevent it
from running for too long.  The NSPR test case in this patch takes a long
time to finish, so I can't check it in.

Comment 29

[Wan-Teh Chang]

Attachment: Add an input length check to PR_strtod

It's very difficult to check the string length as we parse
it, so I just added a string length check at the beginning
of the PR_strtod function.

If the string is too long, PR_strtod returns 0.0 and sets
the PR_INVALID_ARGUMENT_ERROR.  This is modeled after the
strtod function, which returns 0.0 and sets errno to EINVAL
when it cannot convert the string:
http://www.opengroup.org/onlinepubs/000095399/functions/strtod.html

Comment 30

[Wan-Teh Chang]

Attachment: NSPR test case (no unrelated change)

A debug build of this test took 12 minutes to finish on Linux:

wtc@aes$ time ./dtoa
PASSED

real	12m33.549s
user	12m32.939s
sys	0m0.044s

This verifies the crash fix works, but we really need the input length
check.

Comment 31

[Robert Kaiser]

(In reply to comment #28)
> Created an attachment (id=402497) [details]
> NSPR test case
> 
> Does SeaMonkey 1.1.x need to support Windows 9x or Mac OS X 10.2 and 10.3?

We currently have already broken Win9x support due to the NSS upgrade, but that might even be fixed. SeaMonkey 1.1.x has supported all those versions so far and it would be quite bad to break it within the series. We want to avoid that if in any way possible.

Comment 32

[Robert Kaiser]

(In reply to comment #31)
> We currently have already broken Win9x support due to the NSS upgrade

(Just to make it clear - not all of Win9x, only those installations that don't have IE4, i.e. esp. Win95 and NT 3.51)

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

Uh, wait, as I understand it, SM 1.1.x has the same platform support as FF2,
which is the set of platform supported for the 1.8 branch.  As I understand 
it, that set is shown on 
http://www.mozilla.com/en-US/firefox/system-requirements-v2.html
It does not include Win95 or NT 3.5, but it does include Win98 and NT 4.0.

Comment 34

[Robert Kaiser]

No, 1.8.1 branch itself and therefore also SeaMonkey 1.1.x actually do support Win95 (with DCOM for Win95) and NT 3.51, Firefox/Toolkit did desupport those there without the actual platform losing support for them, so we continued to include those system, see http://www.seamonkey-project.org/releases/seamonkey1.1.18/installation#win_requirements - and a number of users were happy about it.
There was no large outcry now that this NSS upgrade broke those systems where IE4 isn't installed, but we did get a number of people reporting it.

Of course, the SeaMonkey 2.0 coming next month on a 1.9.1 base desupports everything below Win2k, but it's a huge jump in every respect after all. It would be nice if we could support the old systems for a few months still (even though I don't dare to talk about the number of security patches we are already missing on 1.8.1 branch compared to 1.9.0 or 1.9.1 so far.

Comment 35

[Reed Loden [:reed]]

Reopening to make sure we get the new patch in.

Comment 36

[Reed Loden [:reed]]

(In reply to comment #29)
> Created an attachment (id=402499) [details]
> Add an input length check to PR_strtod

Who can review this? Ted?

Comment 37

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 402502 [details] [diff] [review]
NSPR test case (no unrelated change)

How about another NSPR module owner?

Comment 38

[Wan-Teh Chang]

Comment on attachment 402499 [details] [diff] [review]
Add an input length check to PR_strtod

Nelson, it's this patch that needs reviewing.  The 20480 upper bound
is just an arbitrary number I picked.  We should pick a number that allows
PR_strtod to finish in, say, 10 seconds.  Thanks!

Comment 39

[Daniel Veditz [:dveditz]]

The "reopen" should have applied to the branches, too. Clearing those fixed statuses until the additional patch lands (if I'm understanding this correctly).

Comment 40

[Wan-Teh Chang]

Comment on attachment 400623 [details] [diff] [review]
Patch from dtoa.c upstream

I verified with valgrind on Linux that this patch fixes
the "Invalid write of size 1" error in prdtoa.c, rev. 4.7.

wtc@aes:/home/wtc/nspr-tip/linux.dbg/pr/tests$ valgrind ./dtoa
==15994== Memcheck, a memory error detector
==15994== Copyright (C) 2002-2009, and GNU GPL'd, by Julian Seward et al.
==15994== Using Valgrind-3.6.0.SVN and LibVEX; rerun with -h for copyright info
==15994== Command: ./dtoa
==15994== 
==15994== Invalid write of size 1
==15994==    at 0x69019BB: memcpy /tmp/vg/memcheck/mc_replace_strmem.c:482
==15994==    by 0x691C8A7: multadd /home/wtc/nspr-tip/linux.dbg/pr/src/misc/../../../../mozilla/nsprpub/pr/src/misc/prdtoa.c:675
==15994==    by 0x691C97B: s2b /home/wtc/nspr-tip/linux.dbg/pr/src/misc/../../../../mozilla/nsprpub/pr/src/misc/prdtoa.c:712
==15994==    by 0x691E088: PR_strtod /home/wtc/nspr-tip/linux.dbg/pr/src/misc/../../../../mozilla/nsprpub/pr/src/misc/prdtoa.c:2027
==15994==    by 0x8048B80: main /home/wtc/nspr-tip/linux.dbg/pr/tests/../../../mozilla/nsprpub/pr/tests/dtoa.c:228
==15994==  Address 0x6950000 is not stack'd, malloc'd or (recently) free'd

The same "Invalid write of size 1" error is repeated for three other addresses:
...
==15994==  Address 0x6950001 is not stack'd, malloc'd or (recently) free'd
...
==15994==  Address 0x6950002 is not stack'd, malloc'd or (recently) free'd
...
==15994==  Address 0x6950003 is not stack'd, malloc'd or (recently) free'd

Comment 41

[Wan-Teh Chang]

Attachment: Add an input length check to PR_strtod v2

I increased the maximum length to 64K characters.

 32K:  0.72 seconds
 64K:  2.86 seconds
128K: 11.45 seconds

64K seems long enough.

Comment 42

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 402883 [details] [diff] [review]
Add an input length check to PR_strtod v2



>+	for(s = s00, i = 0; *s && i < 64 * 1024; s++, i++)
>+		;
>+	if (*s) {

I believe the above lines of code are equivalent to the following two lines:
 +#define MAX_STR_LEN 64 * 1024
 +      if (MAX_STR_LEN == strnlen(s00, MAX_STR_LEN)) {

and the strnlen library code might be more well optimized on some 
platforms than the compiler's code for that for loop. Since this entire
exercise is about bounding time spent, seems like we should optimize 
where we can.

>+		PR_SetError(PR_INVALID_ARGUMENT_ERROR, 0);
>+		return 0.0;

Which is better to return in this case?  0.0?  or NAN?  

r+=nelson, because I believe the patch is correct as is, but if you 
think any of these suggestions is an improvement, and want to make it,
please feel free to make another.

Comment 43

[Wan-Teh Chang]

Comment on attachment 402883 [details] [diff] [review]
Add an input length check to PR_strtod v2

Nelson, thanks for the review comments.

I considered strnlen but am not sure if it's
available on all the platforms.  Maybe we can
add a configure test for it.

The 0.0 return value and PR_INVALID_ARGUMENT_ERROR
are modeled after strtod().

I checked in this patch on the NSPR trunk (NSPR 4.8.1).

Checking in prdtoa.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prdtoa.c,v  <--  prdtoa.c
new revision: 4.10; previous revision: 4.9
done

Comment 44

[Wan-Teh Chang]

Comment on attachment 402883 [details] [diff] [review]
Add an input length check to PR_strtod v2

See Nelson's r+=nelson in comment 42.

Comment 45

[Wan-Teh Chang]

Comment on attachment 402883 [details] [diff] [review]
Add an input length check to PR_strtod v2

I pushed this patch to mozilla-central in changeset ed8abff562ad:
http://hg.mozilla.org/mozilla-central/rev/ed8abff562ad

I pushed this patch to mozilla-1.9.2 in changeset 7a6b4aea8917:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7a6b4aea8917

Comment 46

[Daniel Veditz [:dveditz]]

Comment on attachment 402883 [details] [diff] [review]
Add an input length check to PR_strtod v2

Approved for 1.9.1.4, a=dveditz for release-drivers

Comment 47

[Wan-Teh Chang]

Comment on attachment 402883 [details] [diff] [review]
Add an input length check to PR_strtod v2

I pushed this patch to mozilla-1.9.1 in changeset 491f6fe5c98d:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/491f6fe5c98d

Comment 48

[Al Billings [:abillings - ex-MoCo]]

Running Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.1.4pre) Gecko/20090929 Shiretoko/3.5.4pre, the Secunia PoC attached here and the testcase in comment 2 still hang Firefox (but no outright crashes).

Comment 49

[Wan-Teh Chang]

Comment on attachment 400490 [details]
Testcase for JavaScript based on comment #1

Al: this test case is actually for JavaScript (bug 516862).
This test case doesn't test NSPR's PR_strtod function.
We should be able to construct an NSPR test
case using SVG.

Comment 50

[Jeff Muizelaar [:jrmuizel]]

This change seems to have caused a 0.82%-1.46% regression in our tSVG results. Is that expected and is there anything we can do to avoid that?

Comment 51

[Boris Zbarsky [:bzbarsky]]

Did we ever file a bug on jsdtoa here?

Comment 52

[Boris Zbarsky [:bzbarsky]]

As for comment 50, prdtoa is a huge performance bottleneck.  We simply stopped using it in the CSS parser (since we don't in fact need most of the things it does for us)...  I don't know whether that's reasonable in the SVG case; if not the only way I see to fix the performance issue is to do the length check during conversion.

Comment 53

[Boris Zbarsky [:bzbarsky]]

To be more precise, in the css parser we used to both call dtoa and ToInteger; we switched to producing both results at once; with some loss of precision on the dtoa that's not noticeable in practice due to how css stores its floats.

Comment 54

[Reed Loden [:reed]]

(In reply to comment #51)
> Did we ever file a bug on jsdtoa here?

Yes, bug 516862.

Comment 55

[Wan-Teh Chang]

Thank you for the report of performance regression in SVG.
The performance degradation is expected.  I made a judgment
call to favor developer productivity (including time
required for another developer to review the length check)
over best possible performance.  If you can devote more
time than I can and can line up a competent code reviewer,
please do the length check during conversion.

The need for the length check is clear if you click Reed's
JavaScript test case in comment 2 and watch the browser
consume 100% CPU for a long time.  It shouldn't be hard to
construct a SVG test case.

Comment 56

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #55)
> Thank you for the report of performance regression in SVG.
> The performance degradation is expected.  I made a judgment
> call to favor developer productivity (including time
> required for another developer to review the length check)
> over best possible performance.  If you can devote more
> time than I can and can line up a competent code reviewer,
> please do the length check during conversion.
> 
> The need for the length check is clear if you click Reed's
> JavaScript test case in comment 2 and watch the browser
> consume 100% CPU for a long time.  It shouldn't be hard to
> construct a SVG test case.

This makes sense to me and I agree that it's probably not worth worrying about. I've opened a bug 519794 about adding a comment to the svg code about the performance of PR_strtod. If the problem becomes important we can deal with it then.

Comment 57

[Wan-Teh Chang]

Attachment: Do the length check during conversion

It's not as hard as I thought, if I dive into the code.
You can consider using this patch for JavaScript.

This patch does the length check after PR_strtod has
counted the number of digits.  Since the various counters
nd, nd0, nf, nz, nz0 have the type 'int', in 64-bit
environment these counters can overflow if the input
string is longer than 2G characters.  I didn't bother
to fix this integer overflow issue in this patch.
The fix is probably to declare these counters as
size_t and eliminate any compiler warnings by
typecasts.

Comment 58

[Wan-Teh Chang]

Comment on attachment 400623 [details] [diff] [review]
Patch from dtoa.c upstream

I checked in this patch on the NSPR_4_7_BRANCH for NSPR 4.7.6.

Checking in prdtoa.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prdtoa.c,v  <--  prdtoa.c
new revision: 4.5.36.2; previous revision: 4.5.36.1
done

Comment 59

[Jonathan Watt [:jwatt]]

This regressed SVG on 1.9.2 and trunk: bug 519839.

Comment 60

[Jonathan Watt [:jwatt]]

(In reply to comment #57)
> Created an attachment (id=403970) [details]
> Do the length check during conversion

If you land on 1.9.0, can you be sure to take this too to fix the SVG bustage?

Comment 61

[Reed Loden [:reed]]

Reopening to make sure we get the new patch on trunk and all the branches.

Comment 62

[Wan-Teh Chang]

Comment on attachment 403970 [details] [diff] [review]
Do the length check during conversion

Who can review this patch?  You need to dive into the first
part of the PR_strtod function.  The 0 return value and the
PR_INVALID_ARGUMENT_ERROR error code (equivalent to EINVAL)
when no conversion could be performed come from the strtod
man page in the Single Unix Specification:
http://www.opengroup.org/onlinepubs/9699919799/functions/strtod.html

I checked in this patch on the NSPR trunk (NSPR 4.8.2).

Checking in prdtoa.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prdtoa.c,v  <--  prdtoa.c
new revision: 4.11; previous revision: 4.10
done

Comment 63

[Reed Loden [:reed]]

Comment on attachment 403970 [details] [diff] [review]
Do the length check during conversion

Try Nelson again?

Comment 64

[Daniel Veditz [:dveditz]]

(In reply to comment #61)
> Reopening to make sure we get the new patch on trunk and all the branches.

Why? This bug/symptom is fixed, reopening implies they aren't. We're taking additional patches to fix regressions but that's what the regression bug is for.

Comment 65

[Nelson Bolyard (seldom reads bugmail)]

I haven't followed this entire bug in great detail before now.
Was the input length check that was added in attachment 402883 [details] [diff] [review] necessary to 
eliminate the vulnerability?  Or was it merely a performance enhancement, 
bounding the time spent in this function on absurdly long strings?

I'm not sure exactly what the objectives of the patch in attachment 403970 [details] [diff] [review]
are, so it is difficult for me to judge if it accomplishes those or not.
So, instead of giving r+ or r-, I will make some review observations and let
Wan-Teh or others determine if these should be considered r- or not.

The patch in attachment 402883 [details] [diff] [review] absolutely limited the input string length
to an upper bound of a certain size in all cases.  The patch in attachment
403970 [review] does not.  Leading zero characters, before the decimal point (if any)
are not counted against that limit.  That's probably OK.

Likewise, the number of zero characters immediately following the decimal
point are not counted against that total, unless they are followed by a 
non-zero digit.  That's probably OK too.

I think I could construct some strings that would be detected by the
patch in attachment 402883 [details] [diff] [review], but not by the patch in attachment 403970 [details] [diff] [review], and
would waste oceans of CPU time, and maybe produce incorrect results.  Would 
the existence of such a string mean that this patch is not accomplishing its 
objective?

Consider an input string consisting of a '1' following by hundreds of 
thousands, or millions of digits, followed by a decimal point.  Or alternatively, a decimal point, followed by hundreds of thousands, or 
millions of zero digits, followed by a non-zero decimal digit. 
Eventually, the huge number of digits would be detected, but not before 
hundreds of thousands, or millions, of integer multiplies (by 10) had been 
done, wasting LOTS of CPU time.  Seems like, at the very least, the code
should detect overflow of the accumulators in those multiply-by-10-and-add 
loops, but it doesn't.  But that's probably a separate pre-existing bug.

Comment 66

[Wan-Teh Chang]

Attachment: Do the length check during conversion v2

This patch is equivalent to the previous patch but is more
readable.  'nd' won't change after the dig_done label, so it's
better to check 'nd' as soon as we reach that stage.

dveditz: Can we just fix the vulnerability for now?  The input
length check requires a lot of time from two developers (one
to write the patch, the other to review it).  I can't spend
too much time on this bug.

Nelson, the input length check is not necessary to eliminate the
vulnerability.  It merely bounds the time spent in PR_strtod on
absurdly long strings.  However, without the input length check,
it's hard for QA to verify the vulnerability fix because the
application consumes 100% CPU for a very long time (at least two
minutes) on Reed's test case for JavaScript.

The original input length check is incorrect because it applies
to the entire input string, but we should only check the initial
part of the input string that is a number.  For example, an
absurdly long string of the form
  "0.0not a number not a number not a number ..."
should be accepted by PR_strtod because only the "0.0" part of
the string needs to be converted.  The original input length
check causes this valid input to fail.

During the initial parsing phase, up to the dig_done label,
PR_strtod never multiplies by 10 for more than roughly
DBL_DIG + 1 times.

Comment 67

[Nelson Bolyard (seldom reads bugmail)]

BTW, FWIW, after reviewing the portion of the code that Wan-Teh patched,
I'm not sure that the function PR_strtod will produce the correct result
for a number with more than about 20 digits, e.g. a number like 10^22 
when represented without an E+ notation, e.g. 10000000000000000000000.
which is too big to be represented in a 64-bit integer. 

I believe that PR_strtod _should_ be able to produce the same answer for that 
value as for 1E+22, an answer that is correct to the limits of the precision
of a double, but I suspect it doesn't.  I should write a little test program.  
But this is a separate issue.

Comment 68

[Daniel Veditz [:dveditz]]

(In reply to comment #66)
> dveditz: Can we just fix the vulnerability for now?  The input
> length check requires a lot of time from two developers (one
> to write the patch, the other to review it).  I can't spend
> too much time on this bug.

We agree. How do we get to a state where the crash/security-bug is fixed, but we have to live with a hang on stupidly-long strings? I think that means we need to back-out the checkins in comment 45 and comment 47, and create a new 4.7.6 BETA tag that Firefox 3.0 can use.

Comment 69

[Robert Longson [:longsonr]]

Attachment: svg reftest

reftest for what's needed to keep SVG working.

Comment 70

[Wan-Teh Chang]

Attachment: Back out the input length check

I backed out the input length check on the NSPR trunk (NSPR 4.8.2)
and NSPR_4_7_BRANCH (NSPR 4.7.6).

Checking in prdtoa.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prdtoa.c,v  <--  prdtoa.c
new revision: 4.12; previous revision: 4.11
done

Checking in prdtoa.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prdtoa.c,v  <--  prdtoa.c
new revision: 4.5.36.4; previous revision: 4.5.36.3
done

Comment 71

[Wan-Teh Chang]

The new NSPR_4_8_2_RTM and NSPR_4_7_6_BETA2 tags have the vulnerability
fix but not the input length check.

I pushed NSPR_4_8_2_RTM to mozilla-central in changeset f6b3d9aaaa7f:
http://hg.mozilla.org/mozilla-central/rev/f6b3d9aaaa7f

I pushed NSPR_4_8_2_RTM to mozilla-1.9.2 in changeset 449846a09609:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/449846a09609

I pushed NSPR_4_8_2_RTM to mozilla-1.9.1 in changeset d9554296d4e2:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d9554296d4e2

Comment 72

[Daniel Veditz [:dveditz]]

Comment on attachment 400623 [details] [diff] [review]
Patch from dtoa.c upstream

Approved for 1.9.0.15, a=dveditz

Comment 73

[Daniel Veditz [:dveditz]]

Attachment: update client.mk for 1.9.0

This tag update, then, should be all that remains to fix this on the 1.9.0 branch for Firefox 3.0

Comment 74

[Wan-Teh Chang]

Comment on attachment 400623 [details] [diff] [review]
Patch from dtoa.c upstream

I checked this in (via the NSPR_4_7_6_BETA2 tag) on the Mozilla
1.9.0 branch.

Checking in client.mk;
/cvsroot/mozilla/client.mk,v  <--  client.mk
new revision: 1.395; previous revision: 1.394
done

Comment 75

[Wan-Teh Chang]

Comment on attachment 404742 [details] [diff] [review]
update client.mk for 1.9.0

r=wtc.  I already committed this (see comment 74).

Comment 76

[Wan-Teh Chang]

(In reply to comment #67)
Nelson, PR_strtod always protects multiplications of 'y' and 'z' by 10
with nd < xxx checks:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/src/misc/prdtoa.c&rev=4.11&mark=1675,1677#1672

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/src/misc/prdtoa.c&rev=4.11&mark=1718,1720,1722,1724#1715

I verified that PR_strtod converts "10000000000000000000000"
to 1e22.

Comment 77

[Wan-Teh Chang]

Comment on attachment 404742 [details] [diff] [review]
update client.mk for 1.9.0

dveditz: Do the 1.9.0 builds look good?  If you don't
know of any problem introduced by NSPR_4_6_7_BETA2, I
will create the NSPR_4_7_6_RTM tag today.

Comment 78

[Wan-Teh Chang]

Comment on attachment 404742 [details] [diff] [review]
update client.mk for 1.9.0

I updated the NSPR tag to NSPR_4_7_6_RTM for Mozilla 1.9.0.

Checking in client.mk;
/cvsroot/mozilla/client.mk,v  <--  client.mk
new revision: 1.396; previous revision: 1.395
done

Comment 79

[Henrik Skupin [:whimboo][⌚️UTC+2]]

As spoken on IRC with dveditz the hang is expected for now due to the regression (bug 519839) started with the fix for the hang. So with the following builds no crash happens anymore. Marking verified fixed on 1.9.1, 1.9.2, and trunk.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20091007 Minefield/3.7a1pre ID:20091007093342

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b1pre) Gecko/20091007 Namoroka/3.6b1pre ID:20091007034618

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.4) Gecko/20091006 Firefox/3.5.4 ID:20091006224018

Comment 80

[Wan-Teh Chang]

Al, Henrik: We don't have an SVG test case that makes Firefox
crash in prdtoa.c.  Reed's test case exercises JavaScript's copy
of dtoa.c.

Reed's test case has an input string of 1024 * 1024 characters.
A shorter test case of 384 * 1024 characters should still crash
JavaScript, and will take only 2 minutes to complete with the
crash fix.  It'll allow QA to verify the crash fix.

Comment 81

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thanks Wan-Teh but I was already able to verify the fix even when I had to wait around 5 minutes for each test. Oh, and I missed to say that I have also run tests on Windows and Linux.

Comment 82

[Al Billings [:abillings - ex-MoCo]]

I did the same thing as Henrik with 1.9.0 (Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.15) Gecko/2009101601 Firefox/3.0.15 (.NET CLR 3.5.30729)) and verified the lack of a crash, even though it takes forever.

Comment 83

[Wan-Teh Chang]

Attachment: SVG test case for this NSPR bug

Al, Henrik: there were no browser test cases for this NSPR bug.
If you used the first attachment in this bug, you verified
the JavaScript bug.

Please use this SVG test case to verify this NSPR bug.

Comment 84

[Al Billings [:abillings - ex-MoCo]]

That was what I was afraid of. So I verified the *other* bug. Great.

Verified for 1.9.0.15 using the new testcase from comment 83. Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.15) Gecko/2009101601 Firefox/3.0.15 (.NET CLR 3.5.30729)

Comment 85

[Henrik Skupin [:whimboo][⌚️UTC+2]]

No crash with the newly attached SVG testcase with builds on OS X and Windows:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.4) Gecko/20091016 Firefox/3.5.4 ID:20091016081620

Can we make sure to get those tests checked-in once this bug gets opened to the public? Sadly we don't have a in-testsuite flag for NSPR.

Comment 86

[Reed Loden [:reed]]

Maksymilian Arciemowicz just informed security@m.o that this bug seems to be similar to -- if not the same as -- CVE-2009-0689 (http://securityreason.com/achievement_securityalert/63). He reported the issue in June to NetBSD, FreeBSD, and to Google Chrome (as per http://googlechromereleases.blogspot.com/2009/09/stable-channel-update_30.html).

A slightly different fix was used in at least NetBSD's gdtoa implementation:
http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libc/gdtoa/gdtoaimp.h.diff?r1=1.6&r2=1.7

Are we sure this bug is completely (and correctly) fixed?

Comment 87

[Wan-Teh Chang]

This is the current version of dtoa.c in the V8 JavaScript engine
used by Google Chrome:
http://code.google.com/p/v8/source/browse/trunk/src/third_party/dtoa/dtoa.c

The fix is the same as ours except for the reduction of Kmax from
15 to 7:
http://code.google.com/p/v8/source/diff?spec=svn3178&r=2891&format=side&path=/trunk/src/third_party/dtoa/dtoa.c&old_path=/trunk/src/third_party/dtoa/dtoa.c&old=2739

Comment 88

[Wan-Teh Chang]

Changed the target milestone from 4.8.1 to 4.8.2.  The fix for
this bug in NSPR 4.8.1 was incorrect.

Comment 89

[Daniel Veditz [:dveditz]]

Updating CVE number per Mitre, coalescing -1563 into the older -0689

Comment 91

[Daniel Veditz [:dveditz]]

wtc said the 1.8.1 branch can be updated to NSPR 4.7.6, that it both contains the fix and corrects the bustage on older Mac platforms that kept us from using 4.7.5 on that branch. It'll be important to test on the oldest supported versions of Win and Mac to verify we didn't break things. The fix itself doesn't need much more than simple verification as 4.7.6 is the version we're currently using on the 1.9.0 branch

Comment 92

[Wan-Teh Chang]

(In reply to comment #91)
See bug 509262 comment 1 for the changes I made in NSPR 4.7.6
to restore support for Mac OS X 10.2 and 10.3.  Please focus
your testing on two things on Mac OS X:
1. No build errors.
2. Test a feature that uses the PR_GetPhysicalMemorySize function.
Alternatively, attach a debugger and verify PR_GetPhysicalMemorySize
returns the right value.

You don't need to do extra testing on Windows.

Comment 93

[Al Billings [:abillings - ex-MoCo]]

MoCo QA has a single OS X 10.3 machine available to test on. We don't have 10.2.

As far as testing a feature that uses the PR_GetPhysicalMemorySize function, can someone suggest one for Thunderbird or Seamonkey 1.1?

Comment 94

[Al Billings [:abillings - ex-MoCo]]

After doing a few rounds with IT, QA has no way to test on OS X 10.2. We have a 10.3 machine to test on but no access to 10.2 media to even install it.

Comment 95

[Wan-Teh Chang]

Al: testing on just Mac OS X 10.3 is fine.  Don't go out of your way
to install Mac OS X 10.2.

Comment 96

[Al Billings [:abillings - ex-MoCo]]

This broke, at the very least, OS X 10.5 and 10.6. Thunderbird 2 will just hang, before even getting to profile manager, when invoked.

Here is a sample from the latest nightly with the problem: http://ss.pastebin.mozilla.org/704905

Last good build is Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.24pre) Gecko/2010022203 Thunderbird/2.0.0.24pre

The only checkin is this bug. 

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2010-02-22&maxdate=2010-02-23&cvsroot=/cvsroot

I'm going to find out if 10.4 is broken. We've been waiting on an IT fix happening right now to check 10.2 and 10.3 but that should be resolved anytime.

Comment 97

[Al Billings [:abillings - ex-MoCo]]

Nick Thomas has 10.4 and commented in IRC that it is broken there as well.

Comment 98

[Al Billings [:abillings - ex-MoCo]]

<humor> Fortunately for us all, 10.2 and 10.3 work great! I just downloaded my mail on 10.2 from a test account! </humor>

Comment 99

[Al Billings [:abillings - ex-MoCo]]

The 10.2 and 10.3 tests were on a PPC box. I checked 10.4 on PPC (the highest we have there) and it works there as well. It looks like Intel is what is busted.

I am told that for PPC, we build with the 10.2.8 SDK and for Intel I think we use the 10.4u SDK.

Comment 100

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to comment #99)
> I am told that for PPC, we build with the 10.2.8 SDK and for Intel I think we
> use the 10.4u SDK.

That's right
 http://mxr.mozilla.org/mozilla1.8/source/build/macosx/universal/mozconfig#49
That file is sourced at the top of the mozconfig for the build.

Comment 101

[Daniel Veditz [:dveditz]]

Dunno if this is related, but when we landed NSPR 4.7.3 on the 1.9.0 branch (cvs head) we had to back it out because it caused bug 466531 (crashes on PPC). Relanding 4.7.3 involved patching mozilla/js/src/jscpucfg.cpp, and the 1.8.1 branch does not have those fixes.

Since PPC is working fine now maybe that's completely unrelated.

Comment 102

[Wan-Teh Chang]

Dan: good memory!

Al: What's the "uname -a" output of the Mac build machine?
If you cross-compile or do a universal build, you should
backport my JS patch from bug 466531.  Use attachment 353918 [details] [diff] [review].

Comment 103

[Al Billings [:abillings - ex-MoCo]]

Wan-Teh, I think you want to ask Nick this. I'm QA so I don't work with the Mac build machine and I don't have 1.8.1 setup to build on any box that I own.

Comment 104

[Nick Thomas [:nthomas] (UTC+12)]

The box is 
  Darwin bm-xserve05 8.7.0 Darwin Kernel Version 8.7.0: Fri May 26 15:20:53 PDT 2006; root:xnu-792.6.76.obj~1/RELEASE_PPC Power Macintosh powerpc
so we're building natively on PPC, then cross-compile for Intel, then create a Universal build from those.

Dan, could you look at the suggested attachment ?

Comment 105

[Wan-Teh Chang]

Nick: Thanks.  Then the Intel bits cross-compiled on that PPC box are
incorrect in JavaScript.  Could you or Dan backport the JS changes in
attachment 353918 [details] [diff] [review] to the branch?  Thanks.

Comment 106

[Daniel Veditz [:dveditz]]

Attachment: jscpucfg fix from bug 466531 for the 1.8 branch

The patch applied cleanly, shifted a bit (minus the client.mk part, of course). I've attached a merged version here because I can't check into mozilla/js/src -- it's locked down.

Comment 107

[Daniel Veditz [:dveditz]]

Comment on attachment 429065 [details] [diff] [review]
jscpucfg fix from bug 466531 for the 1.8 branch

Approved for 1.8.1.24, a=dveditz

Comment 108

[Daniel Veditz [:dveditz]]

Comment on attachment 429065 [details] [diff] [review]
jscpucfg fix from bug 466531 for the 1.8 branch

This additional fix checked in to the 1.8.1 branch

Comment 109

[Al Billings [:abillings - ex-MoCo]]

(In reply to comment #2)
> Created an attachment (id=400490) [details]
> Testcase based on comment #1
> 
> My simple testcase based on comment #1. Crashes most adequately.
> 
> Tested on Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20090912
> Minefield/3.7a1pre.
> 
> bp-0d6034fa-6df3-43e2-8a5b-b7f802090914
> bp-ec99a893-f615-4db5-bb71-ef0b62090914

I note that this testcase pegs my CPU on Seamonkey 1.1's nightly and on my current Firefox 3.7 nightly both.

Comment 110

[Al Billings [:abillings - ex-MoCo]]

The bustage for Thunderbird from previous checkins is now fixed and TB is verified to run on OS X 10.5 and 10.6 intel and on 10.4 and 10.3 PPC.

Re-reading all of the old comments, including my own, I realize that the case in comment 1 is for the JS version of this, not the SVG version, per Wan-Teh. I've already verified that SVG is fixed in bug 519839. The JS version was bug 521306. Do we need to fix this for 1.8.1.24?

Comment 111

[Daniel Veditz [:dveditz]]

The js fix is bug 516882, and we've taken that in 1.8.1.24; bug 521306 is about the fact that now that we've prevented the crash we realize the routine can get really really slow on huge numbers. We're not going to hold 1.8.1.24 for that DoS fix.

Comment 112

[Daniel Veditz [:dveditz]]

> The js fix is bug 516882

I meant bug 516862, of course (see comment 27)