The following testcase crashes on mozilla-central revision c16bc8097c10 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe): g = newGlobal(); g.parent = this; g.eval("new Debugger(parent).onExceptionUnwind = function () {}"); function pushLimits(limit, offset) { function pusher() { Array.prototype.push.apply(arr, arguments) } var arr = [0, 1, 2, 3, 4, 5, 6, 7]; oomAtAllocation(limit); for (var i = 0; i < 100; i++) pusher(0, 1, 2, 3, 4, 5, 6, 7); } var limit = 1024 * 1024 * 1024; while (true) { var res = pushLimits(limit, 0); limit = (limit / 1.5) | 0; } Backtrace: received signal SIGSEGV, Segmentation fault. #0 0x0000000000efacf5 in InitFromBailout (caller=..., ionScript=<optimized out>, excInfo=0x7fffffffc0e0, callPC=<synthetic pointer>, nextCallee=..., startFrameFormals=..., builder=..., invalidate=true, iter=..., script=..., fun=..., callerPC=<optimized out>, cx=0x7ffff6948000) at js/src/jit/BaselineBailouts.cpp:1011 #1 js::jit::BailoutIonToBaseline (cx=cx@entry=0x7ffff6948000, activation=0x7fffffffc930, iter=..., invalidate=invalidate@entry=true, bailoutInfo=bailoutInfo@entry=0x7fffffffbbe8, excInfo=excInfo@entry=0x7fffffffc0e0) at js/src/jit/BaselineBailouts.cpp:1656 #2 0x0000000000efcb51 in js::jit::ExceptionHandlerBailout (cx=cx@entry=0x7ffff6948000, frame=..., rfe=rfe@entry=0x7fffffffc62c, excInfo=..., overrecursed=overrecursed@entry=0x7fffffffbf6f) at js/src/jit/Bailouts.cpp:218 #3 0x000000000077e4e1 in js::jit::HandleExceptionIon (overrecursed=0x7fffffffbf6f, rfe=0x7fffffffc62c, frame=..., cx=0x7ffff6948000) at js/src/jit/JitFrames.cpp:204 #4 js::jit::HandleException (rfe=<optimized out>) at js/src/jit/JitFrames.cpp:673 #5 0x000010cc1f39f676 in ?? () #6 0x0000000000000000 in ?? () rax 0x0 0 rbx 0x90 144 rcx 0x7ffff6c28a2d 140737333332525 rdx 0x0 0 rsi 0x7ffff6ef7770 140737336276848 rdi 0x7ffff6ef6540 140737336272192 rbp 0x7fffffffbb90 140737488337808 rsp 0x7fffffffb690 140737488336528 r8 0x7ffff6ef7770 140737336276848 r9 0x7ffff7fe4740 140737354024768 r10 0x58 88 r11 0x7ffff6b9f750 140737332770640 r12 0xa 10 r13 0x7fffffffb890 140737488337040 r14 0x7fffffffc0e0 140737488339168 r15 0x7ffff6946180 140737330307456 rip 0xefacf5 <js::jit::BailoutIonToBaseline(JSContext*, js::jit::JitActivation*, js::jit::JSJitFrameIter const&, bool, js::jit::BaselineBailoutInfo**, js::jit::ExceptionBailoutInfo const*)+11861> => 0xefacf5 <js::jit::BailoutIonToBaseline(JSContext*, js::jit::JitActivation*, js::jit::JSJitFrameIter const&, bool, js::jit::BaselineBailoutInfo**, js::jit::ExceptionBailoutInfo const*)+11861>: movl $0x0,0x0 0xefad00 <js::jit::BailoutIonToBaseline(JSContext*, js::jit::JitActivation*, js::jit::JSJitFrameIter const&, bool, js::jit::BaselineBailoutInfo**, js::jit::ExceptionBailoutInfo const*)+11872>: ud2 I'm marking this s-s until investigated because the assertion sounds quite dangerous. The test uses Debugger, so it is at most sec-moderate if the use of the Debugger is required. But I could imagine that there might be other ways to cause this bailout, so I'll leave this to the JS devs to decide.

JSBugMon: Bisection requested, result: autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: https://hg.mozilla.org/mozilla-central/rev/8b1881ead0b6 user: Nicolas B. Pierron date: Thu Sep 07 13:01:13 2017 +0000 summary: Bug 966743 - Inline Array.prototype.push with more than one argument. r=jandem This iteration took 267.413 seconds to run.

Calling it sec-high out of caution because probable regressor had nothing to do with the Debugger code, and jorendorff was worried enough to make this a P1.

I am able to reproduce this issue, I will investigate it more on Monday.

