Patchwork [V2] patchbomb: protect email addresses from shell

login
register
mail settings
Submitter Floris Bruynooghe
Date Sept. 30, 2019, 7:22 p.m.
Message ID <7c69ab0787ac4931a5d4.1569871337@powell.devork.be>
Download mbox | patch
Permalink /patch/41857/
State Superseded
Headers show

Comments

Floris Bruynooghe - Sept. 30, 2019, 7:22 p.m.
# HG changeset patch
# User Floris Bruynooghe <flub@google.com>
# Date 1569794518 -7200
#      Mon Sep 30 00:01:58 2019 +0200
# Node ID 7c69ab0787ac4931a5d4568704eb88e86d9c6b83
# Parent  bbf77341a956b3ba40ae87538fcd200b34c5a3e5
patchbomb: protect email addresses from shell

When patchbomb sends email via a sendmail-like program it invokes this
using procutil.popen which passes the string to a shell to be parsed.
To protect any special characters in the email addresses on the
command line from being interpretered by the shell they must be
quoted.
Yuya Nishihara - Sept. 30, 2019, 11:15 p.m.
On Mon, 30 Sep 2019 21:22:17 +0200, Floris Bruynooghe wrote:
> # HG changeset patch
> # User Floris Bruynooghe <flub@google.com>
> # Date 1569794518 -7200
> #      Mon Sep 30 00:01:58 2019 +0200
> # Node ID 7c69ab0787ac4931a5d4568704eb88e86d9c6b83
> # Parent  bbf77341a956b3ba40ae87538fcd200b34c5a3e5
> patchbomb: protect email addresses from shell

> diff --git a/mercurial/mail.py b/mercurial/mail.py
> --- a/mercurial/mail.py
> +++ b/mercurial/mail.py
> @@ -152,7 +152,8 @@ def _smtp(ui):
>  def _sendmail(ui, sender, recipients, msg):
>      '''send mail using sendmail.'''
>      program = ui.config('email', 'method')
> -    stremail = lambda x: stringutil.email(encoding.strtolocal(x))
> +    stremail = lambda x: \
> +        procutil.quote(stringutil.email(encoding.strtolocal(x)))

procutil.shellquote() ?

I don't think shlex supports byte strings on Python 3, and maybe it wouldn't
respect the cmd.exe rule on Windows.
Floris Bruynooghe - Oct. 2, 2019, 7:35 p.m.
On Tue 01 Oct 2019 at 08:15 +0900, Yuya Nishihara wrote:

> On Mon, 30 Sep 2019 21:22:17 +0200, Floris Bruynooghe wrote:
>> # HG changeset patch
>> # User Floris Bruynooghe <flub@google.com>
>> # Date 1569794518 -7200
>> #      Mon Sep 30 00:01:58 2019 +0200
>> # Node ID 7c69ab0787ac4931a5d4568704eb88e86d9c6b83
>> # Parent  bbf77341a956b3ba40ae87538fcd200b34c5a3e5
>> patchbomb: protect email addresses from shell
>
>> diff --git a/mercurial/mail.py b/mercurial/mail.py
>> --- a/mercurial/mail.py
>> +++ b/mercurial/mail.py
>> @@ -152,7 +152,8 @@ def _smtp(ui):
>>  def _sendmail(ui, sender, recipients, msg):
>>      '''send mail using sendmail.'''
>>      program = ui.config('email', 'method')
>> -    stremail = lambda x: stringutil.email(encoding.strtolocal(x))
>> +    stremail = lambda x: \
>> +        procutil.quote(stringutil.email(encoding.strtolocal(x)))
>
> procutil.shellquote() ?

Sure, I should have spotted that...

> I don't think shlex supports byte strings on Python 3, and maybe it wouldn't
> respect the cmd.exe rule on Windows.

Hmm, I don't think I follow everything correctly.  encoding.strtolocal()
returns bytes?  But stringutils.email() uses .find('>'), that is on py3
this is b'foo'.find('<') and thus mixes unicode and bytes.  AFAIK that
can't work so that means I'm missing something.

