Page MenuHomePhabricator

How To Properly Escape Commands on Windows (A Dark Tragedy)
Open, NormalPublic

Description

At the time of writing, arc does not escape all inputs to command line functions correctly on Windows. That is, for a snippet like this:

list($stdout) = execx('echo -n %s', $input);

..there is a set of possible values for $input where the code will execute without errors but $stdout and $input will have different values at the end of execution. Some examples of such input are:

%APPDATA%
\0
\n

As an aside, this is also true on Linux systems (the code can execute without errors but the input and output may differ), because there does not appear to be any way to make echo echo the string -n, at least on OS X:

$ echo -n
$ echo -n -n
$ echo -n -- -n
-- -n

For the purposes of this discussion, assume echo is a platonic ideal of echo which can truly echo any string.


For prior discussion, see T8298 and connected tasks. This discussion is extensive and meandering, but roughly summarizes as:

  • no one really has any idea how to escape things on Windows; and
  • it appears to be impossible to escape a large set of inputs on Windows.

Environments

Part of the reason this is difficult is that Windows is not a single shell environment, but at least three: cmd.exe, MSYS, and Git Bash. (We may, today or in the future, also need to deal with Powershell.) The three shells have different behavior, with cmd.exe generally having the most absurd behavior.

I currently believe that cmd.exe, at least, can not escape all inputs, and that the behavior of csprintf() under cmd.exe must sometimes be to throw an exception saying "this input can not be escaped, use a different input (or use Git Bash / some other shell / some obscure workaround)". This is wholly absurd, but at least an improvement over the current behavior.

Executables vs Arguments

Windows escaping behavior in at least some shells appears to be different for the first argument (the binary/executable) than for other arguments. This may require separate escaping behavior.

For example, in Linux, ls, 'ls', and "ls" all do the same thing. In cmd.exe, echo and "echo" have different behavior -- "echo" does not work. However, where, where.exe, "where.exe" and "where" all work.

Bypass Shell

Under Windows, proc_open() in PHP has a bypass_shell mode. It is broadly unclear what this option actually does or how it affects command escaping.

PHP

In PHP, command line escaping is handled by escapeshellarg(). This function is completely broken on Windows and makes no effort to escape inputs correctly. It explicitly produces incorrect output without raising an error:

On Windows, escapeshellarg() instead replaces percent signs, exclamation marks (delayed variable substitution) and double quotes with spaces and adds double quotes around the string.

Python

Python's subprocess (in Python 3) has run(...) and list2cmdline(...). These seem generally more promising as models for escaping behavior. One calls the other. This is under Python 3; under Python 2 it looks like run() is call()?

By default, run(...) appears to use a bypass_shell-like mode and successfully escape most inputs. You can pass shell=True to get a shell mode where everything is broken, as one might expect.

For most of these experiments, I've written a version of echo as <?php echo $argv[1]; and am executing it as php -f echo.php -- .... When I say phpecho below I mean this construction, as distinct from the cmd.exe builtin echo.

When run with shell=True, Python does not escape %, so phpecho %APPDATA% prints the environmental variable. phpecho a\nb prints only "a". To its credit, python is at least somewhat better than PHP here and throws when passed a NULL byte, although this behavior (on OSX, under Python 2.7, with actual echo) is confusing to me:

>>> subprocess.call(["echo", "\0"]);
...
TypeError: execv() arg 2 must contain only strings

With shell=False, arguments with spaces are quoted, with internal quotes and backslashes escaped with backslashes. From cursory examination, no other characters appear to receive special treatment.

The Python 3 documentation claims:

When using shell=True, the shlex.quote() function can be used to properly escape whitespace and shell metacharacters in strings that are going to be used to construct shell commands.

I think this is not correct. In particular, shlex.quote(...) does not quote %APPDATA% and this:

>>> subprocess.run([..., shlex.quote('%APPDATA%')], shell=True)

...prints the value of the variable, not the literal %APPDATA%. So this is perhaps a point to PHP, as being completely broken, while bad, is still better than documenting something unsafe to be safe.

Event Timeline

epriestley triaged this task as Normal priority.
epriestley updated the task description. (Show Details)Oct 1 2018, 7:42 PM
epriestley updated the task description. (Show Details)Oct 1 2018, 9:14 PM
epriestley updated the task description. (Show Details)Oct 1 2018, 10:35 PM

I'm currently trying to update my Surface Pro so I can look at making arc unit in the wilds branch pass on Windows. I've restarted to install more updates about 5 times, am currently stuck at "Feature update to Windows 10, version 1803 - Downloading - 0%" which appears to just be not working.

I bought this Surface about a month ago and have only used it to install Windows updates so far, with mixed success.

On startup on Windows, arc currently complains that cURL isn't enabled. It gives you some okay-ish instructions for enabling it.

I'd like to just load cURL for you. We can do this by having arc.bat run php -d extension_dir=ext -d enable_dl=true ... and then using dl('curl') from within arc. I'm unsure if this will, on the balance, create or resolve more problems.

We can't set either extension_dir or enable_dl from within the PHP process so we have to set these blind from the .bat file. We could immediately re-execute a subprocess with those values modified, however. This would let us do more logic to figure out the right values, then run a subprocess once we make a determination.

We also only need cURL to make HTTP requests, and we could implement HTTPSFuture as a subprocess on Windows which runs php -d ... various-flags ... -f utils/curl.php -- <url> or similar. But this feels very yuck.

