Quoting command-line arguments on Windows

(1) By Florian Balmer (florian.balmer) on 2019-03-10 08:49:12 [source]

When trying the recent enhancements to the backoffice command, I found that passing more than one repository on the command-line doesn't work, on Windows:

fossil backoffice repo1 repo2

the [C:\...\master\win\fossil.exe] argument to the "" command
contains a character (ascii 0x5c) that is a security risk

(The message origins from escaping the path to Fossil to relaunch another instance, and not from escaping the paths to the repositories.)

Consequently, the new --poll option to init a backoffice launch daemon doesn't work with multiple repositories, on Windows.

A pragmatic approach to fix this is to use the same quoting rules (i.e. none?) as for the various flavors of the server command on Windows, which also work with relaunched Fossil instances, and command-line arguments containing paths to temporary files (more on this below).

A more general fix would be to implement proper Win32 command-line argument quoting:

Command-line arguments to Win32 applications are allowed to contain embedded literal double quotes escaped by backslashes.
Backslashes immediately preceding an (escaped or unescaped) double quote also need to be escaped by backslashes (that is, doubled) to be interpreted as literal backslashes, rather than escapes to the following double quote, or the escaping backslash, respectively.
The two rules above don't apply to argv[0], which is always a file name that is not allowed to contain a double quote, and that is terminated by any double quote, escaped or not (yet, a file name can't end with a backslash, either).

Some examples of Win32 command-lines, and corresponding argv[] arrays (the first argument is omitted in both the command-line string and the argv[] array):

"em\bedded" "backslash"
├───[em\bedded]
└───[backslash]

"em\\bedded" "backslash"
├───[em\\bedded]
└───[backslash]

"em\"bedded" "quote"
├───[em"bedded]
└───[quote]

"em\\"bedded" "quote"
└───[em\bedded quote]

"em\\\"bedded" "quote"
├───[em\"bedded]
└───[quote]

"trailing\" "backslash"
└───[trailing" backslash]

"trailing\\" "backslash"
├───[trailing\]
└───[backslash]

"trailing\\\" "backslash"
└───[trailing\" backslash]

Note: (Non-escaped) opening and closing double quotes are removed (without any effect), if not preceded or followed by a space, respectively.

References:

So simply allowing backslashes in the function to escape the command-line arguments is not enough. And even if embedded double quotes would remain disallowed by Fossil, trailing backslashes might still turn a closing double quote into a literal double quote, and mess up argument parsing for the spawned program.

(Proper quoting would also be handy for the ui command, so that URLs with query strings containing &, ", and \ passed with the --page option would be correctly forwarded to the web browser.)

(More on the server command quoting rules: it's safe to assume that paths to existing files won't contain any double quotes on Windows because they are not allowed, and trailing backslashes from directory arguments could be stripped; however, one notable exception is that C: and C:\ are not the same, the former refers to the current directory on drive C, and the latter to the root directory on drive C.)

CAVE: This is about the native Win32 API approach, maybe that more *nix-oriented environments (for example, CYGWIN) follow different rules, here.

Is this something considered worthy to be fixed?

(2) By anonymous on 2019-03-11 16:23:22 in reply to 1 [link] [source]

IIRC, there was one more place in backoffice processing that needed quoting around repo path on WIN32 (fossil process launched with _wspawnv() ):

src/backoffice.c:backoffice_run_if_needed()

This is applicable to an individual repo arg when its path has characters that need quoting (space, quotes, etc.).

(3) By Florian Balmer (florian.balmer) on 2019-03-12 09:36:26 in reply to 2 [link] [source]

Yes, the "exec" and "spawn" families of functions simply concatenate the arguments to a string, without performing any quoting or escaping.

Your linked example might produce the following command-line:

C:\path with spaces\fossil.exe backoffice -R C:\path with spaces\repo.fossil

Going the easy way and just enclosing the executable and repository paths in double quotes, i.e. without checking for embedded double quotes, or trailing backslashes, is likely to work for almost every case:

"C:\path with spaces\fossil.exe" backoffice -R "C:\path with spaces\repo.fossil"

Neither the executable path, nor the repository path can contain an embedded double quote, or a trailing backslash that would escape the terminating double quote.

The "system" family of functions is different, because it's calling cmd.exe (or whatever %COMSPEC% points to), and therefore requires additional quoting and escaping.

For example, the web browser for the ui command is launched via fossil_system()→_wsystem(). That's why web pages with query strings containing &'s can't be passed via the --page option. Fossil doesn't quote or re-escape them before handing them on to the "system" function, hence cmd.exe will treat them as command chain separators. The &'s would (mostly) work if enclosed in double quotes "&", or (always) work if escaped with the caret/circumflex character ^&.

(4) By Florian Balmer (florian.balmer) on 2019-03-12 14:22:46 in reply to 1 [link] [source]

What about merging/cherry-picking [c52fb5eddb]?

This looks interesting to me, maybe the author can give more comments on the reasons for this change?

Quoting of command-line arguments is also improved, with this commit, and it seems reasonable to pass the --nocgi option to backoffice child processes.

Could analogous argv quoting, and maybe also the --nocgi option, be added to [96fc484876]?

(5) By Thomas Schnurrenberger (tsbg) on 2019-03-13 08:04:26 in reply to 4 [link] [source]

Here are my comments as the author of checkin c52fb5eddb:

The backoffice process is created as a "detached* process, which means, it stays running even if the creating process terminates (CreateProcessW with the DETACH_PROCESS create flag).

The "--nocgi" option is very important.
If Fossil is executed as a CGI process, it tries to process a http request per default.

Fossil looks for the environment variable "GATEWAY_INTERFACE" to decid if it should execute as CGI.

Every process that gets created from a running CGI process by default inherits this variable!

Conclusion: every process that gets created from a running CGI process must have the "--nocgi" option, unless it handles a http request.

The changes in alert.c and popen.c have to do with the "pipe to command" email sending method. On Windows, the pipe must be set to binary mode. The popen2() does that. I had to change the process creation in popen.c to not create a new console window, otherwise command windows are popping up when running fossil server and a backoffice process executes!

popen2() is also used in the SSH code. I am not sure if this change affects the SSH operation, this needs testing.

(6) By Florian Balmer (florian.balmer) on 2019-12-03 15:27:14 in reply to 1 [link] [source]

Sorry for posting to this old thread, just collecting some reference material in case I ever find the time to do more work on this.

How Command Line Parameters Are Parsed by David Deley

This article explains a simple method to quote arguments passed to system() on Windows (going through CMD.EXE).

If the fossil_system() calls are merely used to launch external programs (and not built-in shell commands such as del, dir, etc.) then it may be easier to call CreateProcess() directly, with neither DETACHED_PROCESS nor CREATE_NEW_CONSOLE set in the dwCreationFlags, and use WaitForSingleObject() to wait for the child process to terminate.