Windows: Consider disallowing .bat and .cmd files in Command::new

[ChrisDenton]

In light of CVE-2024-24576 it was suggested we disallow running .bat files using Command::new. It was not a change we wanted to make in a point release, especially without discussion with the full libs-api team. so I'm writing this issue for libs-api to consider and accept or reject.

This was never (previously) documented and originally only worked accidentally due to undocumented CreateProcess behaviour. In fact CreateProcess actively documents against using it this way:

To run a batch file, you must start the command interpreter; set lpApplicationName to cmd.exe and set lpCommandLine to the following arguments: /c plus the name of the batch file.

Which in Rust terms means using Command::new("cmd.exe").args(["/c", "script.bat"]).

However, while this is was not previously documented, people could be (and some are) relying on it. so it would be breaking for them. The fix would be:

• for trusted input, use cmd /c as stated above
• for untrusted input, use [insert crate here]

Note that this would only affect people that use a path to a .bat file. The standard library only searches for .exe files in PATH. The standard library (on Windows) also does not support running script files in general. Previously, .bat files had been an accidental special case. So using a crate (or your own code) is necessary for other script types.

[side note: when I say .bat I also mean .cmd as they are effectively the same thing as far as this issue is concerned]

[jyn514]

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

However, since Windows includes .bat and .cmd files in the PATHEXT environment variable by default, some runtimes execute batch files against the developers’ intention if there is a batch file with the same name as the command that the developer intended to execute

does libstd ignore PATHEXT? or is it possible that Command::new("script") could have the same effect as Command::new("script.bat")? in the latter case i don't see much point in disallowing .bat specifically (although it might be reasonable for clippy to lint on it).

[ChrisDenton]

Rust's std does not use PATHEXT and nor does the underlying CreateProcess API. I did once make an issue about Command running scripts but most felt this was better done in a crate (see #94743)

[cuviper]

FWIW, I was previously confused about PATHEXT too: #50870 (comment)

Other relevant issues for cross reference: #37519 #87945 #93124

[ChrisDenton]

I think we definitely do need to flesh out our documentation. But I'd also like to explicitly decide what our behaviour should be rather than just documenting behaviour which is mostly accidental. Even if that decision ends up being to keep doing what we're currently doing, I'd rather that was a conscious choice.

[ChrisDenton]

This was discussed in the libs-api meeting. There was no consensus on dropping this behaviour.

However there's also not currently a consensus on documenting this behaviour, rather than just keeping some degree of hidden support for compatibility reasons but not actively advertising it (maybe with a lint on static strings passed to Command::new that contain .bat or .cmd extensions).

[ChrisDenton]

This was again discussed in the latest libs-api meeting. It was felt that keeping our current behaviour was worth it for the sake of compatibility. So I'll close this.

I'll add that our documentation does now have warnings around bat use. Improved documentation for Windows specific issues can of course be added but that's a separate issue.