584252 - non-canonical qnan coming out of strtod

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Curiously enough, strtod accepts "NAN(xxx)" (and is standardized to do so http://www.opengroup.org/onlinepubs/000095399/functions/strtod.html).  This means anywhere we call strtod, we accept this syntax as well.  More problematically, NAN(xxx) creates a non-canonical qnan with xxx as the payload.  This means you can make a double that gets interpreted as some other type and crashes:

  -parseFloat("NAN(fffffffffffff)")

I think the fix is to define No_Hex_Nan in dtoa.c, which turns off this "feature".

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

This affects WebKit too (crashes Safari 5, Epiphany, Android 2.1) with a slightly different string - 'ffffeeeeeff0f'.

Luke thinks this is the security bug Mike Pall vaguely hinted at on Reddit, based on comments in the LuaJIT 2.0 source code.

Comment 2

[Reed Loden [:reed]]

(In reply to comment #1)
> Luke thinks this is the security bug Mike Pall vaguely hinted at on Reddit,
> based on comments in the LuaJIT 2.0 source code.

http://www.backtype.com/url/www.reddit.com%252fuser%252fmikemike/comment/00006bbad0001f33b79896da23a0d12d

"But they forgot about a few critical details. Unfortunately the Mozilla bug bounty does not apply to development versions. So I guess I have to wait until 4.0 is released before reporting the bugs (verified crash with FF/x86 and FF/x64 for hg 898ab54a0ce9, remote exploitable)."

Comment 3

[Reed Loden [:reed]]

Note we have at least two copies of dtoa in the tree, so need to make sure both are patched.

Why are we filing rdar:// bugs for webkit issues? Shouldn't they be filed as security bugs on bugs.webkit.org?

Comment 4

[Reed Loden [:reed]]

While our bug bounty FAQ does say we only give bounties for vulnerabilities in currently released versions of Firefox, Firefox Mobile, and Thunderbird, we have been known to offer the bounties for bugs found only on trunk, so what he says isn't completely true...

Comment 5

[Robert Sayre]

We should not land a fix for this problem until September 1st, because it could make Android, Safari, and other WebKit users unsafe.

Comment 6

[Robert Sayre]

reason: We will be shipping a beta with our new jsval around Sep 1, and do not want this bug in that beta. I can bury the fix in an hg merge, though.

Comment 7

[Robert Sayre]

(In reply to comment #3)
> 
> Why are we filing rdar:// bugs for webkit issues? Shouldn't they be filed as
> security bugs on bugs.webkit.org?

Apple filed the rdar bug. We got in touch over email. Please do not make any further procedural comments in this bug.

Comment 8

[Oliver Hunt]

Also tracked by http://webkit.org/b/43454

Comment 9

[Reed Loden [:reed]]

(In reply to comment #3)
> Note we have at least two copies of dtoa in the tree, so need to make sure both
> are patched.

For the record, they are:
* ipc/chromium/src/base/third_party/dmg_fp/dtoa.cc
* js/src/dtoa.c
* nsprpub/pr/src/misc/dtoa.c
* nsprpub/pr/src/misc/prdtoa.c

Comment 10

[Luke Wagner [:luke]]

Attachment: patch and test

Instead of defining No_Hex_Nan, the patch just removes the code.

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #9)
> (In reply to comment #3)
> > Note we have at least two copies of dtoa in the tree, so need to make sure both
> > are patched.
> 
> For the record, they are:
> * ipc/chromium/src/base/third_party/dmg_fp/dtoa.cc
> * js/src/dtoa.c
> * nsprpub/pr/src/misc/dtoa.c
> * nsprpub/pr/src/misc/prdtoa.c

Wouldn't two copies be enough? Usually is.

/be

Comment 12

[Luke Wagner [:luke]]

(In reply to comment #9)
Bug 584168 is going to canonicalize nans at API boundaries, so, with respect to this bug, there is no problem with these other strtods.

Comment 13

[Wan-Teh Chang]

(In reply to comment #9)
> * nsprpub/pr/src/misc/dtoa.c

This copy is not being used.  I added it to
archive the pristine upstream dtoa.c source.

Luke: NSPR's prdtoa.c doesn't define INFNAN_CHECK:
http://mxr.mozilla.org/nspr/ident?i=INFNAN_CHECK

Does this mean prdtoa.c is not vulnerable to this bug?

Why don't you just define the No_Hex_NaN macro for
js/src/dtoa.c?  Removing the #ifndef No_Hex_NaN code
can make it harder to merge with new versions of
dtoa.c.

Comment 14

[David Anderson [:dvander] - inactive, e-mail if emergency]

This code is so dangerous to our encoding that it seems better to remove it entirely than risk the define accidentally disappearing.

Comment 15

[Robert Sayre]

(In reply to comment #6)
> reason: We will be shipping a beta with our new jsval around Sep 1, and do not
> want this bug in that beta.

uh, so this comment is totally wrong. we are going to ship a beta with our new jsvals very soon. we will discuss further tomorrow.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Who's on reviewing this patch?

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Thanks for the review.

Talked with sayrer about this on IRC. We know that one security researcher (see comment 2) has discovered this independently, but it looks like he wants to hold it back until Firefox 4 is released so he can collect a bounty. While I don't want to speculate (or anyone else to!) in this bug about whether or not such a bounty should be granted anyway, it does mean that the chances of him weaponizing or zero-daying is low.

Accordingly, we should be respectful of our browser building colleagues and not zero day them. Rob and I agreed that this should be held until after they've had a chance to address the issue in their shipped products, so moving back to beta5+ - please do not land it until we've got the all clear or it becomes a zero-day issue.

Comment 18

[Reed Loden [:reed]]

Attachment 462633 [details] [diff] takes care of js/src/dtoa.c, comment #13 takes care of nsprpub/pr/src/misc/dtoa.c and nsprpub/pr/src/misc/prdtoa.c, but what about ipc/chromium/src/base/third_party/dmg_fp/dtoa.cc?

Comment 19

[Brendan Eich [:brendan]]

Cc'ing cjones. I hope we are not using that dtoa.cc. If not, can we rm it?

/be

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We Gecko certainly aren't.  The chromium code uses it for string utils, but I can build locally with all that code nuked.  Seeing what tryserver has to say about other platforms.

Comment 21

[Daniel Veditz [:dveditz]]

(In reply to comment #4)
> While our bug bounty FAQ does say we only give bounties for vulnerabilities
> in currently released versions [...], we have been known to offer the
> bounties for bugs found only on trunk, so what he says isn't completely true

It's not true in any sense, the FAQ explicitly includes nightlies http://www.mozilla.org/security/bug-bounty-faq.html#development-releases

Comment 22

[Daniel Veditz [:dveditz]]

Although the dtoa feature exists on the stable branches, it doesn't appear to be causing us any problems prior to recent JS engine changes.

Comment 23

[Jesse Ruderman]

Maybe he only saw the main bounty page and not the FAQ?  See bug 576166.

Comment 24

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #20)
> We Gecko certainly aren't.  The chromium code uses it for string utils, but I
> can build locally with all that code nuked.  Seeing what tryserver has to say
> about other platforms.

Looks good on tryserver.  Spun off bug 584580 on deleting it, as since it's unused there's no security concern.

Comment 25

[Robert Sayre]

(In reply to comment #17)
> 
> Accordingly, we should be respectful of our browser building colleagues and not
> zero day them. Rob and I agreed that this should be held until after they've
> had a chance to address the issue in their shipped products, so moving back to
> beta5+ - please do not land it until we've got the all clear or it becomes a
> zero-day issue.

I will not be so worried about this kind of coordination in the future:

http://trac.webkit.org/changeset/64706

Basically any moron with a copy of feedparser.py can figure out that there's a problem, since the checkin includes a testcase and a link to a security bug. No need for creative research around binary diffs or static analysis.

Comment 26

[Reed Loden [:reed]]

Can we go ahead and land the patch now then?

Comment 27

[Oliver Hunt]

*sigh*, the reference to the crash itself was unintentionally left in by geoff :-/

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

Oliver: preference/opinion on whether we should continue to hold? I'm assuming that we shouldn't now.

If we do respin beta3, we'll probably want to include this. If we land it on mozilla-central, can we also land it on GECKO20b3_20100804_RELBRANCH

Comment 29

[Oliver Hunt]

We think it can probably be landed if the patch doesn't refer to the crash and security implications, eg. something like geoff's patch sans the remaining "crash" comments - I had wanted something along the lines of "disallow non-standard extensions to float syntax" as our bug, but this is how it goes i guess :-/

Comment 30

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/3899c73806fe

Comment 31

[Mike Beltzner [:beltzner, not reading bugmail]]

http://hg.mozilla.org/mozilla-central/rev/34beabb64efd by rsayrer

Comment 32

[Reed Loden [:reed]]

(In reply to comment #28)
> If we do respin beta3, we'll probably want to include this. If we land it on
> mozilla-central, can we also land it on GECKO20b3_20100804_RELBRANCH

http://hg.mozilla.org/mozilla-central/rev/3e2124f5d0d6 for GECKO20b3_20100804_RELBRANCH

Comment 33

[Wan-Teh Chang]

Attachment: Patch for NSPR

NSPR doesn't define INFNAN_CHECK, but I added
    #define No_Hex_NaN
anyway so I don't need to worry about this.

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

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

Comment 34

[Luke Wagner [:luke]]

Comment on attachment 463762 [details] [diff] [review]
Patch for NSPR

Looks good, but I probably shouldn't be the reviewer for an NSPR patch.

Comment 35

[Brendan Eich [:brendan]]

Comment on attachment 463762 [details] [diff] [review]
Patch for NSPR

NSPR and I go way back...

/be