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