Patchwork dispatch: also allow "hg serve --stdio" on the current repo

login
register
mail settings
Submitter via Mercurial-devel
Date May 17, 2017, 4:30 a.m.
Message ID <70c51c9d67c69d4fb4cf.1494995453@martinvonz.svl.corp.google.com>
Download mbox | patch
Permalink /patch/20648/
State Rejected
Headers show

Comments

via Mercurial-devel - May 17, 2017, 4:30 a.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1494994835 25200
#      Tue May 16 21:20:35 2017 -0700
# Node ID 70c51c9d67c69d4fb4cfcd5990a8e8906b55f576
# Parent  779a1ae6d0d9eeb487636f665747e92195eb234e
dispatch: also allow "hg serve --stdio" on the current repo

Since 77eaf9539499 (dispatch: protect against malicious 'hg serve
--stdio' invocations (sec), 2017-04-12), we only allow "hg -R <repo>
serve --stdio" (with <repo> not starting with "--"). It seems safe to
also allow it on the current repo (i.e. without "-R <repo>"), so let's
allow that.
Yuya Nishihara - May 17, 2017, 2:34 p.m.
On Tue, 16 May 2017 21:30:53 -0700, Martin von Zweigbergk via Mercurial-devel wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1494994835 25200
> #      Tue May 16 21:20:35 2017 -0700
> # Node ID 70c51c9d67c69d4fb4cfcd5990a8e8906b55f576
> # Parent  779a1ae6d0d9eeb487636f665747e92195eb234e
> dispatch: also allow "hg serve --stdio" on the current repo
> 
> Since 77eaf9539499 (dispatch: protect against malicious 'hg serve
> --stdio' invocations (sec), 2017-04-12), we only allow "hg -R <repo>
> serve --stdio" (with <repo> not starting with "--"). It seems safe to
> also allow it on the current repo (i.e. without "-R <repo>"), so let's
> allow that.

Seems okay to allow it, but how permissive should "hg serve --stdio" be?

If I understand the idea, we only allow the arguments pattern which an sshpeer
could generate.

> --- a/mercurial/dispatch.py
> +++ b/mercurial/dispatch.py
> @@ -227,11 +227,18 @@
>              # shenanigans wherein a user does something like pass
>              # --debugger or --config=ui.debugger=1 as a repo
>              # name. This used to actually run the debugger.

Nit: the comment above needs to be updated.

> -            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'):
> +            safe = False
> +            if (len(req.args) == 2 and
> +                req.args[0] == 'serve' and
> +                req.args[1] == '--stdio'):
> +                safe = True
> +            if (len(req.args) == 4 and
> +                req.args[0] == '-R' and
> +                not req.args[1].startswith('--') and
> +                req.args[2] == 'serve' and
> +                req.args[3] == '--stdio'):
> +                safe = True
Augie Fackler - May 17, 2017, 7:35 p.m.
On Wed, May 17, 2017 at 11:34:12PM +0900, Yuya Nishihara wrote:
> On Tue, 16 May 2017 21:30:53 -0700, Martin von Zweigbergk via Mercurial-devel wrote:
> > # HG changeset patch
> > # User Martin von Zweigbergk <martinvonz@google.com>
> > # Date 1494994835 25200
> > #      Tue May 16 21:20:35 2017 -0700
> > # Node ID 70c51c9d67c69d4fb4cfcd5990a8e8906b55f576
> > # Parent  779a1ae6d0d9eeb487636f665747e92195eb234e
> > dispatch: also allow "hg serve --stdio" on the current repo
> >
> > Since 77eaf9539499 (dispatch: protect against malicious 'hg serve
> > --stdio' invocations (sec), 2017-04-12), we only allow "hg -R <repo>
> > serve --stdio" (with <repo> not starting with "--"). It seems safe to
> > also allow it on the current repo (i.e. without "-R <repo>"), so let's
> > allow that.
>
> Seems okay to allow it, but how permissive should "hg serve --stdio" be?
>
> If I understand the idea, we only allow the arguments pattern which an sshpeer
> could generate.

I'm a little nervous about this. What's the use case for making this
more flexible?

>
> > --- a/mercurial/dispatch.py
> > +++ b/mercurial/dispatch.py
> > @@ -227,11 +227,18 @@
> >              # shenanigans wherein a user does something like pass
> >              # --debugger or --config=ui.debugger=1 as a repo
> >              # name. This used to actually run the debugger.
>
> Nit: the comment above needs to be updated.
>
> > -            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'):
> > +            safe = False
> > +            if (len(req.args) == 2 and
> > +                req.args[0] == 'serve' and
> > +                req.args[1] == '--stdio'):
> > +                safe = True
> > +            if (len(req.args) == 4 and
> > +                req.args[0] == '-R' and
> > +                not req.args[1].startswith('--') and
> > +                req.args[2] == 'serve' and
> > +                req.args[3] == '--stdio'):
> > +                safe = True
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
via Mercurial-devel - May 17, 2017, 8:20 p.m.
On Wed, May 17, 2017 at 12:35 PM, Augie Fackler <raf@durin42.com> wrote:
> On Wed, May 17, 2017 at 11:34:12PM +0900, Yuya Nishihara wrote:
>> On Tue, 16 May 2017 21:30:53 -0700, Martin von Zweigbergk via Mercurial-devel wrote:
>> > # HG changeset patch
>> > # User Martin von Zweigbergk <martinvonz@google.com>
>> > # Date 1494994835 25200
>> > #      Tue May 16 21:20:35 2017 -0700
>> > # Node ID 70c51c9d67c69d4fb4cfcd5990a8e8906b55f576
>> > # Parent  779a1ae6d0d9eeb487636f665747e92195eb234e
>> > dispatch: also allow "hg serve --stdio" on the current repo
>> >
>> > Since 77eaf9539499 (dispatch: protect against malicious 'hg serve
>> > --stdio' invocations (sec), 2017-04-12), we only allow "hg -R <repo>
>> > serve --stdio" (with <repo> not starting with "--"). It seems safe to
>> > also allow it on the current repo (i.e. without "-R <repo>"), so let's
>> > allow that.
>>
>> Seems okay to allow it, but how permissive should "hg serve --stdio" be?
>>
>> If I understand the idea, we only allow the arguments pattern which an sshpeer
>> could generate.
>
> I'm a little nervous about this. What's the use case for making this
> more flexible?

Just that we're using it in narrowhg's tests. Why nervous? Seemed safe
to me. We could easily add "-R ." in the narrowhg tests if we think
it's unlikely that that others would want the same.

>
>>
>> > --- a/mercurial/dispatch.py
>> > +++ b/mercurial/dispatch.py
>> > @@ -227,11 +227,18 @@
>> >              # shenanigans wherein a user does something like pass
>> >              # --debugger or --config=ui.debugger=1 as a repo
>> >              # name. This used to actually run the debugger.
>>
>> Nit: the comment above needs to be updated.
>>
>> > -            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'):
>> > +            safe = False
>> > +            if (len(req.args) == 2 and
>> > +                req.args[0] == 'serve' and
>> > +                req.args[1] == '--stdio'):
>> > +                safe = True
>> > +            if (len(req.args) == 4 and
>> > +                req.args[0] == '-R' and
>> > +                not req.args[1].startswith('--') and
>> > +                req.args[2] == 'serve' and
>> > +                req.args[3] == '--stdio'):
>> > +                safe = True
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
via Mercurial-devel - May 17, 2017, 9:58 p.m.
On Wed, May 17, 2017 at 1:20 PM, Martin von Zweigbergk
<martinvonz@google.com> wrote:
> On Wed, May 17, 2017 at 12:35 PM, Augie Fackler <raf@durin42.com> wrote:
>> On Wed, May 17, 2017 at 11:34:12PM +0900, Yuya Nishihara wrote:
>>> On Tue, 16 May 2017 21:30:53 -0700, Martin von Zweigbergk via Mercurial-devel wrote:
>>> > # HG changeset patch
>>> > # User Martin von Zweigbergk <martinvonz@google.com>
>>> > # Date 1494994835 25200
>>> > #      Tue May 16 21:20:35 2017 -0700
>>> > # Node ID 70c51c9d67c69d4fb4cfcd5990a8e8906b55f576
>>> > # Parent  779a1ae6d0d9eeb487636f665747e92195eb234e
>>> > dispatch: also allow "hg serve --stdio" on the current repo
>>> >
>>> > Since 77eaf9539499 (dispatch: protect against malicious 'hg serve
>>> > --stdio' invocations (sec), 2017-04-12), we only allow "hg -R <repo>
>>> > serve --stdio" (with <repo> not starting with "--"). It seems safe to
>>> > also allow it on the current repo (i.e. without "-R <repo>"), so let's
>>> > allow that.
>>>
>>> Seems okay to allow it, but how permissive should "hg serve --stdio" be?
>>>
>>> If I understand the idea, we only allow the arguments pattern which an sshpeer
>>> could generate.
>>
>> I'm a little nervous about this. What's the use case for making this
>> more flexible?
>
> Just that we're using it in narrowhg's tests. Why nervous? Seemed safe
> to me. We could easily add "-R ." in the narrowhg tests if we think
> it's unlikely that that others would want the same.

I've sent a patch for narrowhg to add the "-R .", so feel free to drop
this patch.

>
>>
>>>
>>> > --- a/mercurial/dispatch.py
>>> > +++ b/mercurial/dispatch.py
>>> > @@ -227,11 +227,18 @@
>>> >              # shenanigans wherein a user does something like pass
>>> >              # --debugger or --config=ui.debugger=1 as a repo
>>> >              # name. This used to actually run the debugger.
>>>
>>> Nit: the comment above needs to be updated.
>>>
>>> > -            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'):
>>> > +            safe = False
>>> > +            if (len(req.args) == 2 and
>>> > +                req.args[0] == 'serve' and
>>> > +                req.args[1] == '--stdio'):
>>> > +                safe = True
>>> > +            if (len(req.args) == 4 and
>>> > +                req.args[0] == '-R' and
>>> > +                not req.args[1].startswith('--') and
>>> > +                req.args[2] == 'serve' and
>>> > +                req.args[3] == '--stdio'):
>>> > +                safe = True
>>> _______________________________________________
>>> Mercurial-devel mailing list
>>> Mercurial-devel@mercurial-scm.org
>>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
via Mercurial-devel - May 22, 2017, 6:16 p.m.
On Wed, May 17, 2017 at 2:58 PM, Martin von Zweigbergk
<martinvonz@google.com> wrote:
> On Wed, May 17, 2017 at 1:20 PM, Martin von Zweigbergk
> <martinvonz@google.com> wrote:
>> On Wed, May 17, 2017 at 12:35 PM, Augie Fackler <raf@durin42.com> wrote:
>>> On Wed, May 17, 2017 at 11:34:12PM +0900, Yuya Nishihara wrote:
>>>> On Tue, 16 May 2017 21:30:53 -0700, Martin von Zweigbergk via Mercurial-devel wrote:
>>>> > # HG changeset patch
>>>> > # User Martin von Zweigbergk <martinvonz@google.com>
>>>> > # Date 1494994835 25200
>>>> > #      Tue May 16 21:20:35 2017 -0700
>>>> > # Node ID 70c51c9d67c69d4fb4cfcd5990a8e8906b55f576
>>>> > # Parent  779a1ae6d0d9eeb487636f665747e92195eb234e
>>>> > dispatch: also allow "hg serve --stdio" on the current repo
>>>> >
>>>> > Since 77eaf9539499 (dispatch: protect against malicious 'hg serve
>>>> > --stdio' invocations (sec), 2017-04-12), we only allow "hg -R <repo>
>>>> > serve --stdio" (with <repo> not starting with "--"). It seems safe to
>>>> > also allow it on the current repo (i.e. without "-R <repo>"), so let's
>>>> > allow that.
>>>>
>>>> Seems okay to allow it, but how permissive should "hg serve --stdio" be?
>>>>
>>>> If I understand the idea, we only allow the arguments pattern which an sshpeer
>>>> could generate.
>>>
>>> I'm a little nervous about this. What's the use case for making this
>>> more flexible?
>>
>> Just that we're using it in narrowhg's tests. Why nervous? Seemed safe
>> to me. We could easily add "-R ." in the narrowhg tests if we think
>> it's unlikely that that others would want the same.
>
> I've sent a patch for narrowhg to add the "-R .", so feel free to drop
> this patch.

I've dropped it from patchworks myself :-)

Patch

diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
--- a/mercurial/dispatch.py
+++ b/mercurial/dispatch.py
@@ -227,11 +227,18 @@ 
             # shenanigans wherein a user does something like pass
             # --debugger or --config=ui.debugger=1 as a repo
             # name. This used to actually run the debugger.
-            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'):
+            safe = False
+            if (len(req.args) == 2 and
+                req.args[0] == 'serve' and
+                req.args[1] == '--stdio'):
+                safe = True
+            if (len(req.args) == 4 and
+                req.args[0] == '-R' and
+                not req.args[1].startswith('--') and
+                req.args[2] == 'serve' and
+                req.args[3] == '--stdio'):
+                safe = True
+            if not safe:
                 raise error.Abort(
                     _('potentially unsafe serve --stdio invocation: %r') %
                     (req.args,))
diff --git a/tests/test-ssh.t b/tests/test-ssh.t
--- a/tests/test-ssh.t
+++ b/tests/test-ssh.t
@@ -369,6 +369,10 @@ 
   $ hg -R narf serv --stdio
   abort: potentially unsafe serve --stdio invocation: ['-R', 'narf', 'serv', '--stdio']
   [255]
+The "-R repo" is not required
+  $ echo heads | hg serve --stdio
+  41
+  0000000000000000000000000000000000000000
 
 Test hg-ssh using a helper script that will restore PYTHONPATH (which might
 have been cleared by a hg.exe wrapper) and invoke hg-ssh with the right