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

Yuya Nishihara yuya at tcha.org
Fri Jun 22 07:15:53 EDT 2018


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.

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


More information about the Mercurial-devel mailing list