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

[Paul Morelle]

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.
Hello Yuya,

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?

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.

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.

Cheers,

Paul Morelle