Last Comment Bug 386860 - [SECURITY] Insufficient escaping of From address when using Sendmail
: [SECURITY] Insufficient escaping of From address when using Sendmail
Status: RESOLVED FIXED
[ready for 3.x]
:
Product: Bugzilla
Classification: Server Software
Component: Bugzilla-General (show other bugs)
: 2.23.4
: All All
: P1 major (vote)
: Bugzilla 3.0
Assigned To: Max Kanat-Alexander
: default-qa
Mentors:
Depends on:
Blocks: 389843
  Show dependency treegraph
 
Reported: 2007-07-04 09:07 PDT by Loïc Minier
Modified: 2007-08-23 14:03 PDT (History)
2 users (show)
LpSolit: approval+
mkanat: blocking3.1.1+
LpSolit: approval3.0+
mkanat: blocking3.0.1+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (1.01 KB, patch)
2007-07-04 09:26 PDT, Max Kanat-Alexander
no flags Details | Diff | Splinter Review
v2 (1.01 KB, patch)
2007-07-04 10:06 PDT, Max Kanat-Alexander
no flags Details | Diff | Splinter Review
Actual v2 (1.12 KB, patch)
2007-07-04 10:08 PDT, Max Kanat-Alexander
LpSolit: review+
Details | Diff | Splinter Review

Description Loïc Minier 2007-07-04 09:07:58 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.4) Gecko/20070508 (Debian-1.8.1.4-3) Galeon/2.0.2 (Debian package 2.0.2-4)
Build Identifier: 3.0

email_reply.pl performs insufficient escaping of arguments passed to sh -c sendmail.  This might be remotely exploitable; setting security flag.

Long story: After some hours battling, I could get email_reply.pm to run, but when passed valid mail data, it would fail with:
sh: -c: line 0: syntax error near unexpected token `newline'
sh: -c: line 0: `/usr/lib/sendmail -t -oi -i -f<submit@bugzilla.example.com>'

The mail data looks something like:
From lool@dooz.org Wed Jul  4 17:19:58 2007
Date: Wed, 4 Jul 2007 17:19:58 +0200
From: =?iso-8859-1?Q?Lo=EFc?= Minier <lool@dooz.org>
To: submit@bugzilla.example.com
Subject: Essai
Message-ID: <20070704151958.GD9800@bee.dooz.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

Essai

-- 
Loïc Minier

I worked around this problem by adding single quotes to:
push(@args, "-f'$from'") if $from;
in Bugzilla/Mailer.pm, around line 80.

This is with version 2.181 of Email::Send as found in Debian stable (etch) in the libemail-send-perl package.

Note: email_reply.pl still fails for me afterwards with:
sendmail: fatal: No recipient addresses found in message header
There was an error sending mail from '<submit@bugzilla.bpl-group.org>'
    to 'Loïc Minier <lool@dooz.org>':error when closing pipe to /usr/lib/sendmail:


Reproducible: Always
Comment 1 Max Kanat-Alexander 2007-07-04 09:09:58 PDT
Do you mean Email::Reply? Or email_in.pl?

Email::Reply isn't maintained by us, it's a CPAN module.

