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;