Potential for command injection in arguments of `Process.run` on Windows

[straight-shoota]

This issue originates in the Win32 API and is present in many libraries exposing a feature to start a process on Windows. The vulnerability has been dubbed BatBadBut.

CreateProcess() [of the Win32 API] implicitly spawns cmd.exe when executing batch files (.bat, .cmd, etc.), even if the application didn’t specify them in the command line. The problem is that the cmd.exe has complicated parsing rules for the command arguments, and programming language runtimes fail to escape the command arguments properly. Because of this, it’s possible to inject commands if someone can control the part of command arguments of the batch file.

A deeper analysis is available at https://flatt.tech/research/posts/batbadbut-you-cant-securely-execute-commands-on-windows/

Crystal's Process.run is affected as this program shows:

File.write("script.bat", "\n") # The newline is important for a valid batch script
Process.run("script.bat", ["&calc"])

Instead of evaluating script.bat and passing an argument of value &calc, it ends up running the batch script as well as calc.exe.

(Note the content of script.bat doesn't doesn't really matter, the issue is in the argument handling of the implicit cmd.exe /c.)

This is without shell: true, so the expectation is that command arguments passed via args parameter are safely escaped. It's possible to abuse this behaviour by injecting commands into arguments for batch files.

According to the linked article it's possible to prevent this by applying proper escaping. I think we're already doing some amount of that (particularly related to double quotes) but we also need to take other characters (particularly & and %) into account. There is already a discussion about this in #14300.

For reference, this is what Rust did: https://github.com/rust-lang/rust/pull/123681 This is what node.js did: https://github.com/nodejs/node/commit/64b67779f72ea9e4a0f444284576df9e591d79a0

Related discussions: https://github.com/crystal-lang/crystal/issues/9030, https://github.com/crystal-lang/crystal/issues/12873, #14300

[oprypin]

Oh this is so bizarre. In that case we should error out if the value ends with ".bat" or ".cmd"

[oprypin]

Rust indeed took the completely inexplicable approach of trying to escape the args somehow according to the Batch language if the command name ends with ".bat" or ".cmd". They did that because they were also already prepending cmd.exe, which is also inexplicable. We can just keep not supporting this.

[oprypin]

Process.run("test.txt") produces this error: Unhandled exception: Error executing process: 'test.txt': (IO::Error)

We should consider it a bug in Crystal that Process.run("test.bat") doesn't produce the same error

[oprypin]

Consider also this: Thanks to Windows' efforts, these two statements are currently ~~about~~ exactly equivalent:

Process.run("script.bat", ["&calc"])
Process.run("cmd.exe", ["/c", "script.bat", "&calc"])

Maybe something can be done about the 1st footgun, but never about the 2nd. cmd.exe is simply not an executable that uses C-compatible array-based argument parsing, it accepts a single-line command in Batch syntax.

[straight-shoota]

We're in the comfortable position that Windows is still not an officially supported platform and we can more freely introduce changes that might break backwards compatibility. Other languages are more constrained in supporting existing behaviour that might depend on these features.

I'd be fine with disallowing .bat and .cmd. We just need to make sure we filter exactly the same conditions as CreateProcessW does. There is a possible caveat that filename extensions can theoretically be omitted (the command script can be used to run the shell script named script.bat). But I don't think Process.run supports PATHEXT, so we're probably fine.

[HertzDevil]

We actually rely on being able to run bin/crystal.bat on Windows for any standard library spec that requires compiling Crystal code (see ::compile_file in spec/std/spec_helper.cr), e.g. the bulk of spec/std/kernel_spec.cr.

I'm not so sure if we can completely reject anything cmd.exe-related in Process, even assuming we are free to make breaking changes here.

[oprypin]

Run cmd /c script.bat there instead. Surely we don't have to rely on this trick in Windows' internals. That's literally all there is to its functionality: if the argument to CreateProcess starts with *.bat, prepend cmd /c to it

[oprypin]

https://github.com/crystal-lang/crystal/blob/82a208c7be45fe0b130557de3deed35734fb2d8b/spec/std/spec_helper.cr#L85

@HertzDevil I'm not sure if this is even used, actually.

bin/crystal is specified there, not bin\crystal.bat. (CreateProcess does not add the .bat extension itself.)

So it must be that the specs are relying on something like this setup

https://github.com/crystal-lang/crystal/blob/82a208c7be45fe0b130557de3deed35734fb2d8b/.github/workflows/win.yml#L253

[straight-shoota]

@HertzDevil Where does compile_file_ implicitly run a batch script? PATHEXT doesn't work so the extension would need to be explicit and I don't see that anywhere.