Crash on Windows 11 24H2 due to stack-based buffer overrun

[Avamander]

Describe the bug
I stumbled upon STM32 build process using busybox64.exe to run its build scripts, which executes BusyBox's uname. That resulted in a bunch of exceptions and a "Critical event" being logged. A similar crash seems to happen when less is invoked.

I downloaded the latest 64-bit (unicode-enabled) version (1.38.0-FRP-5579-g5749feb35) just to be sure the issue hasn't yet been fixed.

The crash seems to occur at busybox+0x3f2ae :

onecore\net\netprofiles\service\src\nsp\dll\namespaceserviceprovider.cpp(616)\nlansp_c.dll!00007FFD7F54653A: (caller: 00007FFDA507205C) LogHr(1) tid(a160) 8007277C No such service is known. The service cannot be found in the specified name space.
onecore\net\netprofiles\service\src\nsp\dll\namespaceserviceprovider.cpp(616)\nlansp_c.dll!00007FFD7F54653A: (caller: 00007FFDA507205C) LogHr(2) tid(a160) 8007277C No such service is known. The service cannot be found in the specified name space.
(a3a8.a160): Security check failure or stack buffer overrun - code c0000409 (!!! second chance !!!)

The following text is also visible after the crash "The system detected an overrun of a stack-based buffer in this application. This overrun could potentially allow a malicious user to gain control of this application."

After searching for other similar issues, I did stumble upon Git for Windows struggling with Mandatory ASLR, but force-disabling that had no effect. Unfortunately I don't know how to debug this further to see what exactly Windows doesn't like.

Looking at threads online, the errors before the crash are because of the GetHostByName and GetHostName W11 deprecation: https://stackoverflow.com/a/70978244/4636860 (And 00007FFD7F54653A was busybox+0x6e47e, which matches a gethostname call in the disassembly. But this does not seem to be the cause of the crash.)

To Reproduce
Run the following script:

#!/bin/sh -
echo 'a' | less

[rmyorston]

I'm unable to reproduce the problem on my Windows 11 24H2 test system. Both uname and less work as expected.

[Avamander]

Hmm, that would mean there's some kind of a configuration difference. The only thing I can think of right now is something like kernel-mode hardware-enforced stack protection (and related HVCI features) being enabled on my machine, as my hardware is rather new. But I can't disable it to test that theory.

[rmyorston]

I've tried enabling HVCI (Settings -> Privacy & security -> Windows Security -> Device security -> Core isolation details -> Memory integrity toggle to On, followed by a reboot).

System Information shows that HCVI services are running but it makes no difference (other than slowing the machine down). I don't get any reports of stack overrun.

[Avamander]

I think I found the cause, it's Control Flow Guard (which requires CPU support), force-disabling it seems to improve things. So I guess either somehow it control flow integrity does get violated or the build tools do not support CFG properly. I will also note that Memory Integrity does not enable Shadow Stack (and it too depends on hardware support).

[rmyorston]

Thanks for the hint. I can reproduce the issue by enabling Control Flow Guard.

I'll investigate...

[rmyorston]

Here's the current state of my investigation.

The problems you're seeing appear to be related to longjmp(3) calls. I suspect the message implicating a stack overrun is just a catch-all which covers the weird stack manipulations longjmp(3) has to perform.

setjmp(3)/longjmp(3) are used for exception handling in a number of places in BusyBox:

• Some lightweight applets can be invoked without forking a new process. You can see which by running busybox --list-all | grep NOFORK. Note that uname is one of them. These applets terminate on error (or sometimes even with no error) by calling xfunc_die(), which performs a longjmp(3).
• The shell uses setjmp(3)/longjmp(3) exception handling. It's this that results in echo a | cat failing: the shell running the echo calls longjmp(3). (So in the example above it isn't less which is at fault.) Another case is when the user hits Ctrl-C at the command prompt. That also causes the failure.
• A handful of random other applets also use longjmp(3), but I haven't checked them in detail.

Possible mitigations

• Support for NOFORK applets can be configured at build-time. Disabling it avoids the failure for uname and the others. It's less efficient this way, though.
• Avoiding setjmp(3)/longjmp(3) in the shell isn't so easy. It's kind of baked in.
• The llvm-mingw toolchain claims to have support for Control Flow Guard. I've built a busybox-w32 binary with it enabled (using the -mguard=cf flag), but is doesn't seem to make any difference.
• Rather than disabling CFG completely, it's possible to turn it off for busybox.exe specifically. (Settings -> Privacy & security -> Windows Security -> App & browser control -> Exploit protection settings -> Program settings -> Add program to customize, then override the CFG setting for busybox.exe.)

[avih]

I think disabling NOFORK would have a meaningful performance impact, especially for utilities which are typically used in hot (shell) code, like echo, or test.

Does it not affect noexec?

Is it possible for the application to opt-in to disabling this config? (though that would seem counter productive...).

But overall, it's weird that longjmp triggers this issue in general, because it's used in a lot of places outside busybox too. I think cpp exceptions use that too under the hood, so I find it quite strange that longjmp is effectively not allowed in some cases, as I imagine it can break a lot of things...

