Mercurial "--config" and "--debugger" command injection vulnerability
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	epriestley
	Nov 9 2017, 2:50 PM

Description

Summary

Phabricator uses the hg command line binary to serve some requests for information about Mercurial repositories.

The hg binary treats the flags --config=... and --debugger specially, and evaluates them as flags in any position. Attackers could exploit this by accessing certain branches or files with names like --config=... or --debugger to execute code remotely.

You are vulnerable if:

the Phabricator server has hg installed; and
an attacker can browse any Mercurial repository in Diffusion.

The mitigation (available in 2017 Week 45) refuses to execute any hg command where any argument begins with --config or --debugger.

Mercurial anticipates providing a "hardened" mode in a future release with stricter argument parsing rules.

This issue was reported to us via HackerOne. See https://hackerone.com/reports/288704 (this may not yet be public, but should become public once the researcher agrees to disclosure).

Mercurial Behavior

Mercurial parses the special --config and --debugger flags in an unusual way: it processes them before processing other flags and arguments, including flags which take arguments and the -- "end of flags" marker. For example, this command:

$ hg log -- --debugger

...invokes the debugger. Likewise, this:

$ hg log --branch --debugger

...means "invoke the debugger", not "log the branch named --debugger".

Note that --debugger is a valid Mercurial branch name, but to create it you must do this:

$ hg branch --debugger --debugger

Then enter c to convince the debugger to continue execution.

The --debugger flag is dangerous (remote code execution) if users control stdin. See CVE-2017-9462 for an earlier attack using this flag. See also D18611 and followup in D18616.

The --config flag is also dangerous and can also enable remote code execution with, e.g., --config=hooks.pre-log=whoami in a log command.

Broadly, the behavior of Mercurial is this:

The argument --debugger, and any argument beginning with --config, are unsafe to pass to Mercurial as part of any command, in any position, even after a -- "end of flags" marker. Callers must filter arguments and reject any such argument before passing it to hg. Generally, the hg binary can not interact normally with files named --debugger, branches named --debugger, bookmarks named --debugger, search for the pattern --debugger using grep, etc., even though this is a valid file name, a valid branch name, a valid bookmark name, a valid pattern, and so on. Likewise for --config.

See also CVE-2017-1000116, although this is not as closely related as the --debugger vulnerability.

Phabricator Behavior

Phabricator incorrectly assumed that Mercurial used standard behavior for all flags, and executed some commands like this:

hg log --branch <branch_name> --rev <revset>

An attacker could browse a branch named something like --config=execute=evil (which is a valid Mercurial branch name) in the web UI, and cause Phabricator to execute an hg command with --branch '--config=...'. Normally, binaries parse the second argument as a parameter for the --branch argument, not a separate flag, but Mercurial treats --config and --debugger specially and parses them as flags in any position.

Since there is no way to escape these flags, we now filter them and refuse to execute commands with any --config or --debugger argument.

Revisions and Commits

rP Phabricator
	D18769	rPa7921a444809 Filter and reject "--config" and "--debugger" flags to Mercurial in any position

Related Objects

Mentioned In: T13491: Why does Arcanist require "--"?
T13098: Plans: Arcanist toolsets and extensions
rPHU3215e4e291ed: Allow certain flags to be required to appear before non-flag arguments
D19675: Allow certain flags to be required to appear before non-flag arguments
2017 Week 45 (Mid November)
Mentioned Here: D18611: Provide an explicit "-R" flag to "hg serve"
D18616: Swap "-R" and "serve" argument order for Mercurial

Event Timeline

epriestley created this task.Nov 9 2017, 2:50 PM

From @durin42, via Mercurial Security:

We'll get back to you today or tomorrow on this.

In reply:

Great, thanks! In responding to this, I've also found simpler, somewhat related behavior:

$ hg log -- --debugger

This invokes the debugger, in defiance of "--". I can't find any way to safely escape paths to commands like this.

(We have a bad, kludgy, blacklist-based patch ready to go whenever you make a call.)