A lot of tests fail on Windows if core.autocrlf is set to convert "\n" line endings to "\r\n" line endings on checkout, which is the default behavior. In theory, this is fixable, but I'm not sure it's really worthwhile. At the least, I first want to get things passing without dealing with these tests.

ArcanistLinterTestCase->lintFile() has a bug where it does this:

$mode = idx($config, 'mode', 0644);
if ($mode) {
  Filesystem::changePermissions($tmp, octdec($mode));
}

If the test case defines a mode as an octal string, this code works correctly.

If the test case does not define a mode as an octal string, we use 0644 (already specified in octal) and then convert the decimal representation into an octal value. The intent of the code is to convert "0644" (an octal as a string) instead. Effectively, it is calling octdec(octdec("0644")).

Since we only care about +x and I think this happens to survive correctly through sheer luck, this worked properly on Linux systems anyway. On Windows systems, it appears that it can make the file unwritable by us, which prevents unlinking it later, causing TempFile to raise errors about permissions issues on destruction.

To fix this, I'm simply going to remove the TempFile code since we no longer need it, at least for now. However, this may be something to watch out for if we see similar errors with unlink() calls in TempFile failing under Windows.

PHP7 introduces random_bytes(), which is a more accessible pseudorandom source on Windows than the other sources we use.

There's a polyfill available for older PHP, although it relies on a bunch of COM stuff:

https://github.com/paragonie/random_compat/blob/master/lib/random_bytes_com_dotnet.php

For now, I'm not planning to polyfill.

I think I'm done for the night, but we're down to 18 failing tests under Windows.

epriestley updated the task description. (Show Details)Oct 2 2018, 12:50 PM
ftdysa added a subscriber: ftdysa.Oct 2 2018, 2:24 PM

Consider this script:

echo.php
<?php

echo "A B C";
echo "D E F \xED\xA9\x98 G H I";
echo "J K L\n";

When run in Windows 10 cmd.exe as php -f echo.php, what will it print?


Here's what it prints (there's nothing invisible between C and J):

A B CJ K L

It seems that if you try to print a string with certain "scary" bytes, the cmd.exe shell is too scared of the bytes and declines to print any of the text. Note that the bytes are printed correctly if the output is sent to a file with > out.txt.

print and echo have no error codes.

printf('%s', ...) does not fail when passed these scary bytes, and returns the correct length of the string, as though it was printed to the console.

So echo $x may fail wholly and undetectably for some values of $x.

Worse, this appears to be a PHP-ism since the equivalent Python program has behavior more similar to what I'd expect on a Linux system, although it's hard to be certain that Python isn't just covering for cmd.exe here since there no way to tell what bytes are "really" being printed.

Also, guess what this checkbox does on Windows 10?

[√] Always use this application to open ".php" files

That's right: this checkbox has no effect.

zalun added a subscriber: zalun.Oct 25 2018, 2:10 PM
hskiba added a subscriber: hskiba.Nov 14 2018, 2:49 PM
quark.zju added a subscriber: quark.zju.EditedDec 28 2018, 8:14 PM

I'm not an Windows expert. But to my knowledge there are a couple of things missing here:

  • POSIX systems treats command line arguments as char *argv[]. It is an array of C strings.
    • So you cannot store \0 in arguments as it's reserved to represent the end of a C string.
  • Windows system treats command line argument as a single string, instead of an array. It's up to user-space applications to decide how to escape it into a list of strings (i.e. cmd.exe probably does not even try to escape it)
    • Applications using MSVC runtime has documented behavior.
    • Other applications might have different behaviors.

And some Windows-specific complexities:

  • echo in cmd.exe is a builtin command. There is no echo.exe. If echo.exe exists, "echo" would call that exe.
  • Windows APIs ending with A will treat input strings as in the "active code page" encoding, and convert them to Unicode when displaying them in cmd.exe. It does not change the raw bytes that other applications read from, though. It seems PHP 5.6.25's "echo" implementation uses A-family Windows API. Here are some observed outputs in cmd.exe:
> chcp 936
> php echo.php
A B CD E F 愆?G H IJ K L
> php echo.php | py -3 -c "import sys, hashlib; print(hashlib.sha1(sys.stdin.buffer.read()).hexdigest())"
6d27534c315735c3f5afeaf7a82f65b123d3ccdb

> chcp 65001
> php echo.php
A B CD E F �� G H IJ K L
> php echo.php | py -3 -c "import sys, hashlib; print(hashlib.sha1(sys.stdin.buffer.read()).hexdigest())"
6d27534c315735c3f5afeaf7a82f65b123d3ccdb

So characters displayed in cmd.exe do not reflect the real bytes written.

It seems Python 3's sys.stdout.buffer.write can be used to write raw bytes, encoded in utf-8 to terminal regardless of the active code page. So it might use the W-flavored API and does some conversion under the hood (I haven't checked):

; The output is consistent across different code pages (i.e. chcp won't change its output)
; cmd.exe needs to be configured to have a font to display the characters.
> py -3 -c "import sys; sys.stdout.buffer.write(b'a\0b\0\xe4\xbd\xa0\xe5\xa5\xbd\n')"
a b 你好
>py -3 -c "import sys; sys.stdout.buffer.write(b'a\0b\0\xe4\xbd\xa0\xe5\xa5\xbd\n')" | py -3 -c "import sys, hashlib; print(hashlib.sha1(sys.stdin.buffer.read()).hexdigest())"
565ffe4912252e45185dee4692d68626bf707cd8