Last Comment Bug 916580 - (CVE-2013-5595) Use of uninitialized memory, and buffer size computations not checked for overflow
(CVE-2013-5595)
: Use of uninitialized memory, and buffer size computations not checked for ove...
Status: RESOLVED FIXED
[qa-][adv-main25+][adv-esr1710+][adv-...
: csectype-uninitialized, sec-moderate
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla27
Assigned To: Dan Gohman [:sunfish]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-09-15 08:13 PDT by Dan Gohman [:sunfish]
Modified: 2015-02-25 20:16 PST (History)
11 users (show)
dveditz: sec‑bounty+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
fixed
+
fixed
+
fixed
25+
fixed
25+
fixed
25+
fixed
fixed
fixed


Attachments
calloc-fixes.patch (3.74 KB, patch)
2013-09-15 08:13 PDT, Dan Gohman [:sunfish]
no flags Details | Diff | Review
calloc-fixes.patch (3.79 KB, patch)
2013-09-15 17:07 PDT, Dan Gohman [:sunfish]
luke: review+
Details | Diff | Review
calloc-fixes.patch (3.76 KB, patch)
2013-09-16 09:38 PDT, Dan Gohman [:sunfish]
dveditz: sec‑approval+
Details | Diff | Review
the patch, ported to the beta branch (3.71 KB, patch)
2013-09-23 16:58 PDT, Dan Gohman [:sunfish]
akeybl: approval‑mozilla‑beta+
Details | Diff | Review
the patch, ported to the aurora branch (3.74 KB, patch)
2013-09-23 16:59 PDT, Dan Gohman [:sunfish]
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review
the patch, ported to the esr24 branch (3.70 KB, patch)
2013-10-15 17:56 PDT, Dan Gohman [:sunfish]
no flags Details | Diff | Review
the patch, ported to the esr17 branch (586 bytes, patch)
2013-10-15 18:01 PDT, Dan Gohman [:sunfish]
abillings: approval‑mozilla‑esr17+
Details | Diff | Review
the patch, ported to the esr24 branch (3.66 KB, patch)
2013-10-15 18:08 PDT, Dan Gohman [:sunfish]
abillings: approval‑mozilla‑esr24+
Details | Diff | Review
the patch, ported to the b2g18 tree (1.64 KB, patch)
2013-10-16 13:29 PDT, Dan Gohman [:sunfish]
no flags Details | Diff | Review
the patch, ported to the b2g18 tree (1.66 KB, patch)
2013-10-17 15:44 PDT, Dan Gohman [:sunfish]
no flags Details | Diff | Review

Description Dan Gohman [:sunfish] 2013-09-15 08:13:07 PDT
Created attachment 805013 [details] [diff] [review]
calloc-fixes.patch

I observed this use of uninitialized memory in the js shell running under valgrind on Ubuntu Precise x86-64:

==21933== Conditional jump or move depends on uninitialised value(s)
==21933==    at 0x529A4E0: inflateReset2 (inflate.c:157)
==21933==    by 0x529A5D8: inflateInit2_ (inflate.c:193)
==21933==    by 0x816AE9: js::DecompressString(unsigned char const*, unsigned long, unsigned char*, unsigned long) (jsutil.cpp:132)
==21933==    by 0x5ABDA4: JSRuntime::initSelfHosting(JSContext*) (SelfHosting.cpp:753)
==21933==    by 0x6CDB95: js::NewContext(JSRuntime*, unsigned long) (jscntxt.cpp:201)
==21933==    by 0x68B9B2: JS_NewContext(JSRuntime*, unsigned long) (jsapi.cpp:827)
==21933==    by 0x416940: NewContext(JSRuntime*) (js.cpp:4914)
==21933==    by 0x418CEB: main (js.cpp:5546)

This bug is caused by code in jsutil.cpp allocating memory for zlib using regular malloc, where zlib expects the memory to be zeroed.

While investigating this, I also noticed that the multiplication computing the buffer size for the malloc call is not checked for overflow. Informally looking around, I found a few other places that were performing unchecked multiplications to compute buffer sizes. Attached is a patch which fixes all of the bugs I found:

The change in jsd_lock.cpp is a boring cleanup.

The change in Utility.h introduces an overload of js_calloc with two arguments. This is used by the jsutil.cpp fixes described below.

The changes in BaselineBailouts.cpp and IonCode.h add overflow checks on multiplications computing buffer sizes. It might be impossible to force overflows in these places, because other things would very likely fail first, but there wasn't anything explicitly protecting them. This patch explicitly protects them.

The change in jsutil.cpp fixes two bugs: It zeros out the allocated memory, which fixes the uninitialized memory bug described above. And, it uses the two-argument calloc, allowing libc to check the multiplication for overflow.
Comment 1 Dan Gohman [:sunfish] 2013-09-15 17:07:36 PDT
Created attachment 805087 [details] [diff] [review]
calloc-fixes.patch

This patch corrects an error in the earlier patch.
Comment 2 Luke Wagner [:luke] 2013-09-16 08:26:29 PDT
Comment on attachment 805087 [details] [diff] [review]
calloc-fixes.patch

Review of attachment 805087 [details] [diff] [review]:
-----------------------------------------------------------------

Great finds!

::: js/src/jit/IonCode.h
@@ +581,1 @@
>              successors_ = (uint32_t *) js_calloc(numSuccessors * sizeof(uint32_t));

js_pod_calloc<uint32_t>(numSuccessors)?

@@ +681,1 @@
>          blocks_ = (IonBlockCounts *) js_calloc(numBlocks * sizeof(IonBlockCounts));

js_pod_calloc<IonBlockCounts>(numBlocks)?
Comment 3 Dan Gohman [:sunfish] 2013-09-16 09:38:03 PDT
Created attachment 805388 [details] [diff] [review]
calloc-fixes.patch

(In reply to Luke Wagner [:luke] from comment #2)
> ::: js/src/jit/IonCode.h
> @@ +581,1 @@
> >              successors_ = (uint32_t *) js_calloc(numSuccessors * sizeof(uint32_t));
> 
> js_pod_calloc<uint32_t>(numSuccessors)?
> 
> @@ +681,1 @@
> >          blocks_ = (IonBlockCounts *) js_calloc(numBlocks * sizeof(IonBlockCounts));
> 
> js_pod_calloc<IonBlockCounts>(numBlocks)?

That's a good idea. It simplifies them, and is a nicer solution for the overflow checking too. Attached is an updated patch.
Comment 4 Dan Gohman [:sunfish] 2013-09-16 09:59:39 PDT
Comment on attachment 805388 [details] [diff] [review]
calloc-fixes.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

For the uninitialized memory bug, it looks possible to get a free of an uninitialized pointer value, from which an exploit could likely be crafted. The overflow bugs are probably harder; I tried to craft a testcase, but my attempts hit other limits before they became exploitable.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The fixes in the patch make it clear what bugs this is fixing, and what an attacker would need to focus on to exploit them. There's no known testcase at this time though.

Which older supported branches are affected by this flaw?

Both the uninitialized memory bug and the overflow bugs are in all trees back to at least mozilla-release.

If not all supported branches, which bug introduced the flaw?

The uninitialized memory bug appears to come from e080642175e6 (bug 761723). The overflow bugs from ef6530d96b63 (bug 811349) and ed51ae24fee2 (bug 805877).

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

I don't have backports at the moment, but they would be easy to create.

How likely is this patch to cause regressions; how much testing does it need?

It is very unlikely to cause regressions. The fixes are simple changes. It shouldn't need any extraordinary testing.
Comment 5 [On PTO until 6/29] 2013-09-17 04:41:27 PDT
Can someone suggest a security rating?

This looks to be necessary on all branches except for maybe ESR17?
Comment 6 Daniel Veditz [:dveditz] 2013-09-23 14:03:37 PDT
Comment on attachment 805388 [details] [diff] [review]
calloc-fixes.patch

sec-approval+

The only part we'll need on esr17 is the jsutils/Utilities bit, there's no IonMonkey or BaselineCompiler on that branch. Probably don't need this at all on the b2g18 branch since we've shipped 1.1 and 1.2 will be based on mozilla-central circa Fx26.
Comment 7 Dan Gohman [:sunfish] 2013-09-23 16:58:41 PDT
Created attachment 808896 [details] [diff] [review]
the patch, ported to the beta branch
Comment 8 Dan Gohman [:sunfish] 2013-09-23 16:59:22 PDT
Created attachment 808897 [details] [diff] [review]
the patch, ported to the aurora branch
Comment 9 Dan Gohman [:sunfish] 2013-09-25 12:23:20 PDT
Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9ae7613751c
Comment 10 Wes Kocher (:KWierso) 2013-09-25 20:38:36 PDT
https://hg.mozilla.org/mozilla-central/rev/f9ae7613751c
Comment 11 Alex Keybl [:akeybl] 2013-10-08 07:28:25 PDT
Ready for uplift to Aurora/Beta if others are. Please nominate before Thursday to ensure this makes it into FF25.
Comment 12 Dan Gohman [:sunfish] 2013-10-08 08:09:33 PDT
Comment on attachment 808897 [details] [diff] [review]
the patch, ported to the aurora branch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 761723
User impact if declined: 

If declined, the potential security flaw will remain unfixed.

Testing completed (on m-c, etc.):

It is tested on m-c.

Risk to taking this patch (and alternatives if risky): 

Low. The patch is small and simple.

String or IDL/UUID changes made by this patch:

None.
Comment 13 Dan Gohman [:sunfish] 2013-10-08 08:10:34 PDT
Comment on attachment 808896 [details] [diff] [review]
the patch, ported to the beta branch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 761723
User impact if declined: 

If declined, the potential security flaw will remain unfixed.

Testing completed (on m-c, etc.):

It is tested on m-c.

Risk to taking this patch (and alternatives if risky): 

Low. The patch is small and simple.

String or IDL/UUID changes made by this patch:

None.
Comment 14 Ryan VanderMeulen [:RyanVM] 2013-10-11 08:34:42 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/6cfb2b0f594b
https://hg.mozilla.org/releases/mozilla-beta/rev/ac54c9fcd528

Not sure what the plan is for esr/b2g uplifts.
Comment 15 Daniel Veditz [:dveditz] 2013-10-11 13:59:52 PDT
(In reply to Daniel Veditz [:dveditz] from comment #6)
> The only part we'll need on esr17 is the jsutils/Utilities bit, there's no
> IonMonkey or BaselineCompiler on that branch. Probably don't need this at
> all on the b2g18 branch since we've shipped 1.1 and 1.2 will be based on
> mozilla-central circa Fx26.

... but apparently we're supporting FirefoxOS 1.1 until March, so we should in fact take this on b2g18, too.

Note to Release Managers: I think we should take this bug on the ESR branches because it's a simple and safe fix compared to my fear that sec-moderate is possibly a low-ball rating. It's /probably/ hard to exploit, but this is a "better safe than sorry" kind of bug.
Comment 16 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-10-15 13:43:27 PDT
Dan, is there any information you can provide to guide QA in verifying this is fixed?
Comment 17 Dan Gohman [:sunfish] 2013-10-15 14:24:33 PDT
I am able to reproduce the valgrind error by running an unpatched js built with no configure options on an x86_64 Ubuntu system on an empty JS file.

$ : > t.js
$ valgrind js t.js
[...]
==15244== Conditional jump or move depends on uninitialised value(s)
==15244==    at 0x4C3B4E0: inflateReset2 (in /lib/x86_64-linux-gnu/libz.so.1.2.3.4)
==15244==    by 0x4C3B5D8: inflateInit2_ (in /lib/x86_64-linux-gnu/libz.so.1.2.3.4)
==15244==    by 0x616629: js::DecompressString(unsigned char const*, unsigned long, unsigned char*, unsigned long)
[...]

With the patch, this error no longer occurs. Presently, this is the only method I have of observing this bug in action.

The other bugs mentioned here were found by inspection, and I don't currently know of a way to reproduce them.
Comment 18 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-10-15 16:14:22 PDT
Okay, thanks for testing Dan. I'm going to mark this is [qa-] since there doesn't seem to be a good way to verify this 100%. If there's something specific we can test to help you here then please let us know.
Comment 19 [On PTO until 6/29] 2013-10-15 16:46:45 PDT
Dan, can we get ESR17 and ESR25 patches here since we're tracking it for those releases?
Comment 20 Dan Gohman [:sunfish] 2013-10-15 17:56:41 PDT
Created attachment 817556 [details] [diff] [review]
the patch, ported to the esr24 branch
Comment 21 Dan Gohman [:sunfish] 2013-10-15 18:01:40 PDT
Created attachment 817559 [details] [diff] [review]
the patch, ported to the esr17 branch

The less important fixes don't apply in esr17 (no ion?), so I left them out. The main uninitialized memory fix does apply, with a minor adjustment to use the OffTheBooks API as the existing code does.
Comment 22 Dan Gohman [:sunfish] 2013-10-15 18:08:38 PDT
Created attachment 817563 [details] [diff] [review]
the patch, ported to the esr24 branch

And here's an update to the esr24 patch fixing to avoid using mozilla::tl::MulOverflowMask, since esr24 doesn't have that.
Comment 23 [On PTO until 6/29] 2013-10-16 11:18:02 PDT
ESR17 and ESR24 patches approved. Let's get them checked in.
Comment 24 Ryan VanderMeulen [:RyanVM] 2013-10-16 13:04:08 PDT
https://hg.mozilla.org/releases/mozilla-esr24/rev/ed6cbcb9347e
https://hg.mozilla.org/releases/mozilla-esr17/rev/1a5f6504d1e4

Dan said he'll work up a b2g18 patch for approval as well.
Comment 25 Dan Gohman [:sunfish] 2013-10-16 13:29:27 PDT
Created attachment 818037 [details] [diff] [review]
the patch, ported to the b2g18 tree

Here's the patch ported to the b2g18 tree. I omitted the ion parts, since RyanVM mentioned that ion is disabled in this tree.
Comment 26 bhavana bajaj [:bajaj] 2013-10-16 14:02:17 PDT
(In reply to Dan Gohman [:sunfish] from comment #25)
> Created attachment 818037 [details] [diff] [review]
> the patch, ported to the b2g18 tree
> 
> Here's the patch ported to the b2g18 tree. I omitted the ion parts, since
> RyanVM mentioned that ion is disabled in this tree.

a=bajaj for landing this on b2g18.
Comment 27 Ryan VanderMeulen [:RyanVM] 2013-10-17 09:12:57 PDT
https://hg.mozilla.org/releases/mozilla-b2g18/rev/05db0f8744fa
Comment 29 Dan Gohman [:sunfish] 2013-10-17 15:44:56 PDT
Created attachment 818750 [details] [diff] [review]
the patch, ported to the b2g18 tree

Here's an updated b2g18 patch which renames the new js_calloc to js_calloc_members to work around the fact that in that tree, js_calloc is in an extern "C" block where overloading is not permitted. Deviating from the original patch isn't ideal, but this minimizes unnecessary changes to the tree.
Comment 30 Dan Gohman [:sunfish] 2013-10-17 15:50:41 PDT
Comment on attachment 818750 [details] [diff] [review]
the patch, ported to the b2g18 tree

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 761723
User impact if declined: The potential security flaw would remain unfixed.
Testing completed: The original patch has been in mozilla-central for a few weeks.
Risk to taking this patch (and alternatives if risky): Probably very low. The patch is small and simple.
String or UUID changes made by this patch: None.
Comment 31 Ryan VanderMeulen [:RyanVM] 2013-10-18 06:19:26 PDT
Comment on attachment 818750 [details] [diff] [review]
the patch, ported to the b2g18 tree

Already had approval to land :)
Comment 32 Ryan VanderMeulen [:RyanVM] 2013-10-18 06:23:48 PDT
https://hg.mozilla.org/releases/mozilla-b2g18/rev/53542d252c2d

Note You need to log in before you can comment on or make changes to this bug.