Changeset View
Standalone View
src/applications/diffusion/protocol/DiffusionMercurialCommandEngine.php
<?php | <?php | ||||
final class DiffusionMercurialCommandEngine | final class DiffusionMercurialCommandEngine | ||||
extends DiffusionCommandEngine { | extends DiffusionCommandEngine { | ||||
protected function canBuildForRepository( | protected function canBuildForRepository( | ||||
PhabricatorRepository $repository) { | PhabricatorRepository $repository) { | ||||
return $repository->isHg(); | return $repository->isHg(); | ||||
} | } | ||||
protected function newFormattedCommand($pattern, array $argv) { | protected function newFormattedCommand($pattern, array $argv) { | ||||
$args = array(); | $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()) | |||||
amckinley: I haven't dug into the guts of `PhutilShellLexer`, but is it robust enough to catch people… | |||||
Not Done Inline ActionsIt can't, but if users can control echo ... | echo ... they can presumably just execute wget | sh instead. It only handles the basic quoting rules, but we've applied all the quoting ourselves, and it handles (or, at least, should handle) those properly (see PhutilShellLexerTestCase.php in libphutil/). Or, put another way: if it doesn't handle an input, we almost certainly already have a separate RCE vulnerability with a different source that isn't related to Mercurial. epriestley: It can't, but if users can control `echo ... | echo ...` they can presumably just execute `wget… | |||||
->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)); | |||||
} | |||||
Not Done Inline ActionsWorth logging this so we can detect malicious users? amckinley: Worth logging this so we can detect malicious users? | |||||
Not Done Inline ActionsI don't know what we'd do with this knowledge offhand, but if you want to log it in the cluster you can add a subclass of PhabricatorRequestExceptionHandler to services/. It could do this in canHandleRequestThrowable() to just dump them to the log without interrupting processing. phlog($throwable); return false; epriestley: I don't know what we'd do with this knowledge offhand, but if you want to log it in the cluster… | |||||
} | |||||
// NOTE: Here, and in Git and Subversion, we override the SSH command even | // 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 | // if the repository does not use an SSH remote, since our SSH wrapper | ||||
// defuses an attack against older versions of Mercurial, Git and | // defuses an attack against older versions of Mercurial, Git and | ||||
// Subversion (see T12961) and it's possible to execute this attack | // Subversion (see T12961) and it's possible to execute this attack | ||||
// in indirect ways, like by using an SSH subrepo inside an HTTP repo. | // in indirect ways, like by using an SSH subrepo inside an HTTP repo. | ||||
$pattern = "hg --config ui.ssh=%s {$pattern}"; | $pattern = "hg --config ui.ssh=%s {$pattern}"; | ||||
$args[] = $this->getSSHWrapper(); | $args[] = $this->getSSHWrapper(); | ||||
▲ Show 20 Lines • Show All 51 Lines • Show Last 20 Lines |
I haven't dug into the guts of PhutilShellLexer, but is it robust enough to catch people doing something like echo "--deb" | echo "ugger" or whatever?