From 794e185bf90eefa61cac3a48f41859e1020f90df Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 10 Aug 2017 17:04:11 -0700
Subject: [PATCH] Pass SSH wrappers to VCS commands unconditonally, not just if
 there's an SSH remote

Summary:
Ref T12961. In Mercurial, it's possible to have "subrepos" which may use a different protocol than the main repository.

By putting an SSH repository inside an HTTP repository, an attacker can theoretically get us to execute `hg` without overriding `ui.ssh`, then execute code via the SSH hostname attack.

As an immediate mitigation to this attack, specify `ui.ssh` unconditionally. Normally, this will have no effect (it will just be ignored). In the specific case of an SSH repo inside an HTTP repo, it will defuse the `ssh` protocol.

For good measure and consistency, do the same for Subversion and Git. However, we don't normally maintain working copies for either Subversion or Git so it's unlikely that similar attacks exist there.

Test Plan:
  - Put an SSH subrepo with an attack URI inside an HTTP outer repo in Mercurial.
  - Ran `hg up` with and without `ui.ssh` specified.
  - Got dangerous badness without `ui.ssh` and safe `ssh` subprocesses with `ui.ssh`.

I'm not yet able to confirm that `hg pull -u -- <uri>` can actually trigger this, but this can't hurt and our SSH wrapper is safer than the native behavior for all Subversion, Git and Mercurial versions released prior to today.

Reviewers: chad

Reviewed By: chad

Subscribers: cspeckmim

Maniphest Tasks: T12961

Differential Revision: https://secure.phabricator.com/D18389
---
 .../protocol/DiffusionGitCommandEngine.php         |  4 +---
 .../protocol/DiffusionMercurialCommandEngine.php   | 14 ++++++++------
 .../protocol/DiffusionSubversionCommandEngine.php  |  4 +---
 .../__tests__/DiffusionCommandEngineTestCase.php   |  4 ++--
 4 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/src/applications/diffusion/protocol/DiffusionGitCommandEngine.php b/src/applications/diffusion/protocol/DiffusionGitCommandEngine.php
index a16416ae1d..168b18caa5 100644
--- a/src/applications/diffusion/protocol/DiffusionGitCommandEngine.php
+++ b/src/applications/diffusion/protocol/DiffusionGitCommandEngine.php
@@ -26,9 +26,7 @@ final class DiffusionGitCommandEngine
 
     $env['HOME'] = PhabricatorEnv::getEmptyCWD();
 
-    if ($this->isAnySSHProtocol()) {
-      $env['GIT_SSH'] = $this->getSSHWrapper();
-    }
+    $env['GIT_SSH'] = $this->getSSHWrapper();
 
     if ($this->isAnyHTTPProtocol()) {
       $uri = $this->getURI();
diff --git a/src/applications/diffusion/protocol/DiffusionMercurialCommandEngine.php b/src/applications/diffusion/protocol/DiffusionMercurialCommandEngine.php
index 49c618b085..9499b8f5f1 100644
--- a/src/applications/diffusion/protocol/DiffusionMercurialCommandEngine.php
+++ b/src/applications/diffusion/protocol/DiffusionMercurialCommandEngine.php
@@ -11,12 +11,14 @@ final class DiffusionMercurialCommandEngine
   protected function newFormattedCommand($pattern, array $argv) {
     $args = array();
 
-    if ($this->isAnySSHProtocol()) {
-      $pattern = "hg --config ui.ssh=%s {$pattern}";
-      $args[] = $this->getSSHWrapper();
-    } else {
-      $pattern = "hg {$pattern}";
-    }
+    // NOTE: Here, and in Git and Subversion, we override the SSH command even
+    // if the repository does not use an SSH remote, since our SSH wrapper
+    // defuses an attack against older versions of Mercurial, Git and
+    // Subversion (see T12961) and it's possible to execute this attack
+    // in indirect ways, like by using an SSH subrepo inside an HTTP repo.
+
+    $pattern = "hg --config ui.ssh=%s {$pattern}";
+    $args[] = $this->getSSHWrapper();
 
     return array($pattern, array_merge($args, $argv));
   }
diff --git a/src/applications/diffusion/protocol/DiffusionSubversionCommandEngine.php b/src/applications/diffusion/protocol/DiffusionSubversionCommandEngine.php
index d8e9e6ff17..75b8785a6e 100644
--- a/src/applications/diffusion/protocol/DiffusionSubversionCommandEngine.php
+++ b/src/applications/diffusion/protocol/DiffusionSubversionCommandEngine.php
@@ -44,9 +44,7 @@ final class DiffusionSubversionCommandEngine
   protected function newCustomEnvironment() {
     $env = array();
 
-    if ($this->isAnySSHProtocol()) {
-      $env['SVN_SSH'] = $this->getSSHWrapper();
-    }
+    $env['SVN_SSH'] = $this->getSSHWrapper();
 
     return $env;
   }
diff --git a/src/applications/diffusion/protocol/__tests__/DiffusionCommandEngineTestCase.php b/src/applications/diffusion/protocol/__tests__/DiffusionCommandEngineTestCase.php
index 76d9ad9170..1df3ea3522 100644
--- a/src/applications/diffusion/protocol/__tests__/DiffusionCommandEngineTestCase.php
+++ b/src/applications/diffusion/protocol/__tests__/DiffusionCommandEngineTestCase.php
@@ -26,7 +26,7 @@ final class DiffusionCommandEngineTestCase extends PhabricatorTestCase {
       ));
 
     $this->assertCommandEngineFormat(
-      'hg xyz',
+      (string)csprintf('hg --config ui.ssh=%s xyz', $ssh_wrapper),
       array(
         'LANG' => 'en_US.UTF-8',
         'HGPLAIN' => '1',
@@ -102,7 +102,7 @@ final class DiffusionCommandEngineTestCase extends PhabricatorTestCase {
       ));
 
     $this->assertCommandEngineFormat(
-      'hg xyz',
+      (string)csprintf('hg --config ui.ssh=%s xyz', $ssh_wrapper),
       array(
         'LANG' => 'en_US.UTF-8',
         'HGPLAIN' => '1',