Last Comment Bug 521306 - DoS in prdtoa after bug 516396 is fixed
: DoS in prdtoa after bug 516396 is fixed
Status: RESOLVED FIXED
[sg:dos]
: perf, regression, testcase
Product: NSPR
Classification: Components
Component: NSPR (show other bugs)
: 4.8.2
: All All
: P2 normal (vote)
: 4.8.3
Assigned To: Wan-Teh Chang
:
Mentors:
Depends on:
Blocks: 520629 CVE-2009-0689
  Show dependency treegraph
 
Reported: 2009-10-08 14:09 PDT by Daniel Veditz [:dveditz]
Modified: 2010-08-26 16:11 PDT (History)
27 users (show)
mbeltzner: blocking1.9.2+
mbeltzner: blocking1.9.0.19-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
beta4-fixed
.8+
.8-fixed


Attachments
Proposed patch (checked in) (690 bytes, patch)
2009-11-04 18:48 PST, Wan-Teh Chang
crowderbt: review+
mbeltzner: approval1.9.2+
Details | Diff | Review
Add a test case (checked in) (2.46 KB, patch)
2009-11-13 17:40 PST, Wan-Teh Chang
reed: review+
Details | Diff | Review
Update NSPR to NSPR_4_8_3_RTM in mozilla-1.9.2 (pushed to mozilla-1.9.2) (1.64 KB, patch)
2009-12-15 09:40 PST, Wan-Teh Chang
bugzilla: approval1.9.2+
Details | Diff | Review
Update NSPR to NSPR_4_8_3_RTM in mozilla-1.9.1 (10.02 KB, patch)
2010-01-26 18:19 PST, Wan-Teh Chang
dveditz: approval1.9.1.8+
Details | Diff | Review

Description Daniel Veditz [:dveditz] 2009-10-08 14:09:06 PDT
+++ This bug was initially created as a clone of Bug #516396 +++

In bug 516396 we fixed a potentially exploitable crash in prdtoa. But because it no longer crashes early the routine can churn on and on for ridiculously long strings. The initial patch in that bug tried to solve this but led to an SVG regression (bug 519839) so we limited that fix to just the vulnerability proper.

This bug is where we are going to resolve that potential DoS.
Comment 1 Daniel Veditz [:dveditz] 2009-10-08 14:11:02 PDT
Whatever fix we take here needs to go into the JS engine copy in bug 520629 (or vice versa)
Comment 2 Andreas Gal :gal 2009-11-04 15:56:55 PST
A testcase would be nice. For that we might want to have a separate closed bug?
Comment 3 Samuel Sidler (old account; do not CC) 2009-11-04 16:05:54 PST
Isn't there a testcase in bug 516396?
Comment 4 Wan-Teh Chang 2009-11-04 18:48:47 PST
Created attachment 410427 [details] [diff] [review]
Proposed patch (checked in)

PR_strtod should return 0 if the input floating point
number is too long. I use a hardcoded, arbitrary limit
of 64 * 1024 digits in this patch.

To avoid unnecessary complications, I am not setting
the error code to PR_INVALID_ARGUMENT_ERROR at the
ret0 label because I found that the strtod spec uses
"may" as opposed to "shall" regarding EINVAL:
  If no conversion could be performed, 0 shall be
  returned, and errno may be set to [EINVAL].

http://www.opengroup.org/onlinepubs/009695399/functions/strtod.html

I'd like someone who can dive into the code to review
this patch.  The reviewer doesn't need to be an NSPR
module owner or peer.  I sent this patch to the dtoa.c
author on 2009-10-17, hoping he could do the authoritative
review, but he hasn't replied yet.
Comment 5 Samuel Sidler (old account; do not CC) 2009-11-09 14:39:06 PST
Comment on attachment 410427 [details] [diff] [review]
Proposed patch (checked in)

Ted: Can you review this?
Comment 6 Wan-Teh Chang 2009-11-09 15:53:38 PST
I suggest that we ask crowder or mrbkap to review this patch.
The copies of dtoa.c in NSPR and JavaScript are very similar.
Although crowder and mrbkap are not NSPR module owner/peer,
they are qualified to review a patch for prdtoa.c.
Comment 7 Ted Mielczarek [:ted.mielczarek] 2009-11-10 03:43:48 PST
Comment on attachment 410427 [details] [diff] [review]
Proposed patch (checked in)

Yeah, that's probably a better idea, I've never looked at this code before. Forwarding to crowder.
Comment 8 Brian Crowder 2009-11-10 06:56:44 PST
Comment on attachment 410427 [details] [diff] [review]
Proposed patch (checked in)

This of course -looks- right, but I have not actually compiled and tested this myself.  Rubber-stamping on the condition that you have.  Incidentally, 64k is a lot of digits, even in the grand scheme of "engineering for the future" considerations.  Do we need this many?  What is the reasoning?  (sorry if I missed this in earlier comments in this bug)
Comment 9 Wan-Teh Chang 2009-11-10 08:58:37 PST
Brian: I chose the 64K limit purely based on measurements of
the running time of PR_strtod, rather than based on the math
of floating point numbers or the limits from some specifications.

