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.)}

[cherrymui]

nor breaking existing behavior

It sounds to me that if we do this, some programs' behavior would change. Would it only change from fail to not fail, or it can change in other ways?

One could argue that if someone wants to use os/exec or syscall.StartProcess with "cmd.exe /c", they needs to understand how to write the arguments correctly. Is the current escaping rule prevent one from writing the correct arguments?

Another possibility is that we provide a new API that does the right escaping.

Another possibility is that we provide an API (perhaps in golang.org/x/sys/windows) that runs "cmd.exe /c" with the right escaping.

[qmuntal]

It sounds to me that if we do this, some programs' behavior would change. Would it only change from fail to not fail, or it can change in other ways?

I was a bit optimistic when writing this issue. I've been investigating a lot this days about how to better support cmd /c without breaking existing programs that rely on the command not being properly escaped.

Unfortunately, we can't just start escaping all special characters, as there are valid cases to use them, even if they can lead to security issues when accepting untrusted user inputs.

My conclusion is that the only thing we can safely do is to unconditionally apply the following transform: cmd /c <command> -> cmd /s /c "<command>". This will solve the use case where the first argument is the name of an executable that need quoting (because it contains spaces), and there is at least another argument that need quoting or contain a double quote. When that happens, cmd enters in "/s" mode, removing the leading quote from the executable name, making it invalid. See a real bug caused by this behavior in #17149. On the other hand, if we always add the /s argument and surround <command> with double quotes, then cmd will always do the right thing: don't try to be smart and just remove the quotes we added.

The current syscall.StartProcess escaping rules doesn't allow to apply the previous transformation at the caller site, so that would be a neat win.

Another possibility is that we provide a new API that does the right escaping.

Agree. I'll think more about how this API could look like.

[gopherbot]

Change https://go.dev/cl/621795 mentions this issue: syscall: quote "cmd.exe /c" command on Windows