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

[Paul Morelle]

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.

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

What do you think about this?

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

(note: we will need a similar kind of handling to give access to the
obsolete changesets of a server)

--
Paul Morelle