syscall: special case cmd.exe /c <command> in StartProcess

[qmuntal]

Background

It is well known that os/exec doesn't correctly escape nor quote *.bat, *.cmd, cmd /c * arguments. This is because in all three cases the escape/quote rules that apply are the ones defined in cmd.exe docs, which are slightly different than the ones os/exec and syscall.StartProcess follow, defined in parsing-c-command-line-arguments. This discrepancy causes bugs like #1849, #17149, #68313, and can also lead to security vulnerabilities.

The only workaround that exists today to reliably execute *.bat, *.cmd, cmd /c * command using os/exec is to manually escape/quote the arguments at caller site and pass the resulting string to syscall.SysProcAttr.CmdLine. The problem is that having a robust implementation is complicated, so projects tend to have half-backed solutions, if any.

Proposal

Special case %COMSPEC% /c <command> (%COMSPEC% usually points to cmd.exe) by applying the cmd.exe escape/quote rules to <command>. The exact rules are left for the implementer, as they are well documented.

Some considerations to take into account:

• cmd.exe has two types of quotation rules, let's call it default and special. We should follow the special rule (search for /s in the docs), as it is 100% predictable in comparison with the default rule, which has many limitations and can easily fallback to the special rule.
  The special rule is simple: if <command> starts with a ", then the the leading and trailing quotes are stripped. This means that we should always surround <command> with quotes and pass /s before /c.
• cmd.exe allows passing multiple cmd-specific parameters before /c appears. The command is always what goes after /c. Therefore, cmd.exe /d /c <command> is valid and we should special-case it.
• cmd.exe also execute commands passed after the /k parameter. That is used to keep the command processor running after the command is executed, so it doesn't really fit well with the one-shot approach of syscall.StartProcess. We can ignore it.

Why not special case also bat/cmd files

This proposal doesn't attempt to solve issues related to directly executing bat/cmd scripts for the following reasons:

• Windows CreateProcess API explicitly disallows passing the bat/cmd scripts in the application name and recommend to use cmd /c instead.
• It is not documented how to reliably detect bat/cmd files. Just using the extension seems brittle. In part this is why CreateProcess recommend to use cmd /c.
• Rust have been trying to reliably support bat/cmd scripts for more than three years, and the Rust library team recently tried to remove that support due to being difficult to implement correctly: Windows: Consider disallowing .bat and .cmd files in Command::new rust-lang/rust#123728.
• os/exec will now have a good workaround to execute bat files: exec.Command("cmd.exe", "/c", "foo.bat", "arg 1")

Note that I'm not putting this proposal in the proposal process because it is not adding new API nor breaking existing behavior. It is more as an umbrella issue to discuss the design and the implementation.

@golang/security @golang/windows

[gabyhelp]

Related Issues and Documentation

• exec: added comment to Command to explain how to pass arguments to cmd.exe
• Automatic parameter escaping in syscall.StarProcess on windows is too restrictive #1849 (closed)
• syscall/exec_windows.go: appendEscapeArg does not escape all necessary characters #68313 (closed)
• syscall: quote command line correctly for Windows cmd.exe
• syscall: Add double quotes if parameter contain special symbols
• os/exec: execution of batch-files (.cmd/.bat) is vulnerable in go-lang for windows / insufficient escape #27199 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}