The attack is basically:

git clone 'ssh://-oProxyCommand=<anything you want to execute>/' ...

This executes anything.

However, it's not immediately clear that we're vulnerable.

git, svn, and hg... wow

These are slightly more fleshed-out versions of the attack in Mercurial:

https://www.mercurial-scm.org/repo/hg/rev/53224b1ffbc2
https://www.mercurial-scm.org/repo/hg/rev/f9134e96ed0f

All of Mercurial, Subversion and Git appear to be vulnerable in the same way.

The theoretical attack here is:

• A user adds one of these URIs to a repository in Diffusion.
• Then, a few moments later, the daemons clone/fetch the repository and the attacker gets control of the machine.

However, we've been very strict about what can appear in the domain/host part of a URI since D5797 (April 29, 2013) and before D5726 (April 17, 2013). The pattern we require is:

/^([a-zA-Z0-9\\.\\-]*)$/

This does not prevent users from specifying URIs in the form -flag, but does prevent -flag=..., $, spaces, |, etc., which I believe are necessary to do any actual damage. So I think you can break stuff a little bit, but can't really hurt anything.

We also used to have a separate PhutilGitURI which had looser rules, but I removed this in D16100 (June 13, 2016) and all URI parsing now goes through PhutilURI which has the stricter rules.

Additionally, we use GIT_SSH, SVN_SSH and ui.ssh to proxy SSH connections through bin/ssh-connect, which is careful about parsing and provides -- ("end of arguments") to the underlying SSH command before the host argument. So even when a dangerous URI is configured, the daemons actually escape it correctly.

For example, the URI ssh://-oxyz/path can be configured in the URI. If we run this command:

$ GIT_TRACE=1 git clone 'ssh://-oxyz/path' x

...we can see Git do something dangerous:

15:06:54.327634 run-command.c:350       trace: run_command: 'ssh' '-oxyz' 'git-upload-pack '\''/path'\'''

...and the output reflects that:

command-line: line 0: Bad configuration option: xyz

However, when we configure this URI and fetch it with the daemons, we get this instead:

ssh: Could not resolve hostname -oxyz: nodename nor servname provided, or not known

That's because we run:

$ GIT_SSH=/path/to/phabricator/bin/ssh-connect git ls-remote 'ssh://-oxyz/path'

...which runs bin/ssh-connect -oxyz git-upload-pack /path, which runs:

ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -- '-oxyz' 'git-upload-pack '\''/path'\'''
                                                                ^^
                                                                ++-- This is the critical part!

...which is safe because we use -- correctly to demarcate the end of flags.

So I'm reasonably confident that Phabricator is not directly vulnerable here; the attack is mitigated by our relatively strict host/domain parsing and then fully defused by proper use of -- to protect against this class of defect.

I'm going to tighten parsing in PhutilURI slightly to reject URIs which have host/domain names beginning with - -- these are not legitimate URIs. I'll also upgrade the $ in the regexp to a \z. However, I believe these are just reasonable hardening measures and that we are not vulnerable here.

I'll leave this open until I write up the release notes since it deserves a mention (users are still vulnerable if an attacker tricks them into running a suspicious git clone command), but I think we're otherwise unscathed by this.

Note that Phabricator can manifest Mercurial working directories. See executeMercurialUpdate() in src/applications/repository/engine/PhabricatorRepositoryPullEngine.php. It does this when pulling non-hosted repos. I know this occurs when observing repos. Not sure where else this code is used.

Anyway, creating a Mercurial working directory exposes you to the vulnerability since Mercurial will update subrepos automatically when doing working directory update. So, if a Phabricator instance is running a vulnerable Mercurial client and it updates to a changeset with an exploitable subrepo ssh:// URL, you could p0wn the Phabricator server.

