Malloc error with ArrayBuffer, Uint32Array and Uint8Array

VERIFIED FIXED in Firefox 12

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: gkw, Assigned: dmandelin)

Tracking

(Blocks: 1 bug, {crash, testcase})

Trunk
mozilla14
x86
Mac OS X
crash, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox12+ fixed, firefox13+ fixed, firefox14+ fixed, firefox-esr1012+ fixed)

Details

(Whiteboard: [sg:critical][qa?])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Created attachment 606742 [details]
stack

try {
    a = ArrayBuffer(76);
    b = Uint32Array(a);
    uneval()
    c = Uint8Array(a);
    c.set(b)
} catch (e) {}

crashes js debug and opt shell on m-c changeset c7ef262e3024 with a malloc error:

Tested on 32-bit.

malloc: *** error for object 0xb74ae4: incorrect checksum for freed object - object was probably modified after being freed.
*** set a breakpoint in malloc_error_break to debug

s-s because malloc errors seem scary. A Valgrind log is attached.
(Reporter)

Comment 1

5 years ago
a = Int32Array(6)
Uint8Array(a.buffer).set(a)

is a testcase which causes a similar error - the testcase manifests as the following on Linux:

*** glibc detected *** ./js-opt-32-im-linux: free(): invalid next size (fast): 0x095e3a18 ***
======= Backtrace: =========
/lib32/libc.so.6(+0x6ff02)[0xf751cf02]
/lib32/libc.so.6(+0x70ba2)[0xf751dba2]
/lib32/libc.so.6(cfree+0x6d)[0xf7520c5d]
./js-opt-32-im-linux[0x81844d0]

/snip

Comment 2

5 years ago
autoBisect.py -p -a64 --valgrind t.js

Due to skipped revisions, the first bad revision could be any of:
changeset:   96f52c040d20
user:        Nikhil Marathe
date:        Mon Aug 01 15:26:54 2011 -0700
summary:     Bug 667047 - Ensure proper __proto__ behaviour as a normal property after setting it to null once. r=mrbkap

changeset:   794fe3408d3a
user:        Nikhil Marathe
date:        Mon Aug 01 15:49:51 2011 -0700
summary:     Bug 664249 - Inline TypedArray properties into slots. r=mrbkap
Blocks: 664249
Whiteboard: js-triage-needed → [sg:critical] js-triage-needed
status-firefox-esr10: --- → affected
Assigning to dmandelin to find a real assignee. This flaw has been in the code a while but was recently found when we changed our fuzzers to look for more things like the flaw that was used in the Pwn2Own contest. Others may be out there looking for things like this too (at the very least, Willem and Vincenzo might be).
Assignee: general → dmandelin
status-firefox12: --- → affected
status-firefox13: --- → affected
status-firefox14: --- → affected
tracking-firefox-esr10: --- → ?
tracking-firefox12: --- → ?
tracking-firefox13: --- → +
tracking-firefox14: --- → +
(Assignee)

Updated

5 years ago
tracking-firefox12: ? → +
Whiteboard: [sg:critical] js-triage-needed → [sg:critical]
(Assignee)

Comment 4

5 years ago
Created attachment 609492 [details] [diff] [review]
Patch

Great test case: it's a bad bug but very easy to spot with this test case + the memory corruption thing Visual Studio has.
Attachment #609492 - Flags: review?(sphink)
(Assignee)

Comment 5

5 years ago
Alex, can I get your advice on landing this? If you look at the patch you'll see that it's *very* easy to spot what's going on here. We have some typed array feature work scheduled to land in the next few weeks that might work as cover bugs if that would be useful. I don't think we could land those bugs on the ESR though.
Comment on attachment 609492 [details] [diff] [review]
Patch

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

::: js/src/jstypedarray.cpp
@@ +1955,5 @@
>          }
>  
>          // We have to make a copy of the source array here, since
>          // there's overlap, and we have to convert types.
> +        void *srcbuf = cx->malloc_(getByteLength(tarray));

Um, yeah. Your version is better.
Attachment #609492 - Flags: review?(sphink) → review+
(Assignee)

