Page MenuHomePhabricator

Filter and reject "--config" and "--debugger" flags to Mercurial in any position
ClosedPublic

Authored by epriestley on Nov 10 2017, 4:22 PM.

Details

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.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Nov 10 2017, 4:22 PM
epriestley requested review of this revision.Nov 10 2017, 4:23 PM
amckinley accepted this revision.Nov 10 2017, 4:33 PM
amckinley added inline comments.
src/applications/diffusion/protocol/DiffusionMercurialCommandEngine.php
22

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?

27–32

Worth logging this so we can detect malicious users?

This revision is now accepted and ready to land.Nov 10 2017, 4:33 PM
epriestley added inline comments.Nov 10 2017, 4:41 PM
src/applications/diffusion/protocol/DiffusionMercurialCommandEngine.php
22

It 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.

27–32

I 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;
This revision was automatically updated to reflect the committed changes.