A user recently reported a reproducible crash with Tor Browser, which is based on the latest ESR 45 code, when visiting https://help.github.com/categories/writing-on-github/ (https://lists.torproject.org/pipermail/tor-talk/2017-March/043064.html). Tracking this back with our builds leads to the conclusion it got introduced with the switch from ESR 38 to ESR 45. This crash is reproducible on all Windows 7 machines we had the browser tested on but we found other Windows versions are fine (like Windows 2012 Server or Windows 2016 Server). We produced a reduced testcase (https://people.torproject.org/~boklm/bug21795/t5.html) which indicates that it is necessary to have a worker involved. Otherwise the browser is not crashing. It is still crashing after removing all of our custom patches leaving just the ones that are necessary for cross-compilation. This is the reason why I put this ticket here into the DOM: Workers category. Taking a 45.8.0esr build Mozilla ships does not trigger that crash. For cross-compiling Firefox with just the bare minimum of patches we used the following .mozconfig: CROSS_COMPILE=1 ac_add_options --enable-application=browser ac_add_options --target=i686-w64-mingw32 ac_add_options --enable-default-toolkit=cairo-windows mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-mingw mk_add_options MOZ_APP_DISPLAYNAME="Tor Browser" mk_add_options MOZILLA_OFFICIAL=1 mk_add_options BUILD_OFFICIAL=1 ac_add_options --disable-debug ac_add_options --enable-optimize ac_add_options --enable-strip ac_add_options --enable-official-branding ac_add_options --disable-updater ac_add_options --without-intl-api ac_add_options --disable-sandbox ac_add_options --disable-eme ac_add_options --disable-crashreporter ac_add_options --disable-maintenance-service ac_add_options --disable-webrtc ac_add_options --disable-tests We use GCC 5.1.0 and mingw-w64 commit a0cd5afeb60be3be0860e9a203314c10485bb9b8. The additional patches we used on top of 45.8.0esr were: https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-45.8.0esr-6.5-2&id=f54b28dc50354bb8a076a0012b63fba42df967f1 https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-45.8.0esr-6.5-2&id=84e97cbd25dcc406440529b8e482f94b5956c3c6 https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-45.8.0esr-6.5-2&id=999a2048e8f365b49fb0eb940ce708683009b8f6 https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-45.8.0esr-6.5-2&id=7856a64221b169dd9c4d2c8e75a4dd213d043484 https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-45.8.0esr-6.5-2&id=aebe95a1b1cfd9af5b309e4d25efbeac0a8e76a4 https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-45.8.0esr-6.5-2&id=ba28d792ed2c2dc81eb1391f9f06b327eff3fffd We use wrapper scripts for compilation to harden the browser on Windows: https://gitweb.torproject.org/builders/tor-browser-bundle.git/tree/gitian/build-helpers/i686-w64-mingw32-g++?h=maint-6.5 https://gitweb.torproject.org/builders/tor-browser-bundle.git/tree/gitian/build-helpers/i686-w64-mingw32-gcc?h=maint-6.5 https://gitweb.torproject.org/builders/tor-browser-bundle.git/tree/gitian/build-helpers/i686-w64-mingw32-ld?h=maint-6.5 Removing the SSP from those wrappers does not prevent the browser from crashing.

On Windows 7 I can reproduce with TB 6.5.1 and in WinDBG I get a Stack Overflow. But TB Release doesn't have any symbols or anything so that's all I get without staring at assembly. I also reproduced it in TB 7.0a2, but again, no symbols. 7.0a2: {{{ WARNING: Stack overflow detected. The unwound frames are extracted from outside normal stack bounds. eax=00000e5e ebx=000072f8 ecx=0871f400 edx=17f42d90 esi=ffffff88 edi=17acac40 eip=2cd18531 esp=06806da8 ebp=17b27b70 iopl=0 nv up ei pl nz na po nc }}} I tried a few things to get more info (like searching for the assembly instructions at eip on a different PC (same browser) in IDA - but no dice.)

