Closed Bug 516862 Opened 15 years ago Closed 15 years ago

Array indexing error in js/src/dtoa.c's Balloc() leads to floating point memory vulnerability (SA36711)

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta1-fixed
blocking1.9.1 --- .4+
status1.9.1 --- .4-fixed

People

(Reporter: reed, Assigned: reed)

References

Details

(6 keywords, Whiteboard: [sg:critical] same issue as 516396 (CVE-2009-0689))

Attachments

(4 files, 3 obsolete files)

[split from bug 516396]

Secunia notified security@ of a problem in nsprpub/pr/src/misc/prdtoa.c's Balloc() function. The same issue also occurs in js/src/dtoa.c, so the patch from bug 416396 needs to be ported to that copy of dtoa.
Flags: blocking1.9.2?
Flags: blocking1.9.0.15?
blocking1.9.1: .4+ → ?
Whiteboard: [sg:critical] same issue as 516396 (CVE-2009-1563)
Attached patch patch - v1Splinter Review
I ported wtc's patch in bug 516396 to the copy js uses.

I did a diff between the two files, and there are a lot of differences, mostly in casting, but there are some code changes as well...
Assignee: general → reed
Status: NEW → ASSIGNED
Attachment #400936 - Flags: review?(crowder)
blocking1.9.1: ? → .4+
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.15?
Flags: blocking1.9.0.15+
Comment on attachment 400936 [details] [diff] [review]
patch - v1

Thanks, reed...  You may want/need a proper js peer for this, too.  But it looks good to me.
Attachment #400936 - Flags: review?(crowder) → review+
Comment on attachment 400936 [details] [diff] [review]
patch - v1

Since it's a security bug, need sr anyway... tagging brendan.
Attachment #400936 - Flags: superreview?(brendan)
Comment on attachment 400936 [details] [diff] [review]
patch - v1

js/src is still exempt from the sr policy, but I'll stamp this as a JS peer.
Attachment #400936 - Flags: superreview?(brendan) → review+
http://hg.mozilla.org/mozilla-central/rev/2aa5b8b5afe9
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Attachment #400936 - Flags: approval1.9.2?
Attachment #400936 - Flags: approval1.9.1.4?
Attachment #400936 - Flags: approval1.9.0.15?
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.next?
Attachment #400936 - Flags: approval1.9.1.4?
Attachment #400936 - Flags: approval1.9.1.4+
Attachment #400936 - Flags: approval1.9.0.15?
Attachment #400936 - Flags: approval1.9.0.15+
Attachment #400936 - Flags: approval1.8.1.next?
Comment on attachment 400936 [details] [diff] [review]
patch - v1

Approved for 1.9.1.4 and 1.9.0.15, a=dveditz
Blocking 1.9.2+.  P1.
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P1
Attachment #400936 - Flags: approval1.9.2?
Comment on attachment 400936 [details] [diff] [review]
patch - v1

1.9.0 is completely different... need a new patch.
Attachment #400936 - Flags: approval1.9.0.15+
Attachment #400936 - Flags: approval1.8.1.next?
Untested patch for 1.9.0. This version of dtoa doesn't have any of the locking stuff, so definitely please take a closer look here to make sure something isn't missing or wrong or something.
Attachment #401451 - Flags: review?(mrbkap)
Attachment #401451 - Flags: review?(crowder)
Attachment #401451 - Flags: review?(crowder) → review+
Comment on attachment 401451 [details] [diff] [review]
patch - v1 (1.9.0)

Rubber-stamp.  Are we even sure this version has the same bug?
Is there a way to reproduce this problem on both 1.9.1 and 1.9.0 for verification of the fix? I assume not.
Attachment #401451 - Flags: review?(mrbkap) → review+
Attachment #401451 - Flags: approval1.9.0.15?
Comment on attachment 401451 [details] [diff] [review]
patch - v1 (1.9.0)

Approved for 1.9.0.15, a=dveditz
Attachment #401451 - Flags: approval1.9.0.15? → approval1.9.0.15+
Checking in js/src/jsdtoa.c;
/cvsroot/mozilla/js/src/jsdtoa.c,v  <--  jsdtoa.c
new revision: 3.42; previous revision: 3.41
done
Keywords: fixed1.9.0.15
The testcases in bug 516396 use JavaScript -- are they testing the NSPR version of the function in that bug or are they really testing _this_ fix?
Flags: blocking1.8.1.next? → blocking1.8.1.next+
They appear to be testing this bug. They turned what appeared to be a hang into an outright immediate crash. Slightly different places, I'm not sure I'm happy with either.
  bp-f2a1d879-1b83-4a00-b844-e1e0a2090923
  bp-b026cdaa-d503-4d54-b2b1-f4b4a2090923
