Bug 916580 (CVE-2013-5595)

Use of uninitialized memory, and buffer size computations not checked for overflow

RESOLVED FIXED in Firefox 25, Firefox OS v1.1hd

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: sunfish, Assigned: sunfish)

Tracking

({csectype-uninitialized, sec-moderate})

unspecified
mozilla27
csectype-uninitialized, sec-moderate
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox24 wontfix, firefox25+ fixed, firefox26+ fixed, firefox27+ fixed, firefox-esr1725+ fixed, firefox-esr2425+ fixed, b2g1825+ fixed, b2g-v1.1hd fixed, b2g-v1.2 fixed)

Details

(Whiteboard: [qa-][adv-main25+][adv-esr1710+][adv-esr24-1+])

Attachments

(6 attachments, 4 obsolete attachments)

(Assignee)

Description

4 years ago
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.
Attachment #805013 - Flags: review?(mh+mozilla)
Attachment #805013 - Flags: review?(mh+mozilla) → review?(luke)
(Assignee)

Comment 1

4 years ago
Created attachment 805087 [details] [diff] [review]
calloc-fixes.patch

This patch corrects an error in the earlier patch.
Attachment #805013 - Attachment is obsolete: true
Attachment #805013 - Flags: review?(luke)
Attachment #805087 - Flags: review?(luke)

Comment 2

4 years ago
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)?
Attachment #805087 - Flags: review?(luke) → review+
(Assignee)

Comment 3

4 years ago
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.
Attachment #805087 - Attachment is obsolete: true
(Assignee)

Comment 4

4 years ago
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.
Attachment #805388 - Flags: sec-approval?
Can someone suggest a security rating?

This looks to be necessary on all branches except for maybe ESR17?
status-b2g18: --- → ?
status-firefox24: --- → wontfix
status-firefox25: --- → affected
status-firefox26: --- → affected
status-firefox27: --- → affected
status-firefox-esr17: --- → ?
tracking-firefox27: --- → +
status-b2g18: ? → affected
status-firefox-esr17: ? → affected
tracking-b2g18: --- → ?
tracking-firefox25: --- → +
tracking-firefox26: --- → +
tracking-firefox-esr17: --- → 25+
Keywords: csec-uninitialized, sec-moderate
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.
Attachment #805388 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 7

4 years ago
Created attachment 808896 [details] [diff] [review]
the patch, ported to the beta branch
(Assignee)

Comment 8

4 years ago
Created attachment 808897 [details] [diff] [review]
the patch, ported to the aurora branch
(Assignee)

Comment 9

4 years ago
Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9ae7613751c
https://hg.mozilla.org/mozilla-central/rev/f9ae7613751c
Ready for uplift to Aurora/Beta if others are. Please nominate before Thursday to ensure this makes it into FF25.
(Assignee)

Comment 12

4 years ago
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.
Attachment #808897 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 13

4 years ago
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.
Attachment #808896 - Flags: approval-mozilla-beta?

Updated

4 years ago
Attachment #808896 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

4 years ago
Attachment #808897 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox25: affected → fixed
status-firefox26: affected → fixed
status-firefox27: affected → fixed
status-firefox-esr24: --- → affected
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
tracking-firefox-esr24: --- → 25+
(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.
Dan, is there any information you can provide to guide QA in verifying this is fixed?
Flags: needinfo?(sunfish)
(Assignee)

Comment 17

4 years ago
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.
Flags: needinfo?(sunfish)
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.
Whiteboard: [qa-]
Dan, can we get ESR17 and ESR25 patches here since we're tracking it for those releases?
Flags: needinfo?(sunfish)
Whiteboard: [qa-] → [qa-][adv-main25+]
(Assignee)

Comment 20

4 years ago
Created attachment 817556 [details] [diff] [review]
the patch, ported to the esr24 branch
(Assignee)

Comment 21

4 years ago
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.
Flags: needinfo?(sunfish)
(Assignee)

Comment 22

4 years ago
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.
Attachment #817556 - Attachment is obsolete: true
Attachment #817559 - Flags: approval-mozilla-esr17+
Attachment #817563 - Flags: approval-mozilla-esr24+
ESR17 and ESR24 patches approved. Let's get them checked in.
Whiteboard: [qa-][adv-main25+]
Whiteboard: [qa-][adv-main25+]
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.
status-b2g-v1.1hd: --- → affected
status-b2g-v1.2: --- → fixed
status-firefox-esr17: affected → fixed
status-firefox-esr24: affected → fixed
(Assignee)

Comment 25

4 years ago
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.
Whiteboard: [qa-][adv-main25+] → [qa-][adv-main25+][adv-esr1710+][adv-esr24-1+]

Updated

4 years ago
tracking-b2g18: ? → 25+
(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.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/05db0f8744fa
status-b2g18: affected → fixed
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
status-b2g18: fixed → affected
(Assignee)

Comment 29

4 years ago
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.
Attachment #818037 - Attachment is obsolete: true
(Assignee)

Comment 30

4 years ago
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.
Attachment #818750 - Flags: approval-mozilla-b2g18?
Comment on attachment 818750 [details] [diff] [review]
the patch, ported to the b2g18 tree

Already had approval to land :)
Attachment #818750 - Flags: approval-mozilla-b2g18?
https://hg.mozilla.org/releases/mozilla-b2g18/rev/53542d252c2d
status-b2g18: affected → fixed
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/53542d252c2d
status-b2g-v1.1hd: affected → fixed
Alias: CVE-2013-5595
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Group: core-security
You need to log in before you can comment on or make changes to this bug.