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.
Tags
None
Referenced Files
F12809489: D18769.id45040.diff
Wed, Mar 27, 9:32 PM
Unknown Object (File)
Thu, Mar 14, 4:45 PM
Unknown Object (File)
Thu, Mar 14, 4:45 PM
Unknown Object (File)
Thu, Mar 14, 4:45 PM
Unknown Object (File)
Sun, Mar 10, 8:00 AM
Unknown Object (File)
Feb 17 2024, 5:04 PM
Unknown Object (File)
Feb 11 2024, 4:20 AM
Unknown Object (File)
Feb 3 2024, 5:07 PM
Subscribers
None

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
Branch
hgfix1
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 18821
Build 25368: Run Core Tests
Build 25367: arc lint + arc unit

Event Timeline

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