(stable) Promote 2017 Week 45

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);
+      $path_arg = csprintf('%s', $path);
-      $branch_arg = '';
+      $revset_arg = hgsprintf(
+        'reverse(ancestors(%s))',
+        $commit_hash);
     } else {
       $path_arg = '';
-      // NOTE: --branch used to be called --only-branch; use -b for
+      $revset_arg = hgsprintf(
-      // compatibility.
+        'branch(%s) and reverse(ancestors(%s))',
-      $branch_arg = csprintf('-b %s', $drequest->getBranch());
+        $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,
+      $revset_arg,
-      hgsprintf('reverse(ancestors(%s))', $commit_hash),
       $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;