Page MenuHomePhabricator

CVE-2022-24765 - Multi-user Git Privilege Escalation
Closed, ResolvedPublic


On a multi-user machine where Alice can write to some arbitrary .git/ directory, she may be able to trick Bailey into executing arbitrary code by writing a .git/config like this:

monitor = evil.exe

...and convincing Baley to execute a git command (perhaps even via PS1 prompt configuration) in the directory containing the malicious .git/.

Git config is not cloned, so Alice can't upload a poisoned Git project and just get the victim to clone it over the internet -- Alice must perform the attack locally on a machine Bailey has access to.

I personally don't live in a world where multi-user escalation is relevant (and Phabricator usually won't, either) and think this isn't too many layers shy of "Alice may write an evil Makefile and trick Bailey into executing arbitrary code by running make in a directory Alice controls", but obviously it's strictly more severe.

Impact on Phabricator/Phacility

Broadly, this has no direct impact on Phabricator, except that the mitigation is for Git to refuse to run when the repository is owned by another user, and this is a common Phabricator configuration (the directory is owned by the "repository user", while the "web user" may run some commands -- but see below).

In particular, this is the Phacility configuration and Ubuntu 20 has patched itself to include the backwards-compatibility-breaking Git mitigation without operator intervention and without a version number change. All parts of this are a bit perplexing to me -- I'd generally prefer my system not break itself undetectably without operator intervention.

Possible fixes may be:

  • We shouldn't really (?) be running on repositories as two different users -- we're always supposed to "sudo" to the repository user -- so maybe this is an actual bug in Phabricator where some pathways are just missing the "sudo" wrapper?
  • Or maybe add --config safe-directories=/path/to/target to every command to just bypass the fix.

See also PHI2188.

Event Timeline

epriestley triaged this task as Normal priority.Apr 13 2022, 5:48 PM
epriestley created this task.

...maybe this is an actual bug in Phabricator where some pathways are just missing the "sudo" wrapper?

I think the story here is a bit complicated. Phabricator may be configured to sudo to a different user (phd.user) when writing to repositories, but currently does not sudo in any cases when reading from repositories. In particular, Conduit queries like diffusion.browsequery do not sudo.

I don't recall the entire history here, but I think this likely supports simple mirror-only use cases, i.e. lets you get Phabricator working and doing something useful without having to touch sudoers configuration. I don't want to casually break this, although the current behavior is a bit informal and probably "less correct" than the rule "always sudo" would be.

Either fix is probably still on the table:

  • Always Sudo: We never sudo by default, and if administrators have configured phd.user that implies that sudo works, and sudo will be used for writes. So any non-broken configuration shouldn't be broken by always using sudo.
  • Mark Directory Safe: DiffusionGitCommandEngine->newFormattedCommand(...) can add --config with a bit of tweaking: it currently doesn't know which directory it will be executed in, but this information is knowable.

There's perhaps an argument that not sudoing for reads is a security feature (i.e., that bypassing this Git check is actually more secure than adding more sudo), since it means more of the attack surface (pathways which can execute git) runs as a user which (maybe) can't write to the repositories. But this is a big maybe (there's no guarantee the web user is actually any less privileged than the daemon user) and kind of way off in hypothetical land anyway.

I think I'm just going to pursue "Always Sudo" since that generally reduces the amount of special behavior we have and I'm reasonably sure it won't break any not-already-broken configurations.

Just for visibility, the error messages you'll see if you're affected by this issue look something like this:

ERR-CONDUIT-CORE: <diffusion.browsequery> Command failed with error #128! COMMAND git ls-tree -z -l ... --
STDOUT (empty)
STDERR fatal: unsafe repository ('/path/to/some/repo' is owned by someone else) To add an exception for this directory, call: git config --global --add ...

D21756 effectively makes all Git pathways call setSudoAsDaemon(true).

I narrowly deployed this patch to an affected Phacility production shard and saw everything start working again. I believe this patch breaks no configurations which were not already broken (configured with a phd.user which the web user can't actually sudo to) and is likely the least-bad approach of available approaches -- notably, it represents an overall reduction in the amount of special-cased behavior. The only downside is a purely theoretical loss of web vs daemon privilege separation which I suspect no real instance is actually impacted by.

(If I turn out to be wrong about this, I believe I could back this out and pursue a not-much-more-complicated --config patch instead.)

Since this seems to be working in practice and I've convinced myself it's reasonable, I'm planning to land this change, cut a release, and deploy it. (However, the baby is about to wake up -- so I may not get this rolling until at least the next time she goes down for a nap.)

I deployed this everywhere in the Phacility cluster yesterday and things have been quiet, so I'm assuming it worked until evidence arises to the contrary.

Just for visibility, this is I believe the change that broke Diffusion (which was fixed in rP52df4ff515b7), where the error message is something like

Unable to Retrieve Paths
Command failed with error #128! COMMAND /usr/bin/sudo -E -n -u phab-daemon -- git config -l -f /tmp/exjghzd2cx4ok04w/60402-8Yau8N STDOUT (empty) STDERR fatal: unable to read config file '/tmp/exjghzd2cx4ok04w/60402-8Yau8N': Permission denied