From 3125d7a5f48d612c5e97aa2b13acafb50b6a0b9c Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 13 Apr 2022 11:13:31 -0700
Subject: [PATCH] In Git, always "sudo" to the daemon user if a daemon user is
 configured

Summary:
See T13673. Recent versions of Git (and older versions with backported security patches) now refuse to run Git commands if the top-level repository directory is not owned by the user running the command.

Currently, we "sudo" to that user only when performing writes, so upgrading Git can aggressively break a Phabricator system by knocking out essentially all Diffusion/Conduit read pathways.

As an immediate mitigation, just "sudo" in all cases where a daemon user is available. This fixes the problem, and seems like the least-bad approach. The downside is that the web user may theoretically have fewer privileges than the daemon user and this could reduce the number of layers an attacker armed with some other Git vulnerability might have to get through to do something dangerous (e.g., perform a write on a pathway where only reads are expected), but any separation between the web and daemon accounts is essentially theoretical and has never been enforced.

Test Plan: Applied patch to impacted Phacility shard, saw Diffusion work properly again.

Differential Revision: https://secure.phabricator.com/D21756
---
 .../diffusion/protocol/DiffusionCommandEngine.php  |  6 +++++-
 .../protocol/DiffusionGitCommandEngine.php         | 14 ++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/src/applications/diffusion/protocol/DiffusionCommandEngine.php b/src/applications/diffusion/protocol/DiffusionCommandEngine.php
index a0f1adaf1f..db9d930151 100644
--- a/src/applications/diffusion/protocol/DiffusionCommandEngine.php
+++ b/src/applications/diffusion/protocol/DiffusionCommandEngine.php
@@ -117,12 +117,16 @@ abstract class DiffusionCommandEngine extends Phobject {
     return $this->sudoAsDaemon;
   }
 
+  protected function shouldAlwaysSudo() {
+    return false;
+  }
+
   public function newFuture() {
     $argv = $this->newCommandArgv();
     $env = $this->newCommandEnvironment();
     $is_passthru = $this->getPassthru();
 
-    if ($this->getSudoAsDaemon()) {
+    if ($this->getSudoAsDaemon() || $this->shouldAlwaysSudo()) {
       $command = call_user_func_array('csprintf', $argv);
       $command = PhabricatorDaemon::sudoCommandAsDaemonUser($command);
       $argv = array('%C', $command);
diff --git a/src/applications/diffusion/protocol/DiffusionGitCommandEngine.php b/src/applications/diffusion/protocol/DiffusionGitCommandEngine.php
index 995e156d8e..6bc941a6b6 100644
--- a/src/applications/diffusion/protocol/DiffusionGitCommandEngine.php
+++ b/src/applications/diffusion/protocol/DiffusionGitCommandEngine.php
@@ -13,6 +13,20 @@ final class DiffusionGitCommandEngine
     return array($pattern, $argv);
   }
 
+  protected function shouldAlwaysSudo() {
+
+    // See T13673. In Git, always try to use "sudo" to execute commands as the
+    // daemon user (if such a user is configured), because Git 2.35.2 and newer
+    // (and some older versions of Git with backported security patches) refuse
+    // to execute if the top level repository directory is not owned by the
+    // current user.
+
+    // Previously, we used "sudo" only when performing writes to the
+    // repository directory.
+
+    return true;
+  }
+
   protected function newCustomEnvironment() {
     $env = array();