Working directory updates are slow and shouldn't be needed for content indexing. So I'll repeat what I'm pretty sure I've already said on this issue tracker: it is better to either write a custom Mercurial extension to get the raw data you need (this will be fastest but will break periodically because Mercurial doesn't provide internal API stability guarantees) or use the Mercurial command server and invoke the files, cat, log, etc commands to get the raw data you need without the Python process startup overhead making things absurdly slow. This will eliminate the entire class of working directory related vulnerabilities and will save drastic amounts of I/O on Phabricator servers due to not having to populate a working directory.

That same code I pointed to for Mercurial also seems to perform Git working copy checkouts. Although I can't recall Git's semantics for automatically updating submodules (because I don't use them). It is worth auditing.

See also T4416. Removing -u hasn't been a priority because no actual install has expressed interest in it.

I'm not immediately able to reproduce any sort of issue with subrepos. Specifically, our use of the ui.ssh config option (passing a script which uses -- to separate flags and arguments) is the primary line of defense here, and hg appears to preserve ui.ssh when interacting with subrepos.

Compare:

$ hg up
 subrepository nested diverged (local revision: 92b796c9717b, remote revision: 92b796c9717b)
(M)erge, keep (l)ocal or keep (r)emote? r
remote: command-line: line 0: Bad configuration option: xyz
abort: no suitable response from remote hg!

...with ui.ssh using Phabricator's connect wrapper:

$ hg --config ui.ssh=/Users/epriestley/dev/phabricator/bin/ssh-connect up
 subrepository nested diverged (local revision: 92b796c9717b, remote revision: 92b796c9717b)
(M)erge, keep (l)ocal or keep (r)emote? r
remote: ssh: Could not resolve hostname -oxyz: nodename nor servname provided, or not known
abort: no suitable response from remote hg!

Note that there is no remote: command-line: ... line because ssh-connect causes the actual SSH subprocess which is executed to be ssh ... -- <host> (safe) instead of ssh ... <host> (code execution).

A possible attack here is that you might be able to use an HTTP parent repo with an SSH subrepo. That will currently dodge specification of ui.ssh in the hg command.

I'll make an adjustment here; I guess this raises the severity back to "everything is on fire".

I also can't get hg pull -u -- <uri> to fetch subrepos, am I just not setting things up correctly? In my current working state, hg up tries to interact with the subrepo remote but hg pull -u -- <uri> (which is what we actually execute) does not.

I'm going to cherry-pick rP794e185bf90e (the SSH wrapper stuff) to stable and hotfix production, although I'm not entirely certain hg pull -u -- <uri> is vulnerable.

I'm going to hold D18390 until after tomorrow's release cut and then let it bake in master for a week. I believe it's low risk, but doesn't specifically mitigate any known aspect of this which isn't mitigated by other things.

The subrepo issue is when .hgsub has malicious content (ex. foo = ssh://-oProxyCommand=touch%20BAR/). It's not related to command line or config files.

In the example above, I put malicious content in .hgsub, like this:

nested = ssh://-oxyz/path

With this malicious content in .hgsub, the command hg up --debug tries to run this command:

running ssh -oxyz 'hg -R path serve --stdio'

This command is unsafe, -oxyz is treated as a flag to ssh.

With this malicious content in .hgsub, the command hg --config ssh.ui=/path/to/phabricator/bin/ssh-connect up --debug, which is what Phabricator runs (that is, with ssh.ui) tries to run this command:

running /Users/epriestley/dev/phabricator/bin/ssh-connect -oxyz 'hg -R path serve --stdio'

This command is safe, because ssh-connect is careful about argument handling and runs (approximately) ssh -- -oxyz 'hg -R path serve --stdio'.

Put another way, two different commands are being run:

ssh -oxyz 'hg -R path serve --stdio'    # Unsafe and incorrect, without `ui.ssh`
ssh -- -oxyz 'hg -R path serve --stdio' # Safe and correct, with `ui.ssh`
    ^^
    ++-- Note "--" separating flags and arguments to `ssh`.

So this is related to runtime configuration, because runtime configuration controls how the ssh subprocess is invoked.

Also, although ui.ssh appears inneffective against the [git] and [svn] variants of subrepos (Mercurial does not appear to populate GIT_SSH or SVN_SSH based on the ui.ssh setting), I can't get hg to actually interact with remotes using hg clone --noupdate ... or hg pull -u -- <uri>, which are the only relevant commands we run. I can get it to interact with remotes with hg up or hg clone (without --noupdate).

Since the "Caveats" section of the subrepo documentation seems to imply this feature might be a bit unevenly implemented (e.g., "Many commands are not aware of subrepos"), maybe these workflows slide under the radar? Or maybe my local hg (3.5.2) doesn't interact with remotes while running these commands, but other versions do?

And here's an extension which appears to be aimed at solving this problem, by adding a new command to execute hg pull -u in subrepositories:

https://www.mercurial-scm.org/wiki/OnsubExtension

Presumably, this extension wouldn't be necessary if hg pull -u could do this on its own.

Likewise, this SO question has several workarounds (including the "onsub" extension) but no answer is like "it works automatically" or "upgrade to 3.5.3":

https://stackoverflow.com/questions/10988009/any-way-to-pull-update-all-subrepos

I think this is related:
https://www.mercurial-scm.org/wiki/Subrepository#Synchronizing_in_subrepositories

Subrepos don't automatically track the latest changeset of their sources. Instead, they are updated to the changeset that corresponds with the changeset checked out in the top-level changeset. This is so developers always get a consistent set of compatible code and libraries when they update.

Thus, updating subrepos is a manual process. Simply run 'hg pull' and 'hg up' in the target subrepo, test in the top-level repo, then commit in the top-level repo to record the new combination. The onsub extension can be used to automate that.

Never mind, I was able to get hg pull -u to interact. I'm going to land, cherry-pick, and hotfix D18390.

The magic incantation I arrived at was slightly modified from one of the hg test cases:

hg init malicious-proxycommand
cd malicious-proxycommand
echo 's = [git]ssh://-oProxyCommand=rm${IFS}non-existent/path' > .hgsub
git init s
cd s
git commit --allow-empty -m 'empty'
cd ..
hg add .hgsub
hg ci -m 'add subrepo'
cd ..
hg clone malicious-proxycommand malicious-proxycommand-clone --noupdate
cd malicious-proxycommand
# Make another commit in the git repo.
# Make another commit in the hg repo.
# If you don't do those, to dirty the repository, the `hg pull` won't touch the subrepo's remote.
cd ../malicious-proxycommand-clone
hg pull -u # If this fails once, it seems to break the working copy by knocking `.hgsub` and `.hgsubstate` out of whack?

From this writeup:

• http://blog.recurity-labs.com/2017-08-10/scm-vulns

...another issue is that SVN will follow an HTTP 301 redirect to an SSH URI. D18389 should, presumably, mitigate this, although prior that change we would not pass SVN_SSH in those cases.

This is somewhat moot since clearly the guidance is now "everyone must upgrade everything".

@indygreg Thanks for the heads up about subrepos -- I would not have otherwise guessed that hg pull might run git.

So, all three major VCS had the exact same CVE, which was "we invoke ssh command line, don't sanitize input, and don't specify -- anywhere"?

Thanks for the detailed explanations! I should have thought more carefully. Note old Mercurial also fails to do correct shell quoting on Windows (It uses ' where Windows needs "). But Phabricator does not run on Windows, it shouldn't be an issue.

In T12961#230931, @avivey wrote:

So, all three major VCS had the exact same CVE, which was "we invoke ssh command line, don't sanitize input, and don't specify -- anywhere"?

Sort of. I think this is a bit more complicated than just a missing "--" because all three allow callers to override what they'll invoke as ssh (with GIT_SSH, SVN_SSH, and ui.ssh). If they just added "--" that would push the responsibility onto users using these options, and realistically many users of these options probably do not parse or handle "--" correctly. Adding "--" at this point would probably also break a lot of existing callers.

That said, only SVN appears to have actually added "--" anywhere, and only to the "internal default" for how SVN_SSH acts, changing it from svn -q to svn -q --: http://subversion.apache.org/security/CVE-2017-9800-advisory.txt

Git should, in a perfect world, add "--" roughly here, at least, I think, before appending the host argument: https://github.com/git/git/blob/master/connect.c#L904

Mercurial should also, in an ideal world, add "--" roughly here, I think, (and presumably do something similar in the windows.py platform module): https://www.mercurial-scm.org/repo/hg/file/tip/mercurial/posix.py#l99

However, doing this without breaking a lot of stuff is probably very difficult. In all cases, it looks like there are many situations where the first half of the command may not be as simple as ssh.

Also, the "--" escaping mechanism itself bears some of the blame here, since it is unsafe by default (it's very easy to write unsafe code without it, that looks correct and works properly until you hit a -unsafe=evil argument) so I don't think projects misusing -- are making a grave or especially preventable error.

To pick one example that I happen to have been able to dig up quickly, git ls-remote didn't even support invocation with -- until January 2016, so you could not safely separate the URI argument from the command body and flags. If git can't get Linux argument handling right, can anyone else be expected to? See T12416 and:

https://secure.phabricator.com/source/phabricator/browse/master/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php;2c150076b00f73918d6b3c86dc28e424317fc826$395-400

Although I believe we are consistent about use of "--" in Phabricator, it is only because I ultimately write pretty much all of the CLI code and am personally very careful about it, which isn't really a security strategy (this was also our XSS "strategy" for a long time, but we're far better off now that rendering is safe by default). And I think this project may get the similar "everything needs @ at the end" convention in Subversion wrong in at least one case, so I hesitate to throw stones.

And even if the projects could get "--" right consistently, it's reasonable not to trust users to be able to. There's also additional mess here because SSH takes a port as a separate -p argument (not part of the URI) which, e.g., basically means Subversion doesn't work with SSH ports without a whole mess of workarounds (see T11781). I believe that at one point Git passed -p but was explicitly documented as not passing it, claim to pass "exactly two arguments" (it looks like this was fixed in March 2013: https://github.com/git/git/commit/e39c695d871d877a621e39cef7e5bd208bb7b157).

Among available defensive options, one conceptual "fix" here is "never use arguments, only flags", so git clone <uri> would become git clone --uri <uri>. It's at least somewhat more difficult for a parser to handle this in an unsafe way which appears to be safe. If POSIX CLI design had been more informed by this security concerns, we could all be doing cd --into src/ safely today! Phabricator largely adopts this convention in server-side commands (although mostly not for security reasons) but not in arc because I'm not completely detached form the real world.

Another might be to change the exec APIs to accept flags and arguments separately, and enforce that -- is built into every command. So this:

csprintf('hg pull -u -- %P', $uri)

...would maybe become:

// Executes as: `hg pull -u -- ssh://...`.
csprintf('hg pull %- %P', array('-u' => null), $uri);

But this is far less readable and I think there are a fairly large number of cases where we must make exceptions, like git blame <rev> -- <file>, git ls-remote (until January 2016), Subversion @ escaping not being covered by this rule, and so on, because argument handling is pretty lax and freeform at the end of the day and different programs do slightly different things at the edges. Offhand, I don't really see a way to harden this API that seems likely to decrease errors with -- without increasing other kinds of errors by at least as much.

See also this enormously valuable contribution I made to the Git LFS upstream in connection with T7789 some time ago:

https://github.com/git-lfs/git-lfs/pull/1096/commits/ea639a53cf56fe371726b0efbf9d92219984562f

I think this is pretty typical of "--" issues: basically, no one ever thinks about "--" until some bizarre edge case arises and I'd guess a majority of engineers using git/hg/svn on a daily basis don't even really know -- is a thing.

(Although maybe I'm selling the profession short since you can't grep for patterns starting with - without at least knowing about -- a little bit?)

The full set of mitigations is now available in stable, and I've promoted 2017 Week 32 (Mid August).

The reason the upstream projects aren't using -- is that it isn't portable. For example, Putty's ssh doesn't support it.

Thanks for the writeup :)

So it looks like the bigger issue isn't that "important coders aren't careful enough re: security", but rather that "current CLI tools are almost impossible to be used in a secure way".
I guess even if we do sanitize the input, ssh might decide one day to parse something new in a way that is unsafe - like deciding host~command/path should run command in some context.

This cropped up in the HN thread -- works in my browsers (although Phabricator does not recognize it as a valid link):

http://-emmawatson.tumblr.com/

As soon as Tumblr adds "clone any blog as a Git repository over SSH", we're in trouble.