I've updated all Phacility cluster hosts which run any git operations (secure*, sbuild*, saux*, repo*) to 2.8.1.

I'm less sure about adding a warning to arc.

In particular, I'm not sure it will do any good until after T5055, because it's very rare for users to upgrade arc, so we might reasonably not be able to deliver the warning for years. If we had T5055 and had a security channel built in to that I'd say this is definitely worthwhile, but the lumbering pace of arc upgrades means that we might, e.g., write a warning today that users don't see for months, and then maybe Apple or some other vendor backport a 1.8.1b version and the warning is wrong and we're months away from being able to fix it during the next upgrade.

Users are also generally somewhat less vulnerable to this than servers (you can potentially try infinite times against a server, but the game is probably up the first time you have a user clone a repository with a 2GB path in it, I think), and it's not clear that this is RCE-in-practice vs RCE-in-theory.

I think there's also no particularly reasonable way to update Git on OSX until Apple patches it (you'd have to manually install or use Homebrew, and not clear that's even particularly realistic on El Capitan, per above?) and most users don't clone many random repositories, so many might choose not to upgrade even if we could deliver this warning in a timely fashion.

I'm open to other opinions on this if anyone has a different perspective, but am leaning toward waiting for T5055 and building a robust security update channel into that rather than trying to one-off this, primarily because of how incredibly un-timely the message would be for most users.

I think the sequence of action is:

• The attacker runs git push to a hosted repository they have permission to write to, or the server runs git fetch to pull changes from a repository it is observing which has been compromised by an attacker.
• The pushed or fetched change has a 2GB pathname full of virus.exe.
• The server runs arbitrary code during the push/fetch, in the git subprocess, and sends all your private keys to Russian hackers while enrolling the server in a botnet and mining bitcoins.
• Much later on, Herald, Daemons, UI, etc., throw a bunch of exceptions complaining about the filenames.

In particular, we don't get a chance to examine the file names before git does on either a push or a fetch, so I think our poor handling of them later on doesn't impact our vulnerability (except very indirectly by making an attack slightly more obvious).

I got hit by that warning today, and while it doesn't bother me because I knew about the vulnerabilities and versions, it could be confusing for others. I've got 2.1.4-2.1+deb8u2 on Debian, which is old but the fixes are backported. Many other distros have older versions with backported fixes as well (as per https://news.ycombinator.com/item?id=11517894). Unfortunately you cannot see that in git --version though.

Maybe the warning should at least be differently worded to include that information and indicate that it's safe to ignore if you've updated to the latest version even though it's a lower version number?

In other cases (like with Shellshock) we can just test for the vulnerability to see if a binary is vulnerable. Ideally, we'd just perform this test to figure out if you need to upgrade, but I don't think a reasonable test case exists here because we need to create a 2GB pathname and probably can not do that in a reasonable amount of time.

If you ignore this issue, it will currently ignore all future setup issues about git. We should probably change that so that ignoring it only ignores the issue for the specific <current version, known problem> pair. That would mean you had to ignore the issue again for every new minor distro version of Git, though.

We could introduce some new type of ignore for only ignoring the known problem, although this is a lot of effort.

We could also whitelist vendored versions with the patch backported, although this seems like a losing battle to me, although maybe less hopeless after T5055.

We could get rid of all of this and make it our policy not to handle security for dependencies when no empirical test is available to test for the vulnerability.

safe to ignore if you've updated to the latest version even though it's a lower version number

In particular, this is not true: at least one major vendor (Apple) does not have a backported patch.

To make matters worse, Ubuntu 14 (which we run in production) did backport, but git --version reports 1.9.1 for both 1.9.1-1ubuntu0.3 (not vulnerable) and earlier versions of 1.9.1 (vulnerable) so we can't even tell which version is on the system from git --version.

At least for now, maybe I'll remove the setup issue to avoid confusion and just note this in the changelog instead. This is much more likely to be overlooked, but vendors other than Apple seem to have generally taken care of this and using git --version to test for the presence of the vulnerability isn't really meaningful.

Broadly, I lean toward this policy going forward:

• When we can perform an accurate test for the vulnerability in a reasonable amount of time/effort and tell you that you are definitely vulnerable (as with Shellshock), we will continue to do so with an active setup warning.
• When we can not perform such a test (as here), we will publish guidance and note the issue in the changelog, but will not attempt to guess if the installed version may be vulnerable because this test will frequently be confusing/misleading/wrong.
• We can re-evaluate this after T5055, which may give us a wider range of tools for providing more accurate vulnerability notifications.

In T10832#171473, @epriestley wrote:

Broadly, I lean toward this policy going forward:

• When we can perform an accurate test for the vulnerability in a reasonable amount of time/effort and tell you that you are definitely vulnerable (as with Shellshock), we will continue to do so with an active setup warning.
• When we can not perform such a test (as here), we will publish guidance and note the issue in the changelog, but will not attempt to guess if the installed version may be vulnerable because this test will frequently be confusing/misleading/wrong.
• We can re-evaluate this after T5055, which may give us a wider range of tools for providing more accurate vulnerability notifications.

I see what you wrote but I disagree, I included a link on my initial to some of the ressources I used for finding that security threat. It include a sample git repository which can be used to trigger the bug.
You can find that part of the discussion with the download link here.

So you can remove “we do not have a reasonable, conclusive test for the presence of this vulnerability”

Any test we execute must be distributed with phabricator/ and run on the first page load after an upgrade. That generally means it needs to be small, very fast, and completely conclusive. Compare to the test for Shellshock here, which runs in a few milliseconds, requires 10 lines of code, and is totally conclusive:

https://secure.phabricator.com/diffusion/P/browse/master/src/applications/config/check/PhabricatorSecuritySetupCheck.php;00885edc47d46caa9ff10ec133dfe503c07564f0$11-22

I'm not claiming that no conclusive test exists, just that no conclusive test which we can reasonably distribute, execute and evaluate in these conditions is currently known (or, I believe, likely to be discovered). Tests must run very quickly, on the order of 10ms, to be reasonable to execute during Phabricator's startup phase.

Distributing a multi-MB archive file which we decompress into /tmp and clone is not reasonable to execute on the first page load. The crafted repository.7z from that dump requires 17s to clone on my machine, and the failure mode is not conclusive (the git clone could fail if the disk is out of space, for example, even if the version is not vulnerable).