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

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

Details

Reviewers

amckinley

Maniphest Tasks

T13012: Mercurial "--config" and "--debugger" command injection vulnerability

Commits

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

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

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley created this revision.Nov 10 2017, 4:22 PM

Harbormaster completed remote builds in B18821: Diff 45040.Nov 10 2017, 4:23 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;