I actually created an NSPR test case for this bug, but
it took too long to finish, so I can't add it to the
NSPR test suite.

Should we consider failing early if the input string
is too long?
dveditz: the first crash report crashed in malloc:
  bp-f2a1d879-1b83-4a00-b844-e1e0a2090923
Perhaps it ran out of memory?
Wan-Teh, can you grab last night's 1.9.0 build and run your test case for this bug against it in order to verify that this bug is fixed?
(In reply to comment #19)
> Wan-Teh, can you grab last night's 1.9.0 build and run your test case for this
> bug against it in order to verify that this bug is fixed?

This bug isn't fixed, as per bug 516396. Need a different patch for both bugs.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Al, I don't have a test case for this JavaScript bug.
Reed's test case in NSPR bug 516396 comment 2 is actually
a test case for this bug.  You can use Reed's test case
to verify this bug is fixed.
Running the bug 516396 comment 2 testcase on Firefox 3.5.3 causes a crash. When I run it on a nightly 3.5.4 build, Firefox hangs (without actually crashing). Is this really fixed?

Tested on XP with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.4pre) Gecko/20090924 Shiretoko/3.5.4pre (.NET CLR 3.5.30729.
Al: the hang is expected.  Firefox doesn't really hang; that
test case causes Firefox to take a very long time to convert
the string to a double.

I have an NSPR patch in bug 516396 comment 41 to address
the hang.  You may want a similar patch for JavaScript.
Not sure how you can verify something that isn't completely fixed yet. We need to port the second patch from the NSPR version of this bug over to js. I've been working on a patch in my spare time, but I just haven't had time to finish it completely yet.
Keywords: verified1.9.1
Reed: In JavaScript, you can do the string length check in
jsdtoa.cpp, function JS_strtod:

129 JS_FRIEND_API(double)
130 JS_strtod(const char *s00, char **se, int *err)
131 {
132     double retval;
        // Check s00 length here!
133     if (err)
134         *err = 0;
135     LOCK_DTOA();
136     retval = _strtod(s00, se);
137     UNLOCK_DTOA();
138     return retval;
139 }

You need to add JS_DTOA_EINVAL if you want to follow
the 0 return value and EINVAL error code in the strtod'
man page in Single Unix Specification:
http://www.opengroup.org/onlinepubs/9699919799/functions/strtod.html

Note: |err| is not set by JS_strtod correctly on return.  You need
to do this:

    LOCK_DTOA();
+   errno = 0;
    retval = _strtod(s00, se);
+   if (err) {
+       switch (errno) {
+       case ERANGE:
+           *err = JS_DTOA_ERANGE;
+           break;
+       case ENOMEM:
+           *err = JS_DTOA_ENOMEM;
+           break;
+       }
+   }
    UNLOCK_DTOA();
I was marking it "fixed" based on the comment that the hang without a crash was expected.

We're most of a week past code complete now and cutting builds within the new week. If your patch doesn't get checked in Reed, this is going to ship as it is now.

So, everyone, is the current behavior acceptable and "fixed" or not? If not, perhaps we need to back the whole thing out rather than ship half of a fix?

Dveditz, any comments?
Al: the hang does not allow the attacker to execute arbitrary code,
so don't back out what's checked in.
Good point. It isn't my call anyway. It is up to Dveditz and Beltzner to make the call. The question is whether we need Reed's patch or not. I assume we do but it seems likely to miss the train at this point, leaving this bug in limbo.
Thanks, wtc. I was on the right path, but you greatly simplified what I was doing.

JS_DTOA_ENOMEM doesn't seem to be actually used anywhere, as far as actually being set. dtoa.c never sets errno = ENOMEM directly, though I guess one of the memory-related functions could do it.

I just made JS_DTOA_EINVAL return out-of-memory error like JS_DTOA_ENOMEM. If wanted, I can add a new error number/message for it.

I haven't tested this yet... just getting the patch posted before I start a build.
Attachment #403404 - Flags: review?(mrbkap)
Attachment #403404 - Flags: review?(crowder)
Comment on attachment 403404 [details] [diff] [review]
Add an input length check to _strtod - v1

It concerns me that this length-checking stuff isn't being done inside dtoa, since I think it means you're paying to walk the string twice...  what're the perf implications of that?  Is this just meant to prevent a DOS?
Comment on attachment 403404 [details] [diff] [review]
Add an input length check to _strtod - v1

Reed: I like your change to JS_strtod.

We should do the string length check in JS_strtod instead
of _strtod because I believe dtoa.c should be kept as close
to the upstream version as possible.

We can report a long input string as JSMSG_NEED_DIET, or
JSMSG_CANT_CONVERT, or JSMSG_CANT_CONVERT_TO.
Whiteboard: [sg:critical] same issue as 516396 (CVE-2009-1563) → [needs r=mrbkap or crowder][sg:critical] same issue as 516396 (CVE-2009-1563)
The ship has already sailed on keeping dtoa.c close-to-upstream, we should do the thing that least impacts perf here.  Can we emulate the length-check that NSPR got here instead, please?
(In reply to comment #32)
> The ship has already sailed on keeping dtoa.c close-to-upstream, we should do
> the thing that least impacts perf here.  Can we emulate the length-check that
> NSPR got here instead, please?

The length-check in my patch is the exact same one that NSPR is using, so I'm not sure what you mean. wtc did post a new patch in bug 516396 to do the length-check inline, but it's not complete or ready yet.
What's not ready about wtc's inline length-check?  Also, hasn't the actual security-sensitive part of this landed?  The length-check only prevents a hang/DoS, unless I am misunderstanding.

If I'm right, we shouldn't rush in a patch that -must- have performance implications when we have something potentially far better (even if it means waiting a short while for the "far better" bit).
(In reply to comment #34)
> What's not ready about wtc's inline length-check?

Read the entire second paragraph in bug 516396, comment #57.
JS doesn't allow strings that long anyway.
ok, how about this then?
Attachment #403404 - Attachment is obsolete: true
Attachment #404370 - Flags: review?(mrbkap)
Attachment #404370 - Flags: review?(crowder)
Attachment #403404 - Flags: review?(mrbkap)
Attachment #403404 - Flags: review?(crowder)
Comment on attachment 404370 [details] [diff] [review]
Add an input length check to _strtod - v2

>  ret0:
>+			errno = EINVAL;
> 			s = s00;
> 			sign = 0;

Nit: You may want to put that inside #ifndef NO_ERRNO.
(In reply to comment #38)
> Nit: You may want to put that inside #ifndef NO_ERRNO.

Yeah, I thought about that, but the comment describing NO_ERRNO clearly says:
 * #define NO_ERRNO if strtod should not assign errno = ERANGE when
 *      the result overflows to +-Infinity or underflows to 0.
... and this is for EINVAL, not ERANGE. I guess I can just update the comment unless somebody thinks that define is specific to ERANGE for some reason.
with the NO_ERRNO #ifndef
Attachment #404370 - Attachment is obsolete: true
Attachment #404380 - Flags: review?(mrbkap)
Attachment #404380 - Flags: review?(crowder)
Attachment #404370 - Flags: review?(mrbkap)
Attachment #404370 - Flags: review?(crowder)
Blocks: 520629
The original crash/security bug is fixed everywhere. The hang that results from not crashing before we process the entire string has been moved to bug 520629 and can be fixed in the next release.
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Keywords: fixed1.9.0.15
Resolution: --- → FIXED
Does this mean that I can undo Reed's removal of the Verified1.9.1 keyword now?
Attachment #404380 - Flags: review?(mrbkap) → review+
Comment on attachment 404380 [details] [diff] [review]
Add an input length check to _strtod - v2.1

>diff --git a/js/src/jsdtoa.cpp b/js/src/jsdtoa.cpp
>+        switch (errno) {
>+            case ERANGE:
>+                *err = JS_DTOA_ERANGE;
>+                break;
>+            case ENOMEM:
>+                *err = JS_DTOA_ENOMEM;
>+                break;
>+            case EINVAL:
>+                *err = JS_DTOA_EINVAL;
>+                break;
>+            default:
>+                *err = 0;
>+                break;

Nit: the case labels should be indented 2 past the switch and the actual case bodies should be indented 2 past the case labels.
Attachment #404380 - Flags: review?(crowder) → review+
It is a little weird that we're reporting OOM for EINVAL here.  Is there a better error we could use?
(In reply to comment #43)
> Nit: the case labels should be indented 2 past the switch and the actual case
> bodies should be indented 2 past the case labels.

I based the whitespacing in my patch on prevailing style of another switch in jsdtoa.cpp: http://mxr.mozilla.org/mozilla-central/source/js/src/jsdtoa.cpp#191

Do you still wish for me to change the whitespacing to meet your nit?
(In reply to comment #44)
> It is a little weird that we're reporting OOM for EINVAL here.  Is there a
> better error we could use?

I asked mrbkap about this a while ago, and he recommended using OOM, but if something else is wanted, I can use another error or create a new one...
Any new patches will be attached to bug 520629.
Whiteboard: [needs r=mrbkap or crowder][sg:critical] same issue as 516396 (CVE-2009-1563) → [sg:critical] same issue as 516396 (CVE-2009-1563)
Re: reporting OOM for EINVAL

I already looked into better errors we could use in comment 31.
(Caveat: I am not familiar with the JS source code.)  I
suggested some alternative errors that we can report for EINVAL:
JSMSG_NEED_DIET, JSMSG_CANT_CONVERT, or JSMSG_CANT_CONVERT_TO.
No longer blocks: 520629
Depends on: 520629
Depends on: CVE-2009-0689
Verified fixed on trunk, 1.9.2, and 1.9.1 based on the JS testcase in bug 516396 comment 2. No crash happens anymore.

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
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Keywords: verified1.9.1
Comment on attachment 401451 [details] [diff] [review]
patch - v1 (1.9.0)

This patch is wrong. In Balloc(), the Bigint *rv is a local variable and when k > Kmax happens the rv variable is used w/o initialization.
static Bigint *Balloc(int32 k)
{
    int32 x;
    Bigint *rv;

    [...]

    if (k <= Kmax && (rv = freelist[k]) != NULL)
        freelist[k] = rv->next;
    if (rv == NULL) { << here, if k > Kmax
(In reply to comment #50)
> (From update of attachment 401451 [details] [diff] [review])
> This patch is wrong. In Balloc(), the Bigint *rv is a local variable and when k
> > Kmax happens the rv variable is used w/o initialization.

Huh? It's (rv = freelist[k]), so rv is set to freelist[k] during the |if|.
Isn't short boolean evaluation by default in C++?
Anyway, freelist[] is a static array of [Kmax+1] elements so it should not be accessed when k > Kmax+1, right?
(In reply to comment #53)
> Isn't short boolean evaluation by default in C++?

This is C, not C++ (both nsprpub/pr/src/misc/prdtoa.c and js/src/dtoa.c).

(In reply to comment #54)
> Anyway, freelist[] is a static array of [Kmax+1] elements so it should not be
> accessed when k > Kmax+1, right?

Sure, which is why the check is for |k <= Kmax| before freelist[] is accessed.
(In reply to comment #55)
> (In reply to comment #53)
> > Isn't short boolean evaluation by default in C++?
> 
> This is C, not C++ (both nsprpub/pr/src/misc/prdtoa.c and js/src/dtoa.c).

Ah, but js/src/jsdtoa.cpp is C++, and it |#include|s dtoa.c, so yes, it is compiled using C++. Sorry about that.
Yes, but I'm still talking about uninitialized rv when (k > Kmax).
Oh! I was looking at the NSPR code and the upstream dtoa.c, not the js code. My apologies. This is what I get for reading bugmail at 5am.

This problem is because we do the check slightly differently between NSPR and js code:

NSPR:
	if (k <= Kmax && (rv = freelist[k]))
 		freelist[k] = rv->next;
 	else {

js:
     if (k <= Kmax && (rv = freelist[k]) != NULL)
         freelist[k] = rv->next;
     if (rv == NULL) {

Martin correctly points out that rv could never be set because we'll short-circuit on |k <= Kmax| if that returns false. Simple fix would be just to swap to how NSPR does it, I think. Does that seem reasonable?
No, my last comment is still wrong. Basically, the patch we use for 1.9.1 and greater is fine. It's the 1.9.0 patch (attachment 401451 [details] [diff] [review]) that is wrong. *sigh*
Thanks, Martin, for noticing this. My apologies again for looking at the wrong patch when trying to check what you were seeing.

Anyway, this changes the code to look more like what NSPR and js >= 1.9.1 is using, which I believe should fix this issue.

I did say "untested patch" in comment #10! :)
Attachment #404380 - Attachment is obsolete: true
Attachment #406420 - Flags: review?(mrbkap)
Attachment #406420 - Flags: review?(crowder)
Doesn't look like this was ever verified on 1.9.0.15 either.
Keywords: fixed1.9.0.15
Comment on attachment 406420 [details] [diff] [review]
Follow-up to 1.9.0 patch to fix use of uninitialized 'rv' - v1

r=wtc.  Nice catch, Martin.
Attachment #406420 - Flags: review+
Comment on attachment 406420 [details] [diff] [review]
Follow-up to 1.9.0 patch to fix use of uninitialized 'rv' - v1

Yes, thanks Martin!
Attachment #406420 - Flags: review?(crowder) → review+
Attachment #406420 - Flags: review?(mrbkap) → approval1.9.0.15?
(In reply to comment #10)
> Untested patch for 1.9.0. This version of dtoa doesn't have any of the locking
> stuff, so definitely please take a closer look here to make sure something
> isn't missing or wrong or something.

Why was this untested? We should never land untested patches on stable branches.
It's OK that it was untested at that point.  It's not really OK that it was review-stamped without testing, or that it was approved based on a rubber stamp of an untested patch, though!

Thanks, Martin -- you saved us from an embarrassing error here.
Did the new, follow-up patch get tested?
(In reply to comment #66)
> Did the new, follow-up patch get tested?

This is sort of important, folks -- has anyone tested the follow-up patch?  Can they chime in here if so, before we land another untested patch?
(In reply to comment #67)
> (In reply to comment #66)
> > Did the new, follow-up patch get tested?
> 
> This is sort of important, folks -- has anyone tested the follow-up patch?  Can
> they chime in here if so, before we land another untested patch?

I just sent it off to the try server. Once a build pops out, I'll test it against the testcases.
(In reply to comment #68) 
> I just sent it off to the try server. Once a build pops out, I'll test it
> against the testcases.

I can confirm that the follow-up patch does indeed fix the crash.
Attachment #406420 - Flags: approval1.9.0.16+
Attachment #406420 - Flags: approval1.9.0.15?
Attachment #406420 - Flags: approval1.9.0.15+
Comment on attachment 406420 [details] [diff] [review]
Follow-up to 1.9.0 patch to fix use of uninitialized 'rv' - v1

Dan's looked over this patch as well, which makes me a bit more confident.

Approved for 1.9.0.15 and 1.9.0.16. a=ss for release-drivers. Please land on both HEAD and GECKO190_20091006_RELBRANCH as soon as possible.
CVS HEAD:

Checking in js/src/jsdtoa.c;
/cvsroot/mozilla/js/src/jsdtoa.c,v  <--  jsdtoa.c
new revision: 3.43; previous revision: 3.42
done

CVS GECKO190_20091006_RELBRANCH:

Checking in js/src/jsdtoa.c;
/cvsroot/mozilla/js/src/jsdtoa.c,v  <--  jsdtoa.c
new revision: 3.42.2.1; previous revision: 3.42
done
Keywords: fixed1.9.0.15
Keywords: fixed1.9.0.16
I've run the testcase in bug 516396, which doesn't crash on 1.9.0.15 (and does on 1.9.0.14) but takes five minutes to run. 

I find the comments above less than clear so, to be 100% certain, I ask, "How do we verify *this* fix for 1.9.0.15?"
Per conversation with Sam, this is verified via the testcase in bug 516396 (which should be attached here) and is now verified for 1.9.0.
I've also reverified this for 1.9.1 with build 3 of 1.9.1.
Group: core-security
Verified this again for 1.9.0.16 (since it needed to land in 1.9.0.15 and .16 separately) with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.16pre) Gecko/2009111921 GranParadiso/3.0.16pre (.NET CLR 3.5.30729).
Whiteboard: [sg:critical] same issue as 516396 (CVE-2009-1563) → [sg:critical] same issue as 516396 (CVE-2009-0869)
Whiteboard: [sg:critical] same issue as 516396 (CVE-2009-0869) → [sg:critical] same issue as 516396 (CVE-2009-0689)
Landed on MOZILLA_1_8_BRANCH:
mozilla/js/src/jsdtoa.c 	3.33.2.5
Keywords: fixed1.8.1.24
Verified for 1.8.1.24 using Seamonkey (Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.24pre) Gecko/20100225 SeaMonkey/1.1.19pre).
Flags: in-testsuite? → in-testsuite-
You need to log in before you can comment on or make changes to this bug.