My measurement results are in bug 516396 comment 41, reproduced
here for convenience:

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

The running time of PR_strtod seems to be proportional to
the square of the number of digits.  11.45 seconds is
definitely too long.  2.86 seconds seems acceptable, so I
chose the 64K limit.

If you know of any maximum lengths on floating point
numbers used in JavaScript, CSS, or SVG, please let me
know.  I asked this question before and was told that
JavaScript does not specify a maximum length.
Comment 10 Wan-Teh Chang 2009-11-13 17:05:19 PST
Comment on attachment 410427 [details] [diff] [review]
Proposed patch (checked in)

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

Checking in prdtoa.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prdtoa.c,v  <--  prdtoa.c
new revision: 4.13; previous revision: 4.12
done
Comment 11 Wan-Teh Chang 2009-11-13 17:40:01 PST
Created attachment 412354 [details] [diff] [review]
Add a test case (checked in)
Comment 12 Reed Loden [:reed] (use needinfo?) 2009-11-13 19:53:39 PST
Comment on attachment 412354 [details] [diff] [review]
Add a test case (checked in)

>+	fprintf(stderr,"Failed to convert numeric value %s\n",
>+		"0.1111111111111111...");
>+        failed_already = 1;

Nit: Fix the whitespace here (fprint() and failed_already should be on the same level).

Seems like a reasonable test. I'm not an NSPR peer, but r=me if you'll accept it.

I'll work on porting this all to the JS code soon, I hope.
Comment 13 Wan-Teh Chang 2009-11-14 12:01:19 PST
Comment on attachment 412354 [details] [diff] [review]
Add a test case (checked in)

The uneven indentation in the patch was caused by the use of
tabs in some but not all lines.  In the actual code, the
indentation was good.  But I changed the tabs to spaces anyway.

I checked in the patch on the NSPR trunk (NSPR 4.8.3).

Checking in dtoa.c;
/cvsroot/mozilla/nsprpub/pr/tests/dtoa.c,v  <--  dtoa.c
new revision: 1.9; previous revision: 1.8
done
Comment 14 Mike Beltzner [:beltzner, not reading bugmail] 2009-11-18 14:53:09 PST
What's the way we get this into Firefox? A new tag?
Comment 15 Wan-Teh Chang 2009-11-18 19:05:38 PST
I pushed the fix to mozilla-central in changeset 2396b886239d:
http://hg.mozilla.org/mozilla-central/rev/2396b886239d

When this has been baked in mozilla-central for a few days, I'll
create the NSPR_4_8_3_RTM tag for mozilla-1.9.2.  I don't think
this fix needs to go into mozilla-1.9.1 and mozilla-1.9.0.
Comment 16 Samuel Sidler (old account; do not CC) 2009-11-18 19:50:49 PST
I'm no sure we have a few days. Code freeze is set for tomorrow.

