Bugzilla@Mozilla – Bug 516862
Array indexing error in js/src/dtoa.c's Balloc() leads to floating point memory vulnerability (SA36711)
Last modified: 2010-02-26 17:37:22 PST
Summon comment box
[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.
Created attachment 400936 [details] [review] patch - v1 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...
Comment on attachment 400936 [details] [review] patch - v1 Thanks, reed... You may want/need a proper js peer for this, too. But it looks good to me.
Comment on attachment 400936 [details] [review] patch - v1 Since it's a security bug, need sr anyway... tagging brendan.
Comment on attachment 400936 [details] [review] patch - v1 js/src is still exempt from the sr policy, but I'll stamp this as a JS peer.
http://hg.mozilla.org/mozilla-central/rev/2aa5b8b5afe9
Comment on attachment 400936 [details] [review] patch - v1 Approved for 1.9.1.4 and 1.9.0.15, a=dveditz
Blocking 1.9.2+. P1.
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/11ec9dc6d09a http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cbc19a6d7c45
Comment on attachment 400936 [details] [review] patch - v1 1.9.0 is completely different... need a new patch.
Created attachment 401451 [details] [review] patch - v1 (1.9.0) 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.
Comment on attachment 401451 [details] [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.
Comment on attachment 401451 [details] [review] patch - v1 (1.9.0) Approved for 1.9.0.15, a=dveditz
Checking in js/src/jsdtoa.c; /cvsroot/mozilla/js/src/jsdtoa.c,v <-- jsdtoa.c new revision: 3.42; previous revision: 3.41 done
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?
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.
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.
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.
Created attachment 403404 [details] [review] Add an input length check to _strtod - v1 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.
Comment on attachment 403404 [details] [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] [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.
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.
Created attachment 404370 [details] [review] Add an input length check to _strtod - v2 ok, how about this then?
Comment on attachment 404370 [details] [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.
Created attachment 404380 [details] [review] Add an input length check to _strtod - v2.1 with the NO_ERRNO #ifndef
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.
Does this mean that I can undo Reed's removal of the Verified1.9.1 keyword now?
Comment on attachment 404380 [details] [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.
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.
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.
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
Comment on attachment 401451 [details] [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] [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] [review]) that is wrong. *sigh*
Created attachment 406420 [details] [review] Follow-up to 1.9.0 patch to fix use of uninitialized 'rv' - v1 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! :)
Doesn't look like this was ever verified on 1.9.0.15 either.
Comment on attachment 406420 [details] [review] Follow-up to 1.9.0 patch to fix use of uninitialized 'rv' - v1 r=wtc. Nice catch, Martin.
Comment on attachment 406420 [details] [review] Follow-up to 1.9.0 patch to fix use of uninitialized 'rv' - v1 Yes, thanks Martin!
(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.
Comment on attachment 406420 [details] [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
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.
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).
Created attachment 427723 [details] [review] rollup for MOZILLA_1_8_BRANCH Attachment 401451 [details] with attachment 406420 [details] [review] applied on top of it.
Landed on MOZILLA_1_8_BRANCH: mozilla/js/src/jsdtoa.c 3.33.2.5
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).