Filter and reject "--config" and "--debugger" flags to Mercurial in any position

Summary:
Ref T13012. These flags can be exploited by attackers to execute code remotely. See T13012 for discussion and context.

Additionally, harden some Mercurial commands where possible (by using additional quoting or embedding arguments in other constructs) so they resist these flags and behave properly when passed arguments with these values.

Test Plan:
  - Added unit tests.
  - Verified "--config" and "--debugger" commands are rejected.
  - Verified more commands now work properly even with branches and files named `--debugger`, although not all of them do.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13012

Differential Revision: https://secure.phabricator.com/D18769

src/__phutil_library_map__.php

@@ -768,6 +768,7 @@ phutil_register_library_map(array(
     'DiffusionMercurialBlameQuery' => 'applications/diffusion/query/blame/DiffusionMercurialBlameQuery.php',
     'DiffusionMercurialCommandEngine' => 'applications/diffusion/protocol/DiffusionMercurialCommandEngine.php',
     'DiffusionMercurialFileContentQuery' => 'applications/diffusion/query/filecontent/DiffusionMercurialFileContentQuery.php',
+    'DiffusionMercurialFlagInjectionException' => 'applications/diffusion/exception/DiffusionMercurialFlagInjectionException.php',
     'DiffusionMercurialRawDiffQuery' => 'applications/diffusion/query/rawdiff/DiffusionMercurialRawDiffQuery.php',
     'DiffusionMercurialRequest' => 'applications/diffusion/request/DiffusionMercurialRequest.php',
     'DiffusionMercurialResponse' => 'applications/diffusion/response/DiffusionMercurialResponse.php',
@@ -5814,6 +5815,7 @@ phutil_register_library_map(array(
     'DiffusionMercurialBlameQuery' => 'DiffusionBlameQuery',
     'DiffusionMercurialCommandEngine' => 'DiffusionCommandEngine',
     'DiffusionMercurialFileContentQuery' => 'DiffusionFileContentQuery',
+    'DiffusionMercurialFlagInjectionException' => 'Exception',
     'DiffusionMercurialRawDiffQuery' => 'DiffusionRawDiffQuery',
     'DiffusionMercurialRequest' => 'DiffusionRequest',
     'DiffusionMercurialResponse' => 'AphrontResponse',

src/applications/diffusion/conduit/DiffusionExistsQueryConduitAPIMethod.php

@@ -47,7 +47,7 @@ final class DiffusionExistsQueryConduitAPIMethod
     $commit = $request->getValue('commit');
     list($err, $stdout) = $repository->execLocalCommand(
       'id --rev %s',
-      $commit);
+      hgsprintf('%s', $commit));
     return !$err;
   }
 

src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php

@@ -123,21 +123,23 @@ final class DiffusionHistoryQueryConduitAPIMethod
     // branches).
 
     if (strlen($path)) {
-      $path_arg = csprintf('-- %s', $path);
-      $branch_arg = '';
+      $path_arg = csprintf('%s', $path);
+      $revset_arg = hgsprintf(
+        'reverse(ancestors(%s))',
+        $commit_hash);
     } else {
       $path_arg = '';
-      // NOTE: --branch used to be called --only-branch; use -b for
-      // compatibility.
-      $branch_arg = csprintf('-b %s', $drequest->getBranch());
+      $revset_arg = hgsprintf(
+        'branch(%s) and reverse(ancestors(%s))',
+        $drequest->getBranch(),
+        $commit_hash);
     }
 
     list($stdout) = $repository->execxLocalCommand(
-      'log --debug --template %s --limit %d %C --rev %s %C',
+      'log --debug --template %s --limit %d --rev %s -- %C',
       '{node};{parents}\\n',
       ($offset + $limit), // No '--skip' in Mercurial.
-      $branch_arg,
-      hgsprintf('reverse(ancestors(%s))', $commit_hash),
+      $revset_arg,
       $path_arg);
 
     $stdout = DiffusionMercurialCommandEngine::filterMercurialDebugOutput(

src/applications/diffusion/conduit/DiffusionMergedCommitsQueryConduitAPIMethod.php

@@ -77,7 +77,7 @@ final class DiffusionMergedCommitsQueryConduitAPIMethod
     list($parents) = $repository->execxLocalCommand(
       'parents --template=%s --rev %s',
       '{node}\\n',
-      $commit);
+      hgsprintf('%s', $commit));
     $parents = explode("\n", trim($parents));
 
     if (count($parents) < 2) {

src/applications/diffusion/conduit/DiffusionSearchQueryConduitAPIMethod.php

@@ -97,7 +97,7 @@ final class DiffusionSearchQueryConduitAPIMethod
 
     $results = array();
     $future = $repository->getLocalCommandFuture(
-      'grep --rev %s --print0 --line-number %s %s',
+      'grep --rev %s --print0 --line-number -- %s %s',
       hgsprintf('ancestors(%s)', $drequest->getStableCommit()),
       $grep,
       $path);

src/applications/diffusion/exception/DiffusionMercurialFlagInjectionException.php

@@ -0,0 +1,3 @@
+<?php
+
+final class DiffusionMercurialFlagInjectionException extends Exception {}

src/applications/diffusion/protocol/DiffusionMercurialCommandEngine.php

@@ -11,6 +11,27 @@ final class DiffusionMercurialCommandEngine
   protected function newFormattedCommand($pattern, array $argv) {
     $args = array();
 
+    // Crudely blacklist commands which look like they may contain command
+    // injection via "--config" or "--debugger". See T13012. To do this, we
+    // print the whole command, parse it using shell rules, then examine each
+    // argument to see if it looks like "--config" or "--debugger".
+
+    $test_command = call_user_func_array(
+      'csprintf',
+      array_merge(array($pattern), $argv));
+    $test_args = id(new PhutilShellLexer())
+      ->splitArguments($test_command);
+
+    foreach ($test_args as $test_arg) {
+      if (preg_match('/^--(config|debugger)/i', $test_arg)) {
+        throw new DiffusionMercurialFlagInjectionException(
+          pht(
+            'Mercurial command appears to contain unsafe injected "--config" '.
+            'or "--debugger": %s',
+            $test_command));
+      }
+    }
+
     // 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

src/applications/diffusion/protocol/__tests__/DiffusionCommandEngineTestCase.php

@@ -123,6 +123,62 @@ final class DiffusionCommandEngineTestCase extends PhabricatorTestCase {
         'argv' => 'xyz',
         'protocol' => 'https',
       ));
+
+    // Test that filtering defenses for "--config" and "--debugger" flag
+    // injections in Mercurial are functional. See T13012.
+
+    $caught = null;
+    try {
+      $this->assertCommandEngineFormat(
+        '',
+        array(),
+        array(
+          'vcs' => $type_hg,
+          'argv' => '--debugger',
+        ));
+    } catch (DiffusionMercurialFlagInjectionException $ex) {
+      $caught = $ex;
+    }
+
+    $this->assertTrue(
+      ($caught instanceof DiffusionMercurialFlagInjectionException),
+      pht('Expected "--debugger" injection in Mercurial to throw.'));
+
+
+    $caught = null;
+    try {
+      $this->assertCommandEngineFormat(
+        '',
+        array(),
+        array(
+          'vcs' => $type_hg,
+          'argv' => '--config=x',
+        ));
+    } catch (DiffusionMercurialFlagInjectionException $ex) {
+      $caught = $ex;
+    }
+
+    $this->assertTrue(
+      ($caught instanceof DiffusionMercurialFlagInjectionException),
+      pht('Expected "--config" injection in Mercurial to throw.'));
+
+    $caught = null;
+    try {
+      $this->assertCommandEngineFormat(
+        '',
+        array(),
+        array(
+          'vcs' => $type_hg,
+          'argv' => (string)csprintf('%s', '--config=x'),
+        ));
+    } catch (DiffusionMercurialFlagInjectionException $ex) {
+      $caught = $ex;
+    }
+
+    $this->assertTrue(
+      ($caught instanceof DiffusionMercurialFlagInjectionException),
+      pht('Expected quoted "--config" injection in Mercurial to throw.'));
+
   }
 
   private function assertCommandEngineFormat(

src/applications/diffusion/query/rawdiff/DiffusionMercurialRawDiffQuery.php

@@ -16,14 +16,14 @@ final class DiffusionMercurialRawDiffQuery extends DiffusionRawDiffQuery {
       // If `$commit` has no parents (usually because it's the first commit
       // in the repository), we want to diff against `null`. This revset will
       // do that for us automatically.
-      $against = '('.$commit.'^ or null)';
+      $against = hgsprintf('(%s^ or null)', $commit);
     }
 
     $future = $repository->getLocalCommandFuture(
       'diff -U %d --git --rev %s --rev %s -- %s',
       $this->getLinesOfContext(),
       $against,
-      $commit,
+      hgsprintf('%s', $commit),
       $path);
 
     return $future;