Page MenuHomePhabricator
Files F14745369

D18769.diff
No OneTemporary
Actions

D18769.diff
View Options

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -768,6 +768,7 @@
'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 @@
'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
--- a/src/applications/diffusion/conduit/DiffusionExistsQueryConduitAPIMethod.php
+++ b/src/applications/diffusion/conduit/DiffusionExistsQueryConduitAPIMethod.php
@@ -47,7 +47,7 @@
$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
--- a/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php
+++ b/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php
@@ -123,21 +123,23 @@
// 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
--- a/src/applications/diffusion/conduit/DiffusionMergedCommitsQueryConduitAPIMethod.php
+++ b/src/applications/diffusion/conduit/DiffusionMergedCommitsQueryConduitAPIMethod.php
@@ -77,7 +77,7 @@
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
--- a/src/applications/diffusion/conduit/DiffusionSearchQueryConduitAPIMethod.php
+++ b/src/applications/diffusion/conduit/DiffusionSearchQueryConduitAPIMethod.php
@@ -97,7 +97,7 @@
$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
--- /dev/null
+++ b/src/applications/diffusion/exception/DiffusionMercurialFlagInjectionException.php
@@ -0,0 +1,3 @@
+<?php
+
+final class DiffusionMercurialFlagInjectionException extends Exception {}
diff --git a/src/applications/diffusion/protocol/DiffusionMercurialCommandEngine.php b/src/applications/diffusion/protocol/DiffusionMercurialCommandEngine.php
--- a/src/applications/diffusion/protocol/DiffusionMercurialCommandEngine.php
+++ b/src/applications/diffusion/protocol/DiffusionMercurialCommandEngine.php
@@ -11,6 +11,27 @@
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
diff --git a/src/applications/diffusion/protocol/__tests__/DiffusionCommandEngineTestCase.php b/src/applications/diffusion/protocol/__tests__/DiffusionCommandEngineTestCase.php
--- a/src/applications/diffusion/protocol/__tests__/DiffusionCommandEngineTestCase.php
+++ b/src/applications/diffusion/protocol/__tests__/DiffusionCommandEngineTestCase.php
@@ -123,6 +123,62 @@
'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
--- a/src/applications/diffusion/query/rawdiff/DiffusionMercurialRawDiffQuery.php
+++ b/src/applications/diffusion/query/rawdiff/DiffusionMercurialRawDiffQuery.php
@@ -16,14 +16,14 @@
// 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;

File Metadata

Mime Type
text/plain
Expires
Wed, Jan 22, 9:24 AM (1 h, 36 m)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7027965
Default Alt Text
D18769.diff (9 KB)

Event Timeline

Phabricator · Built by Phacility