Comment 7

5 years ago
Thanks for the quick review. I guess now we need a cover bug to land it in. Steve, do you think we can land it in one of your typed array bugs? That would cover it pretty well, but we probably don't want to land those bugs on ESR. But I'm not sure how we do land things on ESR. Do we use cover bugs, or do we just land the security fixes directly because it's the last place they go?
Cover bugs are useful sometimes, if we have something plausible at hand. For ESR and non-trunk branches (e.g. Firefox 12) you don't want to land array feature changes and I can't imagine any bug you could write that would obscure what's going on here if what's actually checked in is a one-line patch.

Landing this safe patch right at the release freeze date for Firefox 12 and ESR will have to be good enough. It's a pretty straightforward heap buffer overrun and likely to be easy to incorporate into an exploit.
tracking-firefox-esr10: ? → 12+
[Triage Comment]

Please nominate the patch for ESR landing as per https://wiki.mozilla.org/Release_Management/ESR_Landing_Process
(Assignee)

Comment 10

5 years ago
Comment on attachment 609492 [details] [diff] [review]
Patch

[Approval Request Comment]
Regression caused by (bug #): probably 667047 or 664249 
User impact if declined: leaves in a buffer overflow vulnerability
Testing completed (on m-c, etc.): standard js regression test suite
Risk to taking this patch (and alternatives if risky): none, AFAICT
String changes made by this patch: none
Attachment #609492 - Flags: approval-mozilla-esr10?
Comment on attachment 609492 [details] [diff] [review]
Patch

(In reply to David Mandelin from comment #5)
> Alex, can I get your advice on landing this? If you look at the patch you'll
> see that it's *very* easy to spot what's going on here. We have some typed
> array feature work scheduled to land in the next few weeks that might work
> as cover bugs if that would be useful. I don't think we could land those
> bugs on the ESR though.

Let's move forward with landing this fix on Aurora/Beta/ESR before our code freeze date (4/13). Pre-approving for Aurora 13 and Beta 12, assuming this patch applies.
Attachment #609492 - Flags: approval-mozilla-esr10?
Attachment #609492 - Flags: approval-mozilla-esr10+
Attachment #609492 - Flags: approval-mozilla-beta+
Attachment #609492 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 12

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/8f783fc30dd3
http://hg.mozilla.org/releases/mozilla-esr10/rev/14c0e5f4ad2a
http://hg.mozilla.org/releases/mozilla-beta/rev/07381182a1e8
http://hg.mozilla.org/releases/mozilla-aurora/rev/863691ed9f15
status-firefox-esr10: affected → fixed
status-firefox12: affected → fixed
status-firefox13: affected → fixed
Target Milestone: --- → mozilla14
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
JSBugMon: This bug has been automatically verified fixed.
Status: RESOLVED → VERIFIED
Group: core-security

Updated

5 years ago
status-firefox14: affected → fixed
Could someone please point me to directions for how to create a 32-bit opt debug shell for verification?
Whiteboard: [sg:critical] → [sg:critical][qa?]
(Reporter)

Comment 15

5 years ago
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #14)
> Could someone please point me to directions for how to create a 32-bit opt
> debug shell for verification?

Which platform (Win/Lin/Mac 10.6/10.7)?
Mac 10.7, I guess.
(Reporter)

Comment 17

5 years ago
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #16)
> Mac 10.7, I guess.

I run something like:

LD=ld CROSS_COMPILE=1 CC="clang -Qunused-arguments -fcolor-diagnostics -arch i386" RANLIB=ranlib CXX="clang++ -Qunused-arguments -fcolor-diagnostics -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments -fcolor-diagnostics" HOST_CXX="clang++ -Qunused-arguments -fcolor-diagnostics" sh ./configure --target=i386-apple-darwin8.0.0 --enable-optimize --disable-debug

in my js/src directory. (I use Clang)

(I also removed parameters that you aren't likely to need, e.g. --enable-profiling --enable-methodjit --enable-type-inference --enable-more-deterministic --disable-tests --enable-valgrind )
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.