From a7921a4448093d00defa8bd18f35b8c8f8bf3314 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 9 Nov 2017 07:23:40 -0800 Subject: [PATCH] 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 | 2 + .../DiffusionExistsQueryConduitAPIMethod.php | 2 +- .../DiffusionHistoryQueryConduitAPIMethod.php | 18 +++--- ...sionMergedCommitsQueryConduitAPIMethod.php | 2 +- .../DiffusionSearchQueryConduitAPIMethod.php | 2 +- ...ffusionMercurialFlagInjectionException.php | 3 + .../DiffusionMercurialCommandEngine.php | 21 +++++++ .../DiffusionCommandEngineTestCase.php | 56 +++++++++++++++++++ .../DiffusionMercurialRawDiffQuery.php | 4 +- 9 files changed, 97 insertions(+), 13 deletions(-) create mode 100644 src/applications/diffusion/exception/DiffusionMercurialFlagInjectionException.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 7180a7ae04..424d72dc72 100644 --- a/src/__phutil_library_map__.php +++ b/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', 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..e36c0a124e 100644 --- a/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php +++ b/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( 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/exception/DiffusionMercurialFlagInjectionException.php b/src/applications/diffusion/exception/DiffusionMercurialFlagInjectionException.php new file mode 100644 index 0000000000..e60e735816 --- /dev/null +++ b/src/applications/diffusion/exception/DiffusionMercurialFlagInjectionException.php @@ -0,0 +1,3 @@ +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 diff --git a/src/applications/diffusion/protocol/__tests__/DiffusionCommandEngineTestCase.php b/src/applications/diffusion/protocol/__tests__/DiffusionCommandEngineTestCase.php index 1df3ea3522..50b7d4a5e2 100644 --- a/src/applications/diffusion/protocol/__tests__/DiffusionCommandEngineTestCase.php +++ b/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( 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;