Closed Bug 521306 Opened 10 years ago Closed 10 years ago

DoS in prdtoa after bug 516396 is fixed


(NSPR :: NSPR, defect, P2)



(status1.9.2 beta4-fixed, blocking1.9.1 .8+, status1.9.1 .8-fixed)

Tracking Status
status1.9.2 --- beta4-fixed
blocking1.9.1 --- .8+
status1.9.1 --- .8-fixed


(Reporter: dveditz, Assigned: wtc)


(Blocks 1 open bug)


(Keywords: perf, regression, testcase, Whiteboard: [sg:dos])


(4 files)

+++ 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.
Whatever fix we take here needs to go into the JS engine copy in bug 520629 (or vice versa)
Blocks: 520629
blocking1.9.1: .4+ → .5+
Flags: blocking1.9.2?
Flags: blocking1.9.0.16+
Whiteboard: [sg:dos]
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Group: core-security
Keywords: crashperf
Whiteboard: [sg:dos] → [sg:dos][needs patch]
A testcase would be nice. For that we might want to have a separate closed bug?
Isn't there a testcase in bug 516396?
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].

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.
Attachment #410427 - Flags: review?
Comment on attachment 410427 [details] [diff] [review]
Proposed patch (checked in)

Ted: Can you review this?
Attachment #410427 - Flags: review? → review?(ted.mielczarek)
blocking1.9.1: .6+ → .7+
Flags: blocking1.9.0.16+ → blocking1.9.0.17+
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 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.
Attachment #410427 - Flags: review?(ted.mielczarek) → review?(crowder)
Attachment #410427 - Flags: review?(crowder) → review+
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)
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.
Whiteboard: [sg:dos][needs patch] → [sg:dos]
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
Attachment #410427 - Attachment description: Proposed patch → Proposed patch (checked in)
Attachment #412354 - Flags: review?(reed)
Attachment #412354 - Flags: review?(reed) → review+
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 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
Attachment #412354 - Attachment description: Add a test case → Add a test case (checked in)
What's the way we get this into Firefox? A new tag?
I pushed the fix to mozilla-central in changeset 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.
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.8.3
Attachment #410427 - Flags: approval1.9.2?
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].)
Attachment #410427 - Flags: approval1.9.2? → approval1.9.2+
Comment on attachment 410427 [details] [diff] [review]
Proposed patch (checked in)

a192=beltzner for good measure, please also land the tests, and thanks!
Whiteboard: [sg:dos] → [sg:dos][can land]
I created the NSPR_4_8_3_BETA CVS tag and pushed it to
mozilla-1.9.2 in changeset 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?
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.
Attachment #417714 - Flags: approval1.9.2?
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.
Attachment #417714 - Flags: approval1.9.2? → approval1.9.2+
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:
Attachment #417714 - Attachment description: Update NSPR to NSPR_4_8_3_RTM in mozilla-1.9.2 → Update NSPR to NSPR_4_8_3_RTM in mozilla-1.9.2 (pushed to mozilla-1.9.2)
How do we get this fix on mozilla-1.9.1?
beltzner: I just need to push NSPR_4_8_3_RTM to mozilla-1.9.1.
Please approve this.
Attachment #423719 - Flags: approval1.9.1.8?
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 on attachment 423719 [details] [diff] [review]
Update NSPR to NSPR_4_8_3_RTM in mozilla-1.9.1

Approved for, a=dveditz for release-drivers
Attachment #423719 - Flags: approval1.9.1.8? → approval1.9.1.8+
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?)
Flags: blocking1.9.0.18+ → blocking1.9.0.19+
I pushed NSPR_4_8_3_RTM to mozilla-1.9.1 (attachment  423719 [details] [diff] [review])
in changeset 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.
The check-in has been caused a huge amount of Mochitest failures on mozilla-1.9.1:

Shall this patched get backed out?
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.
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 which shows those failing Mochitests.
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.
Henrik: we need an answer to comment 31 right away so we know if this is OK to take on the branches.
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?
Henrik: you have to use the "SVG test case for this NSPR bug",
created by me, in bug 516396.
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).
Flags: blocking1.9.0.19+ → blocking1.9.0.19?
Don't think this blocks.
Flags: blocking1.9.0.19? → blocking1.9.0.19-
(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.
- Before: Runs for a very long time (minutes), but doesn't crash.
- After: Runs for just a few (~2) seconds.
Whiteboard: [sg:dos][can land] → [sg:dos]
You need to log in before you can comment on or make changes to this bug.