[PATCH 3 of 7] dispatch: rework the serve --stdio safe argument checks

[Yuya Nishihara]

On Mon, 25 Jun 2018 19:51:52 +0200, Paul Morelle wrote:
> On 22/06/18 13:15, Yuya Nishihara wrote:
> > On Thu, 21 Jun 2018 19:46:24 +0200, Paul Morelle wrote:
> >> On 21/06/18 13:53, Yuya Nishihara wrote:
> >>> On Wed, 20 Jun 2018 18:36:24 +0200, Paul Morelle wrote:
> >>>>> # HG changeset patch
> >>>> # User Boris Feld <boris.feld at octobus.net>
> >>>> # Date 1529489906 -7200
> >>>> #      Wed Jun 20 12:18:26 2018 +0200
> >>>> # Node ID 81edf3431b95d57257c690f7fe125c6676a78e18
> >>>> # Parent  b7051e4bf783c844f705473a2396458acecc59dc
> >>>> # EXP-Topic remote-debug
> >>>> # Available At https://bitbucket.org/octobus/mercurial-devel/
> >>>> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 81edf3431b95
> >>>> dispatch: rework the serve --stdio safe argument checks
> >>>>
> >>>> We prepare the code to check for arguments after the mandatory ones.
> >>>>
> >>>> We want to relax this check to allow for wider argument passing in certain
> >>>> conditions (example --debug). We make preliminary refactoring in different
> >>>> changesets for clarity.
> >>>>
> >>>> diff -r b7051e4bf783 -r 81edf3431b95 mercurial/dispatch.py
> >>>> --- a/mercurial/dispatch.py	Wed Jun 20 12:16:48 2018 +0200
> >>>> +++ b/mercurial/dispatch.py	Wed Jun 20 12:18:26 2018 +0200
> >>>> @@ -285,12 +285,15 @@
> >>>>              def unsafe():
> >>>>                  msg = _('potentially unsafe serve --stdio invocation: %s')
> >>>>                  raise error.Abort(msg % (stringutil.pprint(req.args),))
> >>>> -            if (len(req.args) != 4 or
> >>>> +            if (len(req.args) < 4 or
> >>>>                  req.args[0] != '-R' or
> >>>>                  req.args[1].startswith('--') or
> >>>>                  req.args[2] != 'serve' or
> >>>>                  req.args[3] != '--stdio'):
> >>>>                  unsafe()
> >>>> +            other_args = req.args[4:]
> >>>> +            if other_args:
> >>>> +                unsafe()
> >>> It's a bit scary to extend this just for debugging aids because argument
> >>> parsing at this phase has to be ad-hoc. Can't you instead use the
> >>> ssh/authorized_keys file to redirect a server session to 'hg debugserve'?
> >>>
> >>> Alternatively, we could add a wrapper script like hg-ssh.
> >> If I have correctly understood, your proposition is to keep the
> >> client-side version of this, and move the permission management to the
> >> sshkey/ssh-server level. Is this right?
> > Yes.
> >
> >> Something we could do in this area is to replace the call to sshpeer by
> >> `hg debugserve …` when we need the remote-debugging feature.
> >> However, exposing a "debug" command at the wireprotocol level seems bad
> >> ; maybe we could introduce a `--remote-argument` flag that would lift
> >> the check with a permission handling similar to what we have today (in
> >> patch 4).
> >>
> >> However, having all checks disabled (debugserve) is quite different than
> >> what is proposed in this series, which only allows for more information
> >> to be retrieved, and that's it.
> >> Fully bypassing the argument check would allow the client do a full
> >> range of actions (including arbitrary code execution). This is a much
> >> different access level, and in my current use case it would be a problem.
> > Instead of using debugserve, the wrapper script can set ui.debug/traceback
> > flag just like contrib/hg-ssh does for hooks.
> Doing this at the authorized_keys level is not an option for us. In one
> of our target environment for this kind of debugging, any change to the
> ssh script requires validation by admin people that can take from a
> couple of days to a couple of weeks, making it impossible to turn
> debugging on for a couple of commands, or easily compare various settings.

IMHO, that doesn't justify why we have to put everything in hg core.
"hg serve" itself is an SSH script in security POV.

> The ability to control the used debug options from the client is very
> valuable in this case. We have already gathered many useful information
> from it!
> 
> I agree that ad-hoc argument parsing is less than optimal. Handling
> three extra flags (--debug, --profile and --traceback) is not too awful,
> but then we need to pass some associated configuration option (e.g.
> profiling.time-track=real) which will make things even more verbose.
> 
> Maybe we could leverage the contents of `mercurial/configitems.py` by
> adding a "serversafe" attribute to the configuration items? Such config
> item could be specified on the client side (if the user is allowed to).

No, that sounds worse than the original patch. IIUC, the reason why we have
hard-coded check for req.args is that our option parser isn't strict and is
likely to allow command-line injection. Making the whitelist fully configurable
would be dangerous.

> >> I don't think that moving the permission handling outside of Mercurial
> >> would be a good move : implementing similar features for HTTP would
> >> still require some Mercurial-side restrictions and permissions handling
> >> anyway. It seems more consistent to implement it on the Mercurial side
> >> for all protocols.
> > I agree it should be handled at protocol level, and I think it should be
> > a wire command/option rather than the command argument hack. IMHO, it's
> > way safer than constructing/parsing shell command arguments carefully,
> > and HTTP will need that anyway.
> The life cycle of the server is quite different in ssh and http, and
> this has different implications.
> For the ssh client, there are multiple initializations and cachings that
> are worth tracking, so tracking the full run is what is most appropriate
> for our needs.
> For http, the arguments will have to be processed at the hgweb level
> (instead of the protocol level).

I know the situation of HTTP and SSH is slightly different, but how important
it is to catch the early-stage thingy by extending the SSH command-line
interface, which we have to support forever? I think wire-protocol level
implementation will be good enough, and better in the long run.