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.

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?

Note that this can be fairly intermittent.

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).

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

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

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

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.

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

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

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

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

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.

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

Yes, happy to take this in esr45 as well.

Comment on attachment 8799821 [details] [diff] [review] Patch sec-approval+ for trunk. Please make branch patches as well.

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.

(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 on attachment 8799821 [details] [diff] [review] Patch Sec-high, Aurora51+, Beta50+, ESR45+

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

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.