std.process.Child: Mitigate arbitrary command execution vulnerability on Windows (BatBadBut)

[squeek502]

Note: This first part is mostly a rephrasing of https://flatt.tech/research/posts/batbadbut-you-cant-securely-execute-commands-on-windows/
See that article for more details

On Windows, it is possible to execute .bat/.cmd scripts via CreateProcessW. When this happens, CreateProcessW will (under-the-hood) spawn a cmd.exe process with the path to the script and the args like so:

cmd.exe /c script.bat arg1 arg2

This is a problem because:

• cmd.exe has its own, separate, parsing/escaping rules for arguments
• Environment variables in arguments will be expanded before the cmd.exe parsing rules are applied

Together, this means that (1) maliciously constructed arguments can lead to arbitrary command execution via the APIs in std.process.Child and (2) escaping according to the rules of cmd.exe is not enough on its own.

A basic example argv field that reproduces the vulnerability (this will erroneously spawn calc.exe):

.argv = &.{ "test.bat", "\"&calc.exe" },

And one that takes advantage of environment variable expansion to still spawn calc.exe even if the args are properly escaped for cmd.exe:

.argv = &.{ "test.bat", "%CMDCMDLINE:~-1%&calc.exe" },

(note: if these spawned e.g. test.exe instead of test.bat, they wouldn't be vulnerable; it's only .bat/.cmd scripts that are vulnerable since they go through cmd.exe)

Zig allows passing .bat/.cmd scripts as argv[0] via std.process.Child, so the Zig API is affected by this vulnerability. Note also that Zig will search PATH for .bat/.cmd scripts, so spawning something like foo may end up executing foo.bat somewhere in the PATH (the PATH searching of Zig matches the behavior of cmd.exe).

Side note to keep in mind: On Windows, the extension is significant in terms of how Windows will try to execute the command. If the extension is not .bat/.cmd, we know that it will not attempt to be executed as a .bat/.cmd script (and vice versa). This means that we can just look at the extension to know if we are trying to execute a .bat/.cmd script.

---

This general class of problem has been documented before in 2011 here:

https://learn.microsoft.com/en-us/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way

and the course of action it suggests for escaping when executing .bat/.cmd files is:

• Escape first using the non-cmd.exe rules
• Then escape all cmd.exe 'metacharacters' ((, ), %, !, ^, ", <, >, &, and |) with ^

However, escaping with ^ on its own is insufficient because it does not stop cmd.exe from expanding environment variables. For example:

args.bat %PATH%

escaped with ^ (and wrapped in quotes that are also escaped), it will stop cmd.exe from expanding %PATH%:

> args.bat ^"^%PATH^%^"
"%PATH%"

but it will still try to expand %PATH^%:

set PATH^^=123
> args.bat ^"^%PATH^%^"
"123"

The goal is to stop all environment variable expansion, so this won't work.

Another problem with the ^ approach is that it does not seem to allow all possible command lines to round trip through cmd.exe (as far as I can tell at least).

One known example:

args.bat ^"\^"key^=value\^"^"

where args.bat is:

@echo %1 %2 %3 %4 %5 %6 %7 %8 %9

will print

"\"key value\""

(it will turn the = into a space for an unknown reason; other minor variations do roundtrip, e.g. \^"key^=value\^", ^"key^=value^", so it's unclear what's going on)

It may actually be possible to escape with ^ such that every possible command line round trips correctly, but it's probably not worth the effort to figure it out, since the suggested mitigation for BatBadBut has better roundtripping and leads to less garbled command lines overall.

---

Ultimately, the mitigation used here is the same as the one suggested in:

https://flatt.tech/research/posts/batbadbut-you-cant-securely-execute-commands-on-windows/

The mitigation steps are reproduced here, noted with one deviation that Zig makes (following Rust's lead):

1. Replace percent sign (%) with %%cd:~,%.
2. Replace the backslash (\) in front of the double quote (") with two backslashes (\\).
3. Replace the double quote (") with two double quotes ("").
4. Remove newline characters (\n).
   • Instead, \n, \r, and NUL are disallowed and will trigger error.InvalidBatchScriptArg if they are found in argv. These three characters do not roundtrip through a .bat file and therefore are of dubious/no use. It's unclear to me if \n in particular is relevant to the BatBadBut vulnerability (I wasn't able to find a reproduction with \n and the post doesn't mention anything about it except in the suggested mitigation steps); it just seems to act as a 'end of arguments' marker and therefore anything after the \n is lost (and same with NUL). \r seems to be stripped from the command line arguments when passed through a .bat/.cmd, so that is also disallowed to ensure that argv can always fully roundtrip through .bat/.cmd.
5. Enclose the argument with double quotes (").

The escaped command line is then run as something like:

cmd.exe /d /e:ON /v:OFF /c "foo.bat arg1 arg2"

Note: Previously, we would pass foo.bat arg1 arg2 as the command line and the path to foo.bat as the app name and let CreateProcessW handle the cmd.exe spawning for us, but because we need to pass /e:ON and /v:OFF to cmd.exe to ensure the mitigation is effective, that is no longer tenable. Instead, we now get the full path to cmd.exe and use that as the app name when executing .bat/.cmd files.

---

A standalone test has also been added that tests two things:

1. Known reproductions of the vulnerability are tested to ensure that they do not reproduce the vulnerability
2. Randomly generated command line arguments roundtrip when passed to a .bat file and then are passed from the .bat file to a .exe. This fuzz test is as thorough as possible--it tests that things like arbitrary Unicode codepoints and unpaired surrogates roundtrip successfully.

Note: In order for the CreateProcessW -> .bat -> .exe roundtripping to succeed, the .exe must split the arguments using the post-2008 C runtime argv splitting implementation, see #19655 for details on when that change was made in Zig.

---

Some notes:

• When this was discussed in Discord, one possibility that was brought up was to disallow spawning .bat/.cmd through std.process.Child and instead make users do something more explicit. I am not in favor of that for a few reasons:
  • Zig behaving "like Windows" on Windows when possible is a nice feature to have IMO and makes cross-platform Zig code nicer overall, e.g. with Zig's behavior around searching PATH, running std.process.Child.run(.{ .argv = &.{"foo"} }); will find foo.bat or foo.cmd if it exists in the PATH, which matches what would happen if you tried to run foo in cmd.exe
  • Depending on how it's handled, Zig would either need to rely on the user to mitigate the BatBadBut vulnerability correctly themselves, or Zig would have to provide the mitigation anyway in whatever API would be used to spawn .bat/.cmd scripts. Neither of these options seems like an improvement over allowing .bat/.cmd scripts to be run via std.process.Child.run and having the correct BatBadBut mitigation in place automatically.
• This adds a call to kernel32.GetSystemDirectoryW which I know can be removed in favor of getting the data from the PEB, but I wasn't able to figure out how to interpret PEB.ReadOnlyStaticServerData in order to access the correct memory (I ran into this same problem in Replace GetWindowsDirectoryW usage with PEB reads squeek502/get-known-folder-path#2).

[andrewrk]

Thanks! Although it's technically breaking, I have cherry-picked it into the 0.12.x branch as 600b652.

[squeek502]

Follow up issue for something that I found a workaround for but don't have an explanation for: #19762