email_in.pl is the name of the Bugzilla script.
Comment 2 Loïc Minier 2007-07-04 09:11:57 PDT
Also happens with Email::Send 2.185.
Comment 3 Loïc Minier 2007-07-04 09:13:09 PDT
(In reply to comment #1)
> Do you mean Email::Reply? Or email_in.pl?
> 
> Email::Reply isn't maintained by us, it's a CPAN module.
> 
> email_in.pl is the name of the Bugzilla script.

I meant email_in.pl, sorry for the typo; the actual bug seems to be in Bugzilla/Mailer.pm though.
Comment 4 Max Kanat-Alexander 2007-07-04 09:20:02 PDT
Normally this isn't as much of a problem, because the only source of the mailfrom parameter is Bugzilla itself. However, in a shared hosting environment that would still be a problem.

In email_in.pl, though, Email::Reply uses the "To" address as the "From" address for replies. So that's user-submitted data, and could be a problem.

The solution here is to use Email::Address to parse the $from when setting -f for sendmail, and just include the email address.

We probably should also do some escaping of common shell characters, but that's a bit trickier, since I don't know of any function or module that will do that for us.
Comment 5 Max Kanat-Alexander 2007-07-04 09:26:02 PDT
Created attachment 270917 [details] [diff] [review]
v1

This should do it.
Comment 6 Loïc Minier 2007-07-04 09:29:52 PDT
Ack on the analysis; I wasn't versed enough to use one the dozen of Email:: and
Mail:: functions to properly extract the email address, but I think email
address can be quite complex if you honor all RFC 822/2822 trickiness, so
without solid shell escaping, it is very risky.

As an alternate resolution, I suggest using the configured mailfrom instead of
reading the To:.


Should I file a separate issue for the remaining problem I get afterwards?  It
seems due to a malformed email message text, presumably CRLF/LF mismatch in
line-endings since I see:
From: <submit@bugzilla.example.com>
^MTo: =?UTF-8?Q?Lo=C3=AFc=20Minier=20?=<lool@dooz.org>
^MSubject: Re: Essai
^MIn-Reply-To: <20070704151958.GD9800@bee.dooz.org>
...
in mailer.testfile (note the first line without \r); this doesn't look like a
security issue though.
Comment 7 Loïc Minier 2007-07-04 09:31:33 PDT
(In reply to comment #5)
> Created an attachment (id=270917) [details]
> v1
> 
> This should do it.

Fixes the first bug for me; thanks.
Comment 8 Max Kanat-Alexander 2007-07-04 09:37:19 PDT
(In reply to comment #6)
> As an alternate resolution, I suggest using the configured mailfrom instead of
> reading the To:.

  I want to preserve the "To" that people wrote, in the actual email, but use just the email address on the command line.

> Should I file a separate issue for the remaining problem I get afterwards?
> [snip]

  Those look like a possible problem in Email::Reply. Perhaps contact the author of that module.
Comment 9 Frédéric Buclin 2007-07-04 09:46:59 PDT
Comment on attachment 270917 [details] [diff] [review]
v1

>+            my ($email_obj) = Email::Address->parse($from);
>+            my $from_email = $email_obj->address;

If I pass an invalid string, $email_obj is undefined and you cannot call address(). You should probably write: |$email_obj || return| or something like that.
Comment 10 Max Kanat-Alexander 2007-07-04 10:03:48 PDT
Okay, further testing shows that this is not exploitable on 2.22 or 2.20, because we use Mail::Send there and it uses the multiple-argument form of exec().

It *is* exploitable on 3.0, since 2.23.4. (Actually since bug 353711.)

We may want to mention this to rjbs (the maintainer of Email::Send) to put in the POD for Email::Send::Sendmail.
Comment 11 Max Kanat-Alexander 2007-07-04 10:06:14 PDT
Created attachment 270922 [details] [diff] [review]
v2

Okay, good point.
Comment 12 Max Kanat-Alexander 2007-07-04 10:08:04 PDT
Created attachment 270923 [details] [diff] [review]
Actual v2

Oops, attached the same patch twice, there! :-)
Comment 13 Frédéric Buclin 2007-07-04 10:09:43 PDT
Comment on attachment 270923 [details] [diff] [review]
Actual v2

Looks good. I assume you tested it. r=LpSolit
Comment 14 Loïc Minier 2007-08-16 02:55:25 PDT
It's been 4 months with a remote command execution hole; do you plan to roll a tarball?
Comment 15 Max Kanat-Alexander 2007-08-16 10:12:44 PDT
(In reply to comment #14)
> It's been 4 months with a remote command execution hole; do you plan to roll a
> tarball?

  Hi Loic. Yes, we do. You can track the process of Bugzilla releases on the bugs blocking this one:

  https://bugzilla.mozilla.org/show_bug.cgi?id=bz-release
Comment 16 Frédéric Buclin 2007-08-23 08:47:18 PDT
tip:

Checking in Bugzilla/Mailer.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Mailer.pm,v  <--  Mailer.pm
new revision: 1.12; previous revision: 1.11
done

3.0:

Checking in Bugzilla/Mailer.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Mailer.pm,v  <--  Mailer.pm
new revision: 1.7.2.5; previous revision: 1.7.2.4
done
Comment 17 Max Kanat-Alexander 2007-08-23 14:03:08 PDT
Security advisory from Bug 389843 sent, unlocking bug.

Note You need to log in before you can comment on or make changes to this bug.