If it is still crashing without your patches, and it doesn't crash in Firefox ESR45, it seems like it is some compiler issue, either like a bug or maybe it is doing some aggressive optimization that takes advantage of undefined behavior in our code.

Nathan, any ideas?

Is this reproducible on local builds? If so, could you provide a stack? Or even better, a minidump?

Er, a minidump from a local build wouldn't be useful unless you want to share out the PDBs as well, so nevermind. A stack would be a good starting point.

(In reply to Andrew McCreight [:mccr8] from comment #3) > Nathan, any ideas? Uh. I suppose TB doesn't have any sort of crash reporting infrastructure... You don't --enable-release, so this isn't a problem from ICF. Is the stack overflow on the main thread or somewhere else? If it was somewhere else, we could rule out JS. Do any of those extra compilation options, or particular things you use in your runtime, cause extra stuff to be stored on the thread stack at startup? Our multithreaded JS parsing cares a lot about how much stack space is available, and if there's a mismatch, parsing complex JS can easily overflow the stack: https://hg.mozilla.org/releases/mozilla-esr45/file/tip/js/src/vm/HelperThreads.cpp#l471 Maybe try increasing those constants? Just some out-loud thoughts, nothing concrete, sorry. :(

Thanks. No, we don't have such an infrastructure yet but we are thinking about setting one up. I am currently trying to get some sort of stacktrace. So far I only got corrupted stacks that were not that meaningful. :(

(In reply to Georg Koppen from comment #7) > Thanks. No, we don't have such an infrastructure yet but we are thinking > about setting one up. I am currently trying to get some sort of stacktrace. > So far I only got corrupted stacks that were not that meaningful. :( Out of curiosity, what makes you say the stacks are corrupted?

(In reply to Nathan Froyd [:froydnj] from comment #8) > (In reply to Georg Koppen from comment #7) > > Thanks. No, we don't have such an infrastructure yet but we are thinking > > about setting one up. I am currently trying to get some sort of stacktrace. > > So far I only got corrupted stacks that were not that meaningful. :( > > Out of curiosity, what makes you say the stacks are corrupted? Because I get [New Thread 2796.0xed0] warning: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurat ions if the document contains characters from outside the US-ASCII range. The ch aracter encoding of the page must be declared in the document or in the transfer protocol." {file: "https://people.torproject.org/~boklm/bug21795/t5.html" line: 0}] Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 2796.0xed0] 0x0e158534 in ?? () (gdb) bt #0 0x0e158534 in ?? () #1 0xffffff88 in ?? () #2 0x22c27180 in ?? () #3 0x1d315e60 in ?? () Backtrace stopped: previous frame inner to this frame (corrupt stack?) in GDB. I tried to make sure the frame pointer is not omitted in case that could cause such a result but that did not change things. The GDB version I have does not like all the symbols either it seems. It hangs if I don't strip the build. But it seems like Andrew's comment 2 might be a good point: if I compile with --disable-optimize the build does not crash. It seems as well that the issue is not on the main thread given the stack trace snippet above. Not sure what I could do to get a better stack trace. I guess one thing is trying a newer GDB. I can upload the build somewhere as well in case anyone else has ideas and want to look at it. One further data point: the crash does not happen with ESR52. So, I could try bisecting my way to the problematic code as well although I had some hope I could avoid that as this is usually a huge PITA.

The ?? could be JIT code. In your debugger, you can try running DumpJSStack().

I'm marking this sec-other because it does not affect Firefox per se.

(In reply to Andrew McCreight [:mccr8] from comment #10) > The ?? could be JIT code. In your debugger, you can try running > DumpJSStack(). That just gives me: "Cannot access memory at address 0x1fce74b4"

Okay, a newer GDB copes with the non-stripped build it seems. But still no change regarding the stack trace. :(

I don't speak GDB very well, but if I had this crash in front of me in WinDbg (hi tjr!), I'd do: - "!address @esp" (plus also on the next higher block) and see whether the allocation data looks reasonable, to try to differentiate between a once-sane stack pointer that simply grew too far, versus an altogether garbage one (maybe clobbered by a bad calling convention or something). - "dps @esp L10000" to dump symbolicated pointers until we get back down to known territory (assuming esp isn't complete garbage).

(In reply to David Major [:dmajor] from comment #14) > I don't speak GDB very well, but if I had this crash in front of me in > WinDbg (hi tjr!), I'd do: Hi! I was just prepping some work on this when I saw this. This is what I see in Visual Studio: http://i.imgur.com/95aiM3i.png The strange thing, is if I search for the instruction right before it, I can't find it in any .exe/.dll in the directory: binwalk -R "\x0f\x84\x1d\x00\x00\x00" * > - "!address @esp" (plus also on the next higher block) and see whether the > allocation data looks reasonable, to try to differentiate between a > once-sane stack pointer that simply grew too far, versus an altogether > garbage one (maybe clobbered by a bad calling convention or something). I don't think I can get to "the next higher block" (you mean stack frame?) .frame only shows one frame. > - "dps @esp L10000" to dump symbolicated pointers until we get back down to > known territory (assuming esp isn't complete garbage). Attached the output of !address @esp .frame dps @esp L10000

I just uploaded the build I've been working with today in case someone wants to skip all the Tor cruft and just look at Firefox code: https://people.torproject.org/~gk/testbuilds/firefox_45.8.0_github_crash.zip https://people.torproject.org/~gk/testbuilds/firefox_45.8.0_github_crash.zip.asc

In Tom's file - [0b1d0000, 0b2d0000) is the total area of the stack. Of that, [0b1d0000, 0b2c8000) is reserved but not yet commited. [0b2c8000, 0b2c9000) is the guard page. [0b2c9000, 0b2d0000) is a reasonable-looking stack. esp at the crash is 0b2c7418, so it looks like we had a sane stack until esp got decremented by some amount greater than one page, so we bypassed the guard page and the OS couldn't do fixup, and instead we landed in the uncommitted region.

> I was just prepping some work on this when I saw this. This is what I see > in Visual Studio: http://i.imgur.com/95aiM3i.png The strange thing, is if I > search for the instruction right before it, I can't find it in any .exe/.dll > in the directory: binwalk -R "\x0f\x84\x1d\x00\x00\x00" * I suspect we're in JIT code. Those FFFFFF88's look like some of the magic jsval constants.

I finally got the problematic bug via bisection: it is bug 1108290. rev 274223 is unaffected and rev 274226 is crashing (I can't build rev 274224 or 274225). I've uploaded respective builds in case anyone wants to test that out: https://people.torproject.org/~gk/testbuilds/firefox-274223.zip https://people.torproject.org/~gk/testbuilds/firefox-274223.zip.asc https://people.torproject.org/~gk/testbuilds/firefox-274226.zip https://people.torproject.org/~gk/testbuilds/firefox-274226.zip.asc Lars (or anybody else), any ideas what is going wrong here? If not, how could we debug this further?

There were several fixes posted over time to deal with weaknesses in that changeset. Changeset 280656 (bug 1231024) was a local fix to deal with a register allocation bug in the optimization itself. Changeset 318399 (bug 1309903) was a larger change to the register allocator and how it is used to make it more resilient; the problem is that on x86, Apply uses all available registers and it was pushing at the boundaries of the register allocator. None of those made it into 45, I think. Don't know why this should be crashing on x64, though, as it is not register-starved. You can test whether this optimization is the problem. In IonBuilder.cpp, look for code that looks roughly like this in jsop_funapply: if (native && native->isNative() && native->native() == fun_apply && objTypes && objTypes->getKnownClass(constraints()) == &ArrayObject::class_ && !objTypes->hasObjectFlags(constraints(), OBJECT_FLAG_LENGTH_OVERFLOW) && ElementAccessIsPacked(constraints(), argument)) { return jsop_funapplyarray(argc); } If you insert "false &&" as the first conjunct in that "if" then the optimization should be skipped entirely.

Lars: As I understand it, the problem is that we've decremented esp by such a large amount that it's gone outside the currently-committed stack region _and_ skipped over the guard page entirely, so the OS didn't get an opportunity to do an on-demand commit. In Tom's repro: > http://i.imgur.com/95aiM3i.png I can see `sub esp, ebx`, and ebx itself was recently `shl`d, so the delta might have been big enough to be a problem. I see that bug 1108290 made some changes so how we do `sub` instructions. That seems to me more likely than the fun_apply optimization -- unless they're one and the same and I'm misunderstanding something? Does the masm just need to know that esp can't be decremented by more than one page at a time?

Oh, nasty. The "apply" code is the only code in the JIT that uses variable subtract from SP (subFromStackPtr with a Register argument), apart from one use on ARM64 that is probably safe by construction (didn't look closely, the ARM64 JIT is not currently operational anyway). And the common implementation of subFromStackPtr(Register) just performs a simple subtract, which is wrong, at least on Windows. Nice find. Yes, subFromStackPtr(Register) needs to be taught about the guard pages, as the Imm32-taking variant does.

(In reply to Lars T Hansen [:lth] from comment #20) > There were several fixes posted over time to deal with weaknesses in that > changeset. Changeset 280656 (bug 1231024) was a local fix to deal with a > register allocation bug in the optimization itself. Changeset 318399 (bug > 1309903) was a larger change to the register allocator and how it is used to > make it more resilient; the problem is that on x86, Apply uses all available > registers and it was pushing at the boundaries of the register allocator. > None of those made it into 45, I think. > > Don't know why this should be crashing on x64, though, as it is not > register-starved. > > You can test whether this optimization is the problem. In IonBuilder.cpp, > look for code that looks roughly like this in jsop_funapply: > > if (native && native->isNative() && native->native() == fun_apply && > objTypes && > objTypes->getKnownClass(constraints()) == &ArrayObject::class_ && > !objTypes->hasObjectFlags(constraints(), > OBJECT_FLAG_LENGTH_OVERFLOW) && > ElementAccessIsPacked(constraints(), argument)) > { > return jsop_funapplyarray(argc); > } > > If you insert "false &&" as the first conjunct in that "if" then the > optimization should be skipped entirely. That seems to work around the problem. I tested several builds with and without the change you suggested and builds are only crashing if I don't apply it. (I hope that makes sense). Thanks. I guess we pick that change up for Tor Browser 6.5.2 but it seems to me this is kind of an issue that would be good having a fix for on esr45 in general as well. Do you know whether that kind of JIT code is covered by any preference we could flip? We set `javascript.options.ion.content`, `javascript.options.baselinejit.content`, and `javascript.options.typeinference` to `false` in our security slider (medium level) but the browser is still crashing then. That's a bit surprising as we were under the assumption that flipping those three prefs would be enough to avoid bugs in Firefox's JIT code.

I don't know anything about those preferences. I can't find any trace of .ion.content and .baselinejit.content in the current sources, and looking in modules/libpref/init/all.js for both esr38 and esr45 does not suggest that they exist. Are these local adaptations for Tor perhaps? Let's ask Jan if he knows anything about them. Even with the fix I suggested I think (I need to investigate more deeply but that was the impression I had from reading the source) you should be able to crash Firefox if you apply an arguments object that is large enough, but at least we're all agreed on the problem.

(In reply to Lars T Hansen [:lth] from comment #24) > I don't know anything about those preferences. I can't find any trace of > .ion.content and .baselinejit.content in the current sources, and looking in > modules/libpref/init/all.js for both esr38 and esr45 does not suggest that > they exist. Are these local adaptations for Tor perhaps? Let's ask Jan if > he knows anything about them. Ugh yes, we used to have separate chrome and content JIT prefs, but that changed when we moved these flags to the runtime (bug 939562). Georg, you want to flip the following prefs: javascript.options.ion javascript.options.baselinejit I'd also suggest: javascript.options.native_regexp (regex JIT, it uses the regex interpreter if false) javascript.options.asmjs

The CodeGenerator actually guards against "too many" arguments (see CodeGenerator::visitApplyArrayGeneric) but this only considers the configurable limit maxStackArgs, which is not related to guard pages and so on. As a quick workaround we could in principle insert a fixed, small limit there in Windows builds. However, the code for applying the arguments object (see CodeGenerator::visitApplyArgsGeneric) does not have that guard and likely has the same bug. Also, these are just hacks. The problem with fixing subFromStackPtr(Register) properly is that it needs at least one temp, and we are already highly starved for registers in the situation where we need that temp. But it is certainly the way to go for mozilla-central. For ESR 52 a backport might be reasonable. For ESR 45 I don't know yet; likely we should disable the apply(Array) case there given all the bug fixes that have come in that we aren't going to backport, but we may wish to try to fix subFromStackPtr(Register) anyway. All that said, I have the sense that this stack overflow is not actually an exploitable crash, it is just a crash. If it weren't crashing on something as high profile as github we might let it slide.

Inserting a stack probe at the beginning of emitAllocateSpaceForApply is plausible, we have a spare register there and can push/pop a value to free up another. Then we don't have to worry about probing when we actually subtract from the SP, I hope. The following sketch is crude and needs testing: #ifdef XP_WIN Label probeLoop; masm.push(argcreg); masm.lshiftPtr(Imm32(ValueShift), argcreg); masm.negq(argcreg); // FIXME masm.addPtr(Assembler::getStackPointer(), argcreg); // Lower limit, roughly masm.movePtr(Assembler::getStackPointer(), extraStackSpace); masm.bind(&probeLoop); masm.subPtr(ImmWord(4096), extraStackSpace); masm.or32(Imm32(0), Address(extraStackSpace, 0)); masm.branchPtr(Assembler::AboveOrEqual, extraStackSpace, argcreg, &probeLoop); masm.pop(argcreg); #endif

(In reply to Lars T Hansen [:lth] from comment #26) > The CodeGenerator actually guards against "too many" arguments (see > CodeGenerator::visitApplyArrayGeneric) but this only considers the > configurable limit maxStackArgs, which is not related to guard pages and so > on. As a quick workaround we could in principle insert a fixed, small limit > there in Windows builds. However, the code for applying the arguments > object (see CodeGenerator::visitApplyArgsGeneric) does not have that guard > and likely has the same bug. Also, these are just hacks. FWIW I think the Baseline ICs for this have similar limits. I don't think it's too bad to be conservative by optimizing only the sane-argc cases.

What do you think of this as a PoC? If you agree, I also think I ought to clean up the apply(arguments) case, and then I need to get somebody who can repro the bug to test it on Windows. (The code is for m-c, may need additional backport for esr.)

Comment on attachment 8855758 [details] [diff] [review] bug1351278-dont-apply-too-much.patch Review of attachment 8855758 [details] [diff] [review]: ----------------------------------------------------------------- The bailout is not something we can easily avoid with an OOL VM call? This is probably fine though, definitely the simplest fix to backport. I'd prefer using the more conservative limit on all platforms, so we don't get Windows-only (performance) bug reports we can't repro.

(This still needs testing by somebody who can repro the original problem.) Pretty local fix. The test case / benchmark included clearly shows the cost of bailout but the cutoff is large enough (at 375) that it's probably not a real hardship. Some programs will no doubt regress.

Flagging GK for review also.

While I not feel I am a good reviewer of the code itself I can try to test whether the patch fixes the problem (for us). Contrarily to what I said in comment 9 I seem to have found a way to get ESR52-based Tor Browsers crashed and the crash looks like the one we are dealing with in this bug. Lars, do you think you could give me an ESR52 version of your patch as this would make it much easier for me to verify that it fixes the problem. Or do you need a confirmation for m-c first (not sure if I get that one crashed with the testcase I have, though)?

Georg, I will prep a patch suitable for ESR52 and ping you, probably won't be until Tuesday though.

Georg, the patch that's already attached to this bug applies cleanly to ESR52 (just checked), and you should be able to test with that.

Comment on attachment 8857437 [details] [diff] [review] bug1351278-dont-apply-too-much-v2.patch [Security approval request comment] How easily could an exploit be constructed based on the patch? Probably hard: Not obvious that an exploit is even possible, this may just be a crasher. In the limit it may be possible to exploit it if the heap is right up against the stack but ASLR and other mechanisms tend to prevent that. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The test cases and commit message more or less explain what's going on. Which older supported branches are affected by this flaw? All branches back to ESR45. If not all supported branches, which bug introduced the flaw? Arguably bug 1108290 (Nov 24, 2015) though there have been follow-ups for that. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The current patch applies cleanly to ESR52 and should apply to FF53 and newer. It looks like it might also apply to ESR45 but I have not tried. How likely is this patch to cause regressions; how much testing does it need? It is likely to cause some performance regressions in programs that apply() many arguments (more than 375) but no functional regressions. We have OK test coverage with what's in the repo already and with what's included in this bug.

We're shipping Fx53 and ESR 45.9 today (our final planned release off that branch), so unless this becomes a chemspill-worthy issue, we won't be fixing this for those releases.

This is a sec-other rated bug. Why is sec-approval, which only applies to high and critical rated issues, being asked for here?

abillings, probably just because I don't know the drill properly.

Comment on attachment 8857437 [details] [diff] [review] bug1351278-dont-apply-too-much-v2.patch Cancelling request for sec-approval per abillings msg (above).

Track 54+/55+ as security issue.

This was a huge PITA, sorry for the delay. It turns our while I had a build based on a recent commit on the ESR52 branch that crashed, building again without the workaround mentioned in comment:20 does not crash the resulting browser. I tried different things but no dice. So, I looked at the patch and created a version for ESR45 which is attached. I reverted the above mentioned workaround and the build crashed. Then I applied the ESR45 patch and the build does not crash anymore. Thus, I think the patch does indeed fix the bug.

https://hg.mozilla.org/integration/mozilla-inbound/rev/288f9c879d41b61c90f16af0e002d6331df921bd Bug 1351278 - Avoid stepping over Windows stack guard page, r=jandem

Please request Beta/ESR52 approval on this when you're comfortable doing so.

Comment on attachment 8857437 [details] [diff] [review] bug1351278-dont-apply-too-much-v2.patch Approval Request Comment [Feature/Bug causing the regression]: An optimization of the Function.prototype.apply operator in JS [User impact if declined]: Possible to create DOS (crash) situations on Windows, exploit potential unclear. [Is this code covered by automated tests?]: Yes, included in patch. [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?]: Not really. [Why is the change risky/not risky?]: Local patch, well understood code, we're really just tightening a parameter. [String changes made/needed]: None.

Comment on attachment 8857437 [details] [diff] [review] bug1351278-dont-apply-too-much-v2.patch [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: Problem was discovered by the Tor team on the ESR branch, good to fix it. User impact if declined: Possible DOS crashes on Windows, exploit potential unclear, probably low. Fix Landed on Version: 55 Risk to taking this patch (and alternatives if risky): Low risk. String or UUID changes made by this patch: None. See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment on attachment 8857437 [details] [diff] [review] bug1351278-dont-apply-too-much-v2.patch Fix a security issue. Beta54+. Should be in 54 beta 3.

(In reply to Lars T Hansen [:lth] from comment #46) > [Is this code covered by automated tests?]: > Yes, included in patch. > [Has the fix been verified in Nightly?]: > Yes. > [Needs manual test from QE? If yes, steps to reproduce]: > No. Setting qe-verify- based on Lars' assessment on manual testing needs and the fact that this fix has automated coverage.

Comment on attachment 8857437 [details] [diff] [review] bug1351278-dont-apply-too-much-v2.patch We uplifted this to Beta54, let's keep the ESR52.2 in sync.