It's also far from obvious to me that procutil.shellquote can handle
bytes.  But than they also get %-substituted back into the cmdline.
Yuya Nishihara - Oct. 2, 2019, 11:09 p.m.
On Wed, 02 Oct 2019 21:35:52 +0200, Floris Bruynooghe wrote:
> On Tue 01 Oct 2019 at 08:15 +0900, Yuya Nishihara wrote:
> > On Mon, 30 Sep 2019 21:22:17 +0200, Floris Bruynooghe wrote:
> >> # HG changeset patch
> >> # User Floris Bruynooghe <flub@google.com>
> >> # Date 1569794518 -7200
> >> #      Mon Sep 30 00:01:58 2019 +0200
> >> # Node ID 7c69ab0787ac4931a5d4568704eb88e86d9c6b83
> >> # Parent  bbf77341a956b3ba40ae87538fcd200b34c5a3e5
> >> patchbomb: protect email addresses from shell
> >
> >> diff --git a/mercurial/mail.py b/mercurial/mail.py
> >> --- a/mercurial/mail.py
> >> +++ b/mercurial/mail.py
> >> @@ -152,7 +152,8 @@ def _smtp(ui):
> >>  def _sendmail(ui, sender, recipients, msg):
> >>      '''send mail using sendmail.'''
> >>      program = ui.config('email', 'method')
> >> -    stremail = lambda x: stringutil.email(encoding.strtolocal(x))
> >> +    stremail = lambda x: \
> >> +        procutil.quote(stringutil.email(encoding.strtolocal(x)))
> >
> > procutil.shellquote() ?
> 
> Sure, I should have spotted that...
> 
> > I don't think shlex supports byte strings on Python 3, and maybe it wouldn't
> > respect the cmd.exe rule on Windows.
> 
> Hmm, I don't think I follow everything correctly.  encoding.strtolocal()
> returns bytes?

Yes.

> But stringutils.email() uses .find('>'), that is on py3
> this is b'foo'.find('<') and thus mixes unicode and bytes.

We have code transformer which basically rewrites every '' to b''.

https://www.mercurial-scm.org/wiki/Python3#Source_Rewriting_Module_Importer

Anyway, this patch has already been queued, and I sent a follow-up patch mostly
identical to your V3, thanks.
Floris Bruynooghe - Oct. 5, 2019, 10:12 a.m.
Thanks for the explanation and merging the fix!

On Thu, 3 Oct 2019, 00:09 Yuya Nishihara, <yuya@tcha.org> wrote:

> On Wed, 02 Oct 2019 21:35:52 +0200, Floris Bruynooghe wrote:
> > On Tue 01 Oct 2019 at 08:15 +0900, Yuya Nishihara wrote:
> > > On Mon, 30 Sep 2019 21:22:17 +0200, Floris Bruynooghe wrote:
> > >> # HG changeset patch
> > >> # User Floris Bruynooghe <flub@google.com>
> > >> # Date 1569794518 -7200
> > >> #      Mon Sep 30 00:01:58 2019 +0200
> > >> # Node ID 7c69ab0787ac4931a5d4568704eb88e86d9c6b83
> > >> # Parent  bbf77341a956b3ba40ae87538fcd200b34c5a3e5
> > >> patchbomb: protect email addresses from shell
> > >
> > >> diff --git a/mercurial/mail.py b/mercurial/mail.py
> > >> --- a/mercurial/mail.py
> > >> +++ b/mercurial/mail.py
> > >> @@ -152,7 +152,8 @@ def _smtp(ui):
> > >>  def _sendmail(ui, sender, recipients, msg):
> > >>      '''send mail using sendmail.'''
> > >>      program = ui.config('email', 'method')
> > >> -    stremail = lambda x: stringutil.email(encoding.strtolocal(x))
> > >> +    stremail = lambda x: \
> > >> +        procutil.quote(stringutil.email(encoding.strtolocal(x)))
> > >
> > > procutil.shellquote() ?
> >
> > Sure, I should have spotted that...
> >
> > > I don't think shlex supports byte strings on Python 3, and maybe it
> wouldn't
> > > respect the cmd.exe rule on Windows.
> >
> > Hmm, I don't think I follow everything correctly.  encoding.strtolocal()
> > returns bytes?
>
> Yes.
>
> > But stringutils.email() uses .find('>'), that is on py3
> > this is b'foo'.find('<') and thus mixes unicode and bytes.
>
> We have code transformer which basically rewrites every '' to b''.
>
> https://www.mercurial-scm.org/wiki/Python3#Source_Rewriting_Module_Importer
>
> Anyway, this patch has already been queued, and I sent a follow-up patch
> mostly
> identical to your V3, thanks.
>
>