Thanks,
Evan

I have a not-so-great patch for this ready when Mercurial makes a decision. This isn't great, but it's the best I could come up with after trying a few things.

The major defense is that before we execute any hg command, we generate the string for it, parse that string as a shell command, and then examine each argument. If any argument begins --config or --debugger, we reject the command.

Since I believe we never add --config or --debugger ourselves, this shouldn't have false positives. (Technically, we add --config, but after doing this check.)

Additionally, I've rewritten some commands into "less vulnerable" forms, where arguments are escaped more aggressively or included in compound revset expressions. This makes inputs like --debugger (which is a perfectly valid branch name and also a perfectly valid path name) work more consistently, although some commands seem to be impossible to escape in the most general case (like hg log -- --debugger).

commit f937c4621bb4c1c8c5661557b38d9a067b741bc8
Author: epriestley <git@epriestley.com>
Date:   Thu Nov 9 07:23:40 2017 -0800

    WIP

diff --git a/src/applications/diffusion/conduit/DiffusionExistsQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionExistsQueryConduitAPIMethod.php
index 4280230002..2d4a221171 100644
--- a/src/applications/diffusion/conduit/DiffusionExistsQueryConduitAPIMethod.php
+++ b/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;
   }
 
diff --git a/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php
index 200be8567b..145ec261bb 100644
--- a/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php
+++ b/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php
@@ -123,21 +123,26 @@ 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 {
+      // NOTE: This is a crude workaround for the Mercurial flag parsing issue
+      // described in T13012.
+
       $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(
diff --git a/src/applications/diffusion/conduit/DiffusionMergedCommitsQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionMergedCommitsQueryConduitAPIMethod.php
index 9d2d6caadc..79587a2e5e 100644
--- a/src/applications/diffusion/conduit/DiffusionMergedCommitsQueryConduitAPIMethod.php
+++ b/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) {
diff --git a/src/applications/diffusion/conduit/DiffusionSearchQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionSearchQueryConduitAPIMethod.php
index 389f931c0f..af973f102d 100644
--- a/src/applications/diffusion/conduit/DiffusionSearchQueryConduitAPIMethod.php
+++ b/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);
diff --git a/src/applications/diffusion/protocol/DiffusionMercurialCommandEngine.php b/src/applications/diffusion/protocol/DiffusionMercurialCommandEngine.php
index 9499b8f5f1..f154413d80 100644
--- a/src/applications/diffusion/protocol/DiffusionMercurialCommandEngine.php
+++ b/src/applications/diffusion/protocol/DiffusionMercurialCommandEngine.php
@@ -11,6 +11,26 @@ 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_pattern = "hg {$pattern}";
+    $test_command = vcsprintf($test_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 Exception(
+          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
diff --git a/src/applications/diffusion/query/rawdiff/DiffusionMercurialRawDiffQuery.php b/src/applications/diffusion/query/rawdiff/DiffusionMercurialRawDiffQuery.php
index 39eed01226..9edcbb6380 100644
--- a/src/applications/diffusion/query/rawdiff/DiffusionMercurialRawDiffQuery.php
+++ b/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;

The other major thing I tried was throwing exceptions when the values for %s, etc., contained --debugger or --config while constructing Mercurial commands.

However, this was involved and not very effective. We construct several commands in pieces, and there's no way to know that a given piece is ultimately becoming a Mercurial command.

amckinley added a subscriber: amckinley.Nov 9 2017, 10:18 PM

epriestley renamed this task from Mercurial "--config" command injection vulnerability to Mercurial "--config" and "--debugger" command injection vulnerability.Nov 10 2017, 3:42 PM

epriestley updated the task description. (Show Details)

epriestley shifted this object from the Restricted Space space to the S1 Core space.Nov 10 2017, 3:48 PM

epriestley changed the visibility from "Public (No Login Required)" to "Custom Policy".

epriestley edited projects, added Security, Mercurial; removed Phacility, Ops.

epriestley added a subscriber: durin42.

(I change visibility for this to @epriestley, @amckinley and @durin42 for now -- note that I intend to eventually make this issue public once the fix hits the commit log, so don't stockpile all your 0-days here.)

From @durin42, via Mercurial Security:

Okay, so here's the plan: this isn't *quite* a vulnerability in hg itself, but it is definitely a headache for people that want to shell out to hg. We're going to add (on the stable branch, so in 4.4.2, which will be December 1st.) a "hardened" mode that adds some paranoia around flags parsing and the like - it'll break some people, due to some annoying details (you won't be able to load extensions with a --config flag in hardened mode), so it won't be on by default, but we plan to make it accessible both via a config setting and an environment variable.

If you want to wait and do a coordinated release, you're welcome to. Might be nice in case other tools or services have the same bug so we can help them out too.

I'd also be happy to review your mitigation if you want, in case that'd be helpful.

I don't want to leave RCE in Phabricator for 3 weeks, so I'm planning to land, deploy and disclose some version of the patch above today.

We'll also use the "hardened" mode as soon as it's available.

A rough version of the mitigation patch is above, with a brief description. Essentially, we'll refuse to execute any command which has any argument beginning with --config or --debugger in any position. I'll formally attach a patch shortly.

epriestley added a revision: D18769: Filter and reject "--config" and "--debugger" flags to Mercurial in any position.Nov 10 2017, 4:22 PM

epriestley added a commit: rPa7921a444809: Filter and reject "--config" and "--debugger" flags to Mercurial in any position.Nov 10 2017, 4:42 PM

epriestley changed the visibility from "Custom Policy" to "Public (No Login Required)".Nov 10 2017, 4:42 PM

Herald added subscribers: cburroughs, cspeckmim, andypuettmann and 2 others. · View Herald TranscriptNov 10 2017, 4:42 PM

This is now in master so I've made the task public.

epriestley mentioned this in 2017 Week 45 (Mid November).Nov 10 2017, 4:45 PM

epriestley updated the task description. (Show Details)

epriestley updated the task description. (Show Details)Nov 10 2017, 4:58 PM

epriestley updated the task description. (Show Details)Nov 10 2017, 5:03 PM

epriestley updated the task description. (Show Details)

We'll use the hardened mode once it's available, but I don't think we expect to take any further action here until then.

https://phab.mercurial-scm.org/D1483 should make it possible to use -- to defend against non-flag user input. For inputs that are flags, use the form --flag=X and avoid --flag X.

Whether --config=X in hg foo --bar --config=X should be an argument of --bar or not is a chicken-egg problem:

--bar may take one or zero argument, which is unknown until all extensions are loaded and define their commands
--config extensions.name= could change what extensions to load

In theory, you could require --config appear between hg and foo in hg foo .... This is already a valid position for --config (for example, hg --config x=y foo is valid), and already not a valid position for foo flags (for example, hg --branch default log is not valid).

This would break existing hg foo --config x=y ... commands and require they be rewritten as hg --config x=y foo ..., but would disambiguate parsing. But perhaps these commands are sufficiently unusual that this compatibility break is reasonable (presumably, normal users rarely type hg log --config x=y or similar, at least).

That's a good point! I wish it was designed like that since the beginning. I guess it won't happen with the current compatibility rules since it is likely to break automation.

EDIT: https://www.mercurial-scm.org/repo/hg-committed/rev/02845f7441af is the fix for the --bar --config=X issue.

epriestley mentioned this in D19675: Allow certain flags to be required to appear before non-flag arguments.Sep 14 2018, 6:50 PM

epriestley mentioned this in rPHU3215e4e291ed: Allow certain flags to be required to appear before non-flag arguments.Sep 15 2018, 3:00 PM

epriestley mentioned this in T13098: Plans: Arcanist toolsets and extensions.Oct 16 2018, 2:54 PM

epriestley mentioned this in T13491: Why does Arcanist require "--"?.Feb 15 2020, 5:40 PM