521306 - DoS in prdtoa after bug 516396 is fixed

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P2)

Description

[Daniel Veditz [:dveditz]]

+++ 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]]

Whatever fix we take here needs to go into the JS engine copy in bug 520629 (or vice versa)

Comment 2

[Andreas Gal :gal]

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)]

Isn't there a testcase in bug 516396?

Comment 4

[Wan-Teh Chang]

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

Comment on attachment 410427 [details] [diff] [review]
Proposed patch (checked in)

Ted: Can you review this?

Comment 6

[Wan-Teh Chang]

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

[(not currently active) Ted Mielczarek]

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]

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]

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]

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]

Attachment: Add a test case (checked in)

Comment 12

[Reed Loden [:reed]]

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]

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]]

What's the way we get this into Firefox? A new tag?

Comment 15

[Wan-Teh Chang]

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)]

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]]

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]

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]

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

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]

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]]

How do we get this fix on mozilla-1.9.1?

Comment 23

[Wan-Teh Chang]

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

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]]

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]]

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]

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][⌚️UTC+2]]

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]

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][⌚️UTC+2]]

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]

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]]

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][⌚️UTC+2]]

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]

Henrik: you have to use the "SVG test case for this NSPR bug",
created by me, in bug 516396.

Comment 35

[Daniel Veditz [:dveditz]]

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]]

Don't think this blocks.

Comment 37

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

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

Henrik:
- Before: Runs for a very long time (minutes), but doesn't crash.
- After: Runs for just a few (~2) seconds.