[avih]

I don't have the setup to test this, but if you want to test longjmp outside of busybox in this setup, you can try mujs (small JS 5.1 interpreter):

git clone https://github.com/ccxvii/mujs
cd mujs
touch UnicodeData.txt SpecialCasing.txt utfdata.h  # avoid download+regen
make release HAVE_READLINE=no
echo 'try { foo() } catch (e) { print("myerror: " + e) }' | ./build/release/mujs.exe
Welcome to MuJS 1.3.5.
myerror: ReferenceError: 'foo' is not defined
        at [string]:1

The try does a setjmp, entering the catch clause is a result of longjmp from the error in mujs, and the myerror: ... output is an indication that it worked.

EDIT: modified the touch command.

[rmyorston]

Only NOFORK applets are affected. A simple test program with a longjmp(3) didn't show any problem.

The issue arises because the BusyBox build uses the flags -fno-unwind-tables and -fno-asynchronous-unwind-tables. These were introduced by upstream commit 9b80b90 (build system: stop .eh_frame generation). BusyBox itself doesn't need unwind tables, but it seems Control Flow Guard does.

I've added a build-time option (UNWIND_TABLES) to control whether or not the unwind tables are included in the binaries. Prerelease binaries are here (PRE-5582).

The downside is that the unwind tables increase the size of the binaries, by about 7% for the 64-bit builds but 15-20% for 32-bit. The 32-bit builds are bigger than 64-bit, which was never the case before.

[avih]

Nice.

Did you confirm it works for both gcc and clang? UCRT and MSVCRT?

Presumably the issue was with clang too, and fixed there too?

[rmyorston]

I haven't tried all combinations of gcc/clang/UCRT/MSVCRT but I've done enough to see that in general the problem exists if unwind tables are omitted and goes away if they're included.

Except in two odd cases. It isn't necessary to include unwind tables for:

• The 32-bit x86 binary on 64-bit Windows 11. I don't have a 32-bit Windows 10/11 to test on.
• The ARM64 binary on Windows 11 for ARM.

I guess Control Flow Guard works differently in those cases.

I'd prefer to avoid shipping the bloated binaries, if possible.

[avih]

I'd prefer to avoid shipping the bloated binaries, if possible.

I've said that more than once in the past, and I'll repeat it:

That's not bloat. I don't think there's any busybox-w32 user which cares if the binary was even 10x bigger, especially when it's a result of compiler options and not actual code bloat, and certainly no user cares about 20-50% size increase due to compiler options to avoid breakage.

IMO the main measure of bloat is code size and complexity in C - not at the binary. The C code is what makes it harder to maintain and/or make further changes - not the binary size. Which is also the reason I generally dislike C code tricks/tweaks to make the binary smaller while the C code itself becomes more complex and/or harder to modify. It's just a bad tradeoff IMO, because humans write and deal with C code, so that's the resource which we should protect and care about most IMO, both in terms of quality and bloat.

It's true that upstream makes an effort to reduce the binary size too, but that means very little these days with the current bb sizes, and especially on Windows, where just the swap file or random windows update swings the free space by GBs at a time.

Therefore, IMO it's best to keep this option enabled (as seems to be the case), and not try to find ways to disable it in specific cases where no issue was observed. IMO if it's to be enabled selectively, then it should be due to (ms) docs which describe where it's required and where it's not, rather than empiric observations.

[ale5000-git]

Personally I do care about binary size but current size on Windows is still good enough, even with this change.

[janko-jj]

Asynchronous-unwind-tables are still non needed, right? Regarding the unwind-tables, as they are specified in gcc options, if I understand correctly gcc documentation, and knowing what the Windows ABI expects, these generated data are then in fact a part of Windows' 64-bit ABI, and I'm not too surprised that that "Control Flow Guard" feature depends on them. This documents them in the context of a C++ compiler, but they are actually more than that: https://learn.microsoft.com/en-us/cpp/build/exception-handling-x64?view=msvc-170

They aren't some kind of a debug info, but the part of the system runtime info of the binary (i.e. they need to be there if the system-level exceptions happen, independently of C++-specific exceptions, and are expected to exist for each function which can call some other function, even if the code is compiled without "debug info". From the ABI's perspective, it's definitely not "C++-specific" even if reading some documentation could give such an impression. On the system level, code can call some other code which causes system defined "structured exception" and it has to have the unwind tables for its own functions even if its functions aren't doing any "try" or "catch". In short, a C code could survive without them if it is so simple that never any API is called which can cause an exception, and if the system debugging infrastructure never interacts with the binary generated by such C code. That "never interacts" doesn't hold anymore, at least with "Control Flow Guard", it seems. Busybox has so much functionality that it's not surprising that its execution can't be reduced to the simplest scenarios.

The unwind info could simply be considered as a needed part of the x64 code. The good news is that it's data, not actual machine instructions, so the presence of it doesn't make code slower. Its non-presence, however, could result in non-working code.