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… | |||||
epriestleyAuthorUnsubmitted 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)); | |||||
| } | |||||
amckinleyUnsubmitted Not Done Inline ActionsWorth logging this so we can detect malicious users? amckinley: Worth logging this so we can detect malicious users? | |||||
epriestleyAuthorUnsubmitted 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?