(In reply to Christian Holler (:decoder) from comment #0) > function pusher() { > Array.prototype.push.apply(arr, arguments) > } The problem is here, and suppose that we OOM or freeze of the array "arr", which cause the array.push to fail and bailout, then we would be restoring twice the set of arguments, as part of the bailout frame. I am not completely sure of the security implication of this modification, but I am not confident that the Debugger is mandatory for creating a similar issue.

This patch would be good if we need to backport a fix, and if I cannot find a better solution in the mean time. This restores the behaviour that we had before Bug 966743.

It's too late for 57, wontfix.

This adds the fuzzer's test case in addition to a copy of the existing fun.apply test case for fun.call, just to ensure that we can bailout correctly as well with fun.call.

This patch rename pushFormals to pushCallStack and pushPriorCallStack. - pushCallStack is used for filling the content of the Outer resume points. This content is expected to explicitly list of the arguments, such that the call-frame can be emulated in Baseline. - pushPriorCallStack is used for filling the content of the stack for At resume points. This content is expected to be used for resuming at the call instruction, with enough content to resume the execution properly. The problem seen here happens when we reconstruct the baseline frame under HandleException (when resizing the array?). From this point of view I would not think that this issue is restricted to the Debugger. The problem seen here, is that the resume point contained the full set of arguments instead of the state of the stack prior JSOP_FUNAPPLY. Thus, the frame recovery which attempted to reconstruct the stack certainly gets it wrong. This patch adds machinery to capture the stack, if needed, such that it can be restored properly as-is. It defaults to restoring the set of call-stack definition if no state were captured. Thus JSOP_FUNCALL get one argument less when restored (could that cause Debugger issues?). This should solve the issue, as we restore a proper stack where the execution can be resumed, and satisfies the bailout assertion raised under HandleException.

Comment on attachment 8928198 [details] [diff] [review] Update test cases to cover all code paths. Review of attachment 8928198 [details] [diff] [review]: ----------------------------------------------------------------- We will land the tests once this is in release, right?

Comment on attachment 8928206 [details] [diff] [review] Distinguish call stack used for outer resume points than call stacks used for resuming at one instruction. Review of attachment 8928206 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay; I thought this was more complicated but the patch makes sense. ::: js/src/jit/IonBuilder.h @@ +1255,5 @@ > } > > + // Before doing any pop to the stack, capture whatever flows into the > + // instruction, such that we can restore it later. > + AbortReasonOr<Ok> savePriorCallStack(MIRGenerator* mir, MBasicBlock* current, size_t peekDepth) Nit: we should define this in IonBuilder.cpp since it's pretty big and inlining it doesn't matter (and the caller is in IonBuilder.cpp too). @@ +1261,5 @@ > + MOZ_ASSERT(priorArgs_.empty()); > + if (!priorArgs_.reserve(peekDepth)) > + return mir->abort(AbortReason::Alloc); > + while (peekDepth) { > + priorArgs_.infallibleAppend(current->peek(0 - peekDepth)); Nit: int32_t(peekDepth) to make the unsigned-to-signed conversion explicit and (maybe) to keep some compilers from complaining.

Comment on attachment 8928206 [details] [diff] [review] Distinguish call stack used for outer resume points than call stacks used for resuming at one instruction. > [Security approval request comment] > How easily could an exploit be constructed based on the patch? I tried to do it without the Debugger, and failed to make something crash. Still, I think this is possible, as this MArrayPush is made to bailout, and resume its execution in Baseline, thus resuming with a corrupted JS stack, and failing again. > Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Yes, based on the patch it is obvious that the way the stack is recovered on a bailout is different in case of MArrayPush with multiple arguments. > Which older supported branches are affected by this flaw? Firefox 57, and newer. > If not all supported branches, which bug introduced the flaw? Bug 966743 > Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The same patch should apply cleanly. > How likely is this patch to cause regressions; how much testing does it need? Not likely. The test added in the previous patch, and the existing test should be enough.

Comment on attachment 8928206 [details] [diff] [review] Distinguish call stack used for outer resume points than call stacks used for resuming at one instruction. sec-approval+ for trunk. If you nominate it for beta, I can approve it as well so we fix it everywhere affected.

Comment on attachment 8928206 [details] [diff] [review] Distinguish call stack used for outer resume points than call stacks used for resuming at one instruction. Landed, with fixup patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/6596b8c168f5 https://hg.mozilla.org/integration/mozilla-inbound/rev/43ccb7de1afe

https://hg.mozilla.org/mozilla-central/rev/43ccb7de1afe https://hg.mozilla.org/mozilla-central/rev/6596b8c168f5

JSBugMon: This bug has been automatically verified fixed.

This needs rebasing for Beta uplift. Please post a rebased patch and nominate it for approval when you get a chance.

This is the beta backport patch.

Comment on attachment 8936854 [details] [diff] [review] Distinguish between call stack used for outer resume points from call stacks used for resuming at one instruction. Approval Request Comment [Feature/Bug causing the regression]: Bug 966743 [User impact if declined]: Potential crash, and misuse of pointers. [Is this code covered by automated tests?]: (new & old test cases works) [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: no. [List of other uplifts needed for the feature/fix]: none. [Is the change risky?]: no. [Why is the change risky/not risky?]: this patch mostly rename a function, and changes the behaviour of one to match the expected resume state. [String changes made/needed]: none.

Comment on attachment 8936854 [details] [diff] [review] Distinguish between call stack used for outer resume points from call stacks used for resuming at one instruction. Sec-high, Beta58+

JSBugMon: This bug has been automatically verified fixed on Fx58