Patch

diff --git a/mercurial/mail.py b/mercurial/mail.py
--- a/mercurial/mail.py
+++ b/mercurial/mail.py
@@ -152,7 +152,8 @@  def _smtp(ui):
 def _sendmail(ui, sender, recipients, msg):
     '''send mail using sendmail.'''
     program = ui.config('email', 'method')
-    stremail = lambda x: stringutil.email(encoding.strtolocal(x))
+    stremail = lambda x: \
+        procutil.quote(stringutil.email(encoding.strtolocal(x)))
     cmdline = '%s -f %s %s' % (program, stremail(sender),
                                ' '.join(map(stremail, recipients)))
     ui.note(_('sending mail: %s\n') % cmdline)
diff --git a/mercurial/pycompat.py b/mercurial/pycompat.py
--- a/mercurial/pycompat.py
+++ b/mercurial/pycompat.py
@@ -328,8 +328,11 @@  if ispy3:
         ret = shlex.split(s.decode('latin-1'), comments, posix)
         return [a.encode('latin-1') for a in ret]
 
+    shlexquote = shlex.quote
+
 else:
     import cStringIO
+    import pipes
 
     xrange = xrange
     unicode = unicode
@@ -393,6 +396,7 @@  else:
     sysplatform = sys.platform
     sysexecutable = sys.executable
     shlexsplit = shlex.split
+    shlexquote = pipes.quote
     bytesio = cStringIO.StringIO
     stringio = bytesio
     maplist = map
diff --git a/mercurial/utils/procutil.py b/mercurial/utils/procutil.py
--- a/mercurial/utils/procutil.py
+++ b/mercurial/utils/procutil.py
@@ -70,6 +70,7 @@  shellsplit = platform.shellsplit
 spawndetached = platform.spawndetached
 sshargs = platform.sshargs
 testpid = platform.testpid
+quote = pycompat.shlexquote
 
 try:
     setprocname = osutil.setprocname
diff --git a/tests/test-patchbomb.t b/tests/test-patchbomb.t
--- a/tests/test-patchbomb.t
+++ b/tests/test-patchbomb.t
@@ -3035,6 +3035,47 @@  single rev
   sending [PATCH] test ...
   sending mail: $TESTTMP/t2/pretendmail.sh -f test foo
 
+Shell characters in addresses
+
+  $ hg email --date '1980-1-1 0:1' -v -t '~foo/bar@example.com' -f 'me*@example.com' -r '10'
+  this patch series consists of 1 patches.
+  
+  warning: invalid patchbomb.intro value "mpmwearaclownnose"
+  (should be one of always, never, auto)
+  -f me*@example.com ~foo/bar@example.com
+  MIME-Version: 1.0
+  Content-Type: text/plain; charset="us-ascii"
+  Content-Transfer-Encoding: 7bit
+  Subject: [PATCH] dd
+  X-Mercurial-Node: 3b6f1ec9dde933a40a115a7990f8b320477231af
+  X-Mercurial-Series-Index: 1
+  X-Mercurial-Series-Total: 1
+  Message-Id: <3b6f1ec9dde933a40a11.315532860@test-hostname>
+  X-Mercurial-Series-Id: <3b6f1ec9dde933a40a11.315532860@test-hostname>
+  User-Agent: Mercurial-patchbomb/5.1.1+333-01ba660965ef+20190930
+  Date: Tue, 01 Jan 1980 00:01:00 +0000
+  From: me*@example.com
+  To: ~foo/bar@example.com
+  
+  # HG changeset patch
+  # User test
+  # Date 5 0
+  #      Thu Jan 01 00:00:05 1970 +0000
+  # Branch test
+  # Node ID 3b6f1ec9dde933a40a115a7990f8b320477231af
+  # Parent  2f9fa9b998c5fe3ac2bd9a2b14bfcbeecbc7c268
+  dd
+  
+  diff -r 2f9fa9b998c5 -r 3b6f1ec9dde9 d
+  --- a/d	Thu Jan 01 00:00:04 1970 +0000
+  +++ b/d	Thu Jan 01 00:00:05 1970 +0000
+  @@ -1,1 +1,2 @@
+   d
+  +d
+  
+  sending [PATCH] dd ...
+  sending mail: $TESTTMP/t2/pretendmail.sh -f 'me*@example.com' '~foo/bar@example.com'
+
 Test pull url header
 =================================