1308346 - Crash [@ __pthread_kill] with [@ free] on the stack

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

The following testcase crashes on mozilla-central revision da986c9f1f72 (build with --enable-debug --enable-more-deterministic --32, run with --fuzzing-safe --no-threads --ion-eager --no-ggc):

function h(f, m) {
    for (var j = 0; j < 9; ++j) {
        f(m[j]);
    }
}
function g() {
    new new Function("arguments.callee.arguments")(1);
}
h(g, []);


Backtrace:

0   libsystem_kernel.dylib        	0x9de41572 __pthread_kill + 10
1   libsystem_pthread.dylib       	0x964c8654 pthread_kill + 101
2   libsystem_c.dylib             	0x96cd6c38 abort + 156
3   libsystem_malloc.dylib        	0x92ddcf31 szone_error + 457
4   libsystem_malloc.dylib        	0x92ddcf65 free_list_checksum_botch + 50
5   libsystem_malloc.dylib        	0x92dd4328 tiny_free_list_remove_ptr + 123
6   libsystem_malloc.dylib        	0x92dd2c11 szone_free_definite_size + 1682
7   libsystem_malloc.dylib        	0x92dd220e free + 301
8   js-dbg-32-dm-clang-darwin-da986c9f1f72	0x00364953 js::jit::IonCannon(JSContext*, js::RunState&) + 995 (Ion.cpp:378)
/snip

For detailed crash information, see attachment.

Setting s-s because free is on the stack, may involve some form of memory error.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Detailed Crash Information

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/c97757afd190
user:        Jan de Mooij
date:        Sat Jun 11 15:01:22 2016 +0200
summary:     Bug 1272598 part 4 - Remove script and dataBytes from ArgumentsData. r=luke

Jan, is bug 1272598 a likely regressor?

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Note that this can be fairly intermittent.

Comment 4

[Jan de Mooij [:jandem]]

Gary, I'm unable to reproduce this on OS X with the revision and flags in comment 0, after trying at least 30 times. Do you have a different testcase, maybe a less reduced one?

If not I can try this with rr on Linux (next week, when I'm home).

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Try with a configuration similar to:

# Full configuration command with needed environment variables is:
# LD=ld CROSS_COMPILE=1 CC="clang -Qunused-arguments -msse2 -mfpmath=sse -arch i386" RANLIB=ranlib CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments -msse2 -mfpmath=sse" AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 HOST_CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse" sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=i386-apple-darwin14.5.0 --disable-jemalloc --enable-debug --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I also saved a local coredump (150MB) that you can take a look should that be necessary, let me know.

Comment 7

[Jan de Mooij [:jandem]]

OK I can reproduce this now. (--disable-jemalloc probably did the trick as malloc is reporting a UAF or so on stderr.) Investigating...

Comment 8

[Jan de Mooij [:jandem]]

OK so InlineFrameIterator::readFrameArgsAndLocals is calling argOp for new.target (if we're constructing), resulting in us overflowing the space we allocated for the arguments data. Treating new.target like an argument here seems like a footgun. Bug 1272598 probably exposed this but the actual bug predates that (maybe it's from bug 1141865).

Good find, Gary, this is pretty bad.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Patch

This fixes the issue in comment 8. readFrameArgsAndLocals now returns new.target separately (like |this|), instead of calling ArgOp for it. I also had to change RematerializedFrame to stop storing new.target in slots_.

Comment 10

[Eric Faust [:efaust]]

Comment on attachment 8799821 [details] [diff] [review]
Patch

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

::: js/src/jit/JitFrameIterator.h
@@ +717,5 @@
>              s.skip();
>  
> +        if (newTarget) {
> +            // For now, only support reading new.target when we are reading
> +            // overflown arguments.

This is pretty scary. Are all usecases OK with this?

Comment 11

[Marcia Knous [:marcia]]

Tracking 52+ for this issue = Comment 8 mentions the severity.

Comment 12

[Jan de Mooij [:jandem]]

(In reply to Eric Faust [:efaust] from comment #10)
> This is pretty scary. Are all usecases OK with this?

Yeah, ReadFrame_Formals is not used anywhere, so it seemed easiest to add this assert (actually it seems we only need ReadFrame_Actuals, as ReadFrame_Overflown is only used for some debugging code that could be simplified...).

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8799821 [details] [diff] [review]
Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
This patch lets an attacker write an ObjectValue (8 bytes) out of bounds. It's probably not trivial to exploit, but it could be used to corrupt memory maybe.

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

> Which older supported branches are affected by this flaw?
All.

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

> How likely is this patch to cause regressions; how much testing does it need?
Not very likely, a few Nightlies + fuzzing during that time should be sufficient.

Comment 14

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #13)
> This patch lets an attacker write an ObjectValue (8 bytes) out of bounds.

Er, this *bug*, obviously...

Comment 15

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

I need release management to weigh in here on whether we could take this on beta and/or ESR45 before I give a sec-approval. We'd want to take this on branches immediately after taking this on trunk.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Al Billings [:abillings] from comment #15)
> I need release management to weigh in here on whether we could take this on
> beta and/or ESR45 before I give a sec-approval. We'd want to take this on
> branches immediately after taking this on trunk.

This is a sec-high and this meets the Beta/ESR bar. It would be nice if it can land today/early tomm so it gets included in 50.0b9 build.

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Yes, happy to take this in esr45 as well.

Comment 18

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

Comment on attachment 8799821 [details] [diff] [review]
Patch

sec-approval+ for trunk. Please make branch patches as well.

Comment 19

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3428128df1170d4f9a0148aabe2a6f717bf43908

Comment 20

[Jan de Mooij [:jandem]]

Comment on attachment 8799821 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: ES6 classes, enabled in 45.
[User impact if declined]: Sec issues, crashes.
[Describe test coverage new/current, TreeHerder]: Fixes the fuzz test.
[Risks and why]: Low risk.
[String/UUID change made/needed]: None.

Comment 21

[Jan de Mooij [:jandem]]

Attachment: Patch for beta

Comment 22

[Jan de Mooij [:jandem]]

Attachment: Patch for esr45

Comment 23

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #20)
> [Feature/regressing bug #]: ES6 classes, enabled in 45.

Actually, this is a regression from new.target, it landed before 45.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3428128df117

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8799821 [details] [diff] [review]
Patch

Sec-high, Aurora51+, Beta50+, ESR45+

Comment 26

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

I'm hitting conflicts trying to apply the beta patch.

Comment 27

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/bf9235ab0289

Comment 28

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1796b06d333cd97b7aec26d41dca1d5d2b348f78

Sorry about that. This file ~never changes, and now two patches touched the same lines. Grmbl.

Comment 29

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/7006b275b8290cbf02b57d1f98e9d33857ccb9f3