(You also don't need approval for your patch since this bug is a blocker and that means immediate approval on 1.9.2 [but not security branches].)
Comment 17 Mike Beltzner [:beltzner, not reading bugmail] 2009-11-18 22:19:00 PST
Comment on attachment 410427 [details] [diff] [review]
Proposed patch (checked in)

a192=beltzner for good measure, please also land the tests, and thanks!
Comment 18 Wan-Teh Chang 2009-11-19 10:21:06 PST
I created the NSPR_4_8_3_BETA CVS tag and pushed it to
mozilla-1.9.2 in changeset e71f5292f801:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e71f5292f801

This includes both the fix and the test for this bug.

I will create the NSPR_4_8_3_RTM tag and push it to mozilla-1.9.2
in a few days.  The only change will be removing "Beta" from the
NSPR version string.  How many days do I have to do that?
Comment 19 Wan-Teh Chang 2009-12-15 09:40:01 PST
Created attachment 417714 [details] [diff] [review]
Update NSPR to NSPR_4_8_3_RTM in mozilla-1.9.2 (pushed to mozilla-1.9.2)

I just created the NSPR_4_8_3_RTM CVS tag.  Please approve
this patch for mozilla-1.9.2 to remove the "Beta" in the
NSPR version string.  It also contains a fix (bug 533093)
for a Perl script that Firefox is not using.
Comment 20 Johnathan Nightingale [:johnath] 2009-12-17 13:35:04 PST
Comment on attachment 417714 [details] [diff] [review]
Update NSPR to NSPR_4_8_3_RTM in mozilla-1.9.2 (pushed to mozilla-1.9.2)

We're right in the endgame, but this is a label-only patch, plus a NPOTB change. You're at the mercy of the tree, now.
Comment 21 Wan-Teh Chang 2009-12-17 13:55:45 PST
Comment on attachment 417714 [details] [diff] [review]
Update NSPR to NSPR_4_8_3_RTM in mozilla-1.9.2 (pushed to mozilla-1.9.2)

I pushed this patch to mozilla-1.9.2 in changeset db04140c6afc:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/db04140c6afc
Comment 22 Mike Beltzner [:beltzner, not reading bugmail] 2010-01-26 10:25:45 PST
How do we get this fix on mozilla-1.9.1?
Comment 23 Wan-Teh Chang 2010-01-26 18:19:57 PST
Created attachment 423719 [details] [diff] [review]
Update NSPR to NSPR_4_8_3_RTM in mozilla-1.9.1

beltzner: I just need to push NSPR_4_8_3_RTM to mozilla-1.9.1.
Please approve this.
Comment 24 Wan-Teh Chang 2010-01-26 18:29:32 PST
Is mozilla-1.9.0 still being built with the Mac OS X 10.3 SDK
(for PowerPC support)?  If so, I'll need to create an NSPR
4.7.7 release for you because NSPR 4.8.x requires Mac OS X
10.4 or later.  Do you really need this fix for mozilla-1.9.0?
Comment 25 Daniel Veditz [:dveditz] 2010-02-01 10:22:19 PST
Comment on attachment 423719 [details] [diff] [review]
Update NSPR to NSPR_4_8_3_RTM in mozilla-1.9.1

Approved for 1.9.1.8, a=dveditz for release-drivers
Comment 26 Daniel Veditz [:dveditz] 2010-02-01 10:24:07 PST
Don't feel entirely comfortable upgrading Firefox 3.0.x from NSPR 4.7.6 to 4.8.3 right before final builds, we'll try to get this next time. (What OS compatibility issues might we hit this time? Why did we stop at 4.7.x last time we had to upgrade NSPR on the 1.9.0 branch?)
Comment 27 Wan-Teh Chang 2010-02-01 11:04:38 PST
I pushed NSPR_4_8_3_RTM to mozilla-1.9.1 (attachment  423719 [details] [diff] [review])
in changeset 5ecd30b72763:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5ecd30b72763

Dan: NSPR 4.8.x doesn't support Mac OS X 10.2 and 10.3.  If
Firefox 3.0.x supports Mac OS X 10.2 and 10.3, it's easier
(for you) to stay with NSPR 4.7.x.
Comment 28 Henrik Skupin (:whimboo) 2010-02-01 18:07:45 PST
The check-in has been caused a huge amount of Mochitest failures on mozilla-1.9.1: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5/1265052610.1265056489.21173.gz

Shall this patched get backed out?
Comment 29 Wan-Teh Chang 2010-02-01 18:22:05 PST
Henrik: Thank you for letting me know.  I don't know how
to run Mochitest, so I can't debug this.  In general I'm
happy to have a broken checkin backed out, but in this
case there are only two small changes in my checkin --
the patch in this bug, and the patch in bug 522992.  All
the other changes in this checkin are build changes.  So
it's strange that this checkin would cause Mochitest
failures.  Could you rerun the Mochitest?  Thanks.
Comment 30 Henrik Skupin (:whimboo) 2010-02-02 01:23:10 PST
I cannot run those Mochitests, sorry. I would prefer if someone else with more experience in reading and understanding those results could have a look at this. I have only checked http://tests.themasta.com/tinderboxpushlog/?tree=Firefox3.5 which shows those failing Mochitests.
Comment 31 Wan-Teh Chang 2010-02-02 10:27:58 PST
Henrik: the next two cycles on "OS X 10.5.2 mozilla-1.9.1 test mochitests"
are green.  Does that mean I'm off the hook?  It'd still be nice if someone
could look at the failing Mochitests on "OS X 10.5.2 mozilla-1.9.1 test
mochitests" after my checkin.
Comment 32 Mike Beltzner [:beltzner, not reading bugmail] 2010-03-03 22:26:35 PST
Henrik: we need an answer to comment 31 right away so we know if this is OK to take on the branches.
Comment 33 Henrik Skupin (:whimboo) 2010-03-04 07:55:13 PST
Given the greenish shimmering of the tinderboxpushlog I would tend to say we are fine, isn't it?

Wan-Teh, to verify this fix we can use the testcase on bug 516396? There should be no hang anymore now?
Comment 34 Wan-Teh Chang 2010-03-04 10:10:09 PST
Henrik: you have to use the "SVG test case for this NSPR bug",
created by me, in bug 516396.
Comment 35 Daniel Veditz [:dveditz] 2010-03-05 09:51:26 PST
I recommend not taking this on 1.9.0 -- I'd hate to upgrade NSPR, find out we broke some obscure edge case, and then have to ship a 3.0.20 (although we look OK on the supported platforms question).
Comment 36 Mike Beltzner [:beltzner, not reading bugmail] 2010-03-05 13:36:11 PST
Don't think this blocks.
Comment 37 Henrik Skupin (:whimboo) 2010-03-05 18:10:11 PST
(In reply to comment #34)
> Henrik: you have to use the "SVG test case for this NSPR bug",
> created by me, in bug 516396.

And what results I do have to expect? Running a Shiretoko build before your check-in doesn't crash.
Comment 38 Wan-Teh Chang 2010-03-05 18:11:49 PST
Henrik:
- Before: Runs for a very long time (minutes), but doesn't crash.
- After: Runs for just a few (~2) seconds.

Note You need to log in before you can comment on or make changes to this bug.