916580 - (CVE-2013-5595) Use of uninitialized memory, and buffer size computations not checked for overflow

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Dan Gohman [:sunfish]]

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

Attachment: calloc-fixes.patch

This patch corrects an error in the earlier patch.

Comment 2

[Luke Wagner [:luke]]

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

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

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

[Al Billings [:abillings - ex-MoCo]]

Can someone suggest a security rating?

This looks to be necessary on all branches except for maybe ESR17?

Comment 6

[Daniel Veditz [:dveditz]]

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

Attachment: the patch, ported to the beta branch

Comment 8

[Dan Gohman [:sunfish]]

Attachment: the patch, ported to the aurora branch

Comment 9

[Dan Gohman [:sunfish]]

Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9ae7613751c

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f9ae7613751c

Comment 11

[Alex Keybl [:akeybl]]

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

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

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

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

(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

[u279076]

Dan, is there any information you can provide to guide QA in verifying this is fixed?

Comment 17

[Dan Gohman [:sunfish]]

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

[u279076]

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

[Al Billings [:abillings - ex-MoCo]]

Dan, can we get ESR17 and ESR25 patches here since we're tracking it for those releases?

Comment 20

[Dan Gohman [:sunfish]]

Attachment: the patch, ported to the esr24 branch

Comment 21

[Dan Gohman [:sunfish]]

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

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

[Al Billings [:abillings - ex-MoCo]]

ESR17 and ESR24 patches approved. Let's get them checked in.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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

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

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

https://hg.mozilla.org/releases/mozilla-b2g18/rev/05db0f8744fa

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/e58ba4276d7b

https://tbpl.mozilla.org/php/getParsedLog.php?id=29257406&tree=Mozilla-B2g18

Comment 29

[Dan Gohman [:sunfish]]

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

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

Comment on attachment 818750 [details] [diff] [review]
the patch, ported to the b2g18 tree

Already had approval to land :)

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/53542d252c2d

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/53542d252c2d