Last Comment Bug 657158 - (CVE-2011-2381) [SECURITY] Request email headers for attachment containing newline are corrupt
(CVE-2011-2381)
: [SECURITY] Request email headers for attachment containing newline are corrupt
Status: VERIFIED FIXED
[infrasec:input][ws:low]
:
Product: Bugzilla
Classification: Server Software
Component: Email Notifications (show other bugs)
: 2.17.1
: All All
: -- normal (vote)
: Bugzilla 3.4
Assigned To: Reed Loden [:reed] (use needinfo?)
: default-qa
:
Mentors:
Depends on:
Blocks: 660528
  Show dependency treegraph
 
Reported: 2011-05-14 12:21 PDT by neil@parkwaycc.co.uk
Modified: 2011-08-24 13:48 PDT (History)
7 users (show)
LpSolit: approval+
LpSolit: blocking4.2+
LpSolit: approval4.0+
LpSolit: blocking4.0.2+
LpSolit: approval3.6+
LpSolit: blocking3.6.6+
LpSolit: approval3.4+
LpSolit: blocking3.4.12+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Example of a corrupt message (4.71 KB, message/rfc822)
2011-05-14 12:23 PDT, neil@parkwaycc.co.uk
no flags Details
patch for 4.x - v1 (1.16 KB, patch)
2011-05-14 22:03 PDT, Reed Loden [:reed] (use needinfo?)
LpSolit: review+
Details | Diff | Splinter Review
patch for 3.x, v1 (1.14 KB, patch)
2011-07-06 08:02 PDT, Frédéric Buclin
glob: review+
Details | Diff | Splinter Review

Description neil@parkwaycc.co.uk 2011-05-14 12:21:09 PDT
I received a request for an attachment with the following description:
>Read the ignored data
>[Checked in: See comment 18]
This caused the notification email to be corrupted.
I don't remember seeing this corruption before so I think it's a regression.
Comment 1 neil@parkwaycc.co.uk 2011-05-14 12:23:27 PDT
Created attachment 532459 [details]
Example of a corrupt message

As you can see, the [Checked in: See comment 18] has been output on a separate line and things have gone downhill from there.
Comment 2 Frédéric Buclin 2011-05-14 12:40:52 PDT
Looks specific to bmo. I cannot reproduce upstream. In both 4.0 and 4.1 is the newline removed, and the email is correctly formatted.
Comment 3 Reed Loden [:reed] (use needinfo?) 2011-05-14 21:01:38 PDT
Nope, I can confirm this on landfill. Definitely an upstream issue. Also marking this security-sensitive, as you could modify contents of e-mail headers this way.

I don't think it's a regression, but I haven't tested enough to confirm.
Comment 4 Reed Loden [:reed] (use needinfo?) 2011-05-14 21:28:26 PDT
This doesn't affect things like the short_desc, as clean_text() is run as part of the validator (disallowing any control characters, including newlines). We don't do the same for attachment descriptions, however.
Comment 5 Reed Loden [:reed] (use needinfo?) 2011-05-14 22:03:10 PDT
Created attachment 532492 [details] [diff] [review]
patch for 4.x - v1

Something like this, maybe? I haven't tested it yet.
Comment 6 Frédéric Buclin 2011-05-15 01:42:26 PDT
(In reply to comment #5)
> Something like this, maybe? I haven't tested it yet.

I would like that you test your patches first. I saw too many regressions recently because such patches haven't been tested.

Also, I need exact STR as I couldn't reproduce it. This will help to know which branches are affected.
Comment 7 Reed Loden [:reed] (use needinfo?) 2011-05-15 01:53:51 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > Something like this, maybe? I haven't tested it yet.
> 
> I would like that you test your patches first. I saw too many regressions
> recently because such patches haven't been tested.

My time is extremely limited. I specifically called out the fact that my patch is untested. Considering this is a *security* bug, I felt that putting a potential patch up for review first is a higher priority than waiting to test it. You're welcome to test it yourself if you have more time than I do or have concerns about my work.

(In reply to comment #6)
> Also, I need exact STR as I couldn't reproduce it. This will help to know
> which branches are affected.

https://landfill.bugzilla.org/bugzilla-4.0-branch/show_bug.cgi?id=333 has my testing. I created a new attachment, went to the attachment details, modified the description to include a new line, and requested review from myself. The subsequent review mail I received included the same type of problems that attachment 532459 [details] shows.
Comment 8 Frédéric Buclin 2011-05-15 02:19:24 PDT
(In reply to comment #7)
> My time is extremely limited.

We all have limited time.


> Considering this is a *security* bug,

It's not a security bug. You cannot do any harm besides moving some headers into the body of the email. And the bug exists at least since Bugzilla 3.2, where I can reproduce the issue, and probably existed since 2.18, when flags have been introduced.


> a potential patch up for review first is a higher priority than waiting to
> test it.

I disagree. Asking someone for review when the patch may not (fully) work is more time consuming as reviewers have even less time for reviews.


> or have concerns about my work.

I have concerns about the work of everbody who attaches untested patches. It's not just about you. It's not my job to catch regressions or to realize that a problem is not fully fixed. This makes us waste more time later.


> I created a new attachment, went to the attachment details,
> modified the description to include a new line, and requested review from
> myself. The subsequent review mail I received included the same type of
> problems that attachment 532459 [details] shows.

That's interesting, the newline doesn't appear in all cases. If I use the exact same description as you, I can reproduce. If I use "patch, v1\n[foo bar]", I cannot. No idea why. Maybe it depends on which position the newline is in the string.

As there is no reason to forbird newlines in attachment descriptions for now, filtering them in the email template is the right thing to do.
Comment 9 Reed Loden [:reed] (use needinfo?) 2011-05-15 02:24:29 PDT
(In reply to comment #8)
> > Considering this is a *security* bug,
> 
> It's not a security bug. You cannot do any harm besides moving some headers
> into the body of the email. And the bug exists at least since Bugzilla 3.2,
> where I can reproduce the issue, and probably existed since 2.18, when flags
> have been introduced.

Wrong, wrong, 100% wrong. This is most definitely a security bug. Injection of headers into e-mail messages is a very bad thing. Spamming is the first thing that comes to mind, but I can also think of other nefarious things.
Comment 10 Max Kanat-Alexander 2011-05-15 23:11:12 PDT
  There's no good reason to allow newlines into attachment descriptions, we can clean them out on input, that's fine with me.
Comment 11 Max Kanat-Alexander 2011-05-15 23:12:11 PDT
Comment on attachment 532492 [details] [diff] [review]
patch for 4.x - v1

  For trunk, I'd actually rather do it in the validator in Bugzilla::Attachment, although what you're doing here is probably good for the branches.
Comment 12 Frédéric Buclin 2011-05-15 23:49:04 PDT
(In reply to comment #10)
>   There's no good reason to allow newlines into attachment descriptions

In that case, all existing attachment descriptions should be fixed, and the textarea should be replaced by a text field.
Comment 13 Dave Miller [:justdave] (justdave@bugzilla.org) 2011-05-16 09:22:12 PDT
Moving to the Bugzilla product since this was acknowledged as an upstream bug.
Comment 14 Dave Miller [:justdave] (justdave@bugzilla.org) 2011-05-16 09:23:18 PDT
Interesting that it never prompted me to verify the version and component when I moved products...
Comment 15 Frédéric Buclin 2011-05-16 09:24:38 PDT
drug is bad. It's already upstream.
Comment 16 Frédéric Buclin 2011-05-16 09:54:53 PDT
glob and I managed to inject headers in emails, which is worse than having some headers in the body of the email. So now I agree to call it a security bug. :)
Comment 17 Frédéric Buclin 2011-05-16 10:10:36 PDT
Comment on attachment 532492 [details] [diff] [review]
patch for 4.x - v1

Works fine on trunk. Also applies cleanly to 4.0. But the patch doesn't apply cleanly to 3.x. r=LpSolit for 4.x.

I think that removing newlines from attachment descriptions should be done as a separate bug.
Comment 18 Frédéric Buclin 2011-05-16 10:11:21 PDT
Backports for 3.6 and 3.4 needed.
Comment 19 Max Kanat-Alexander 2011-05-16 13:00:05 PDT
(In reply to comment #17)
> I think that removing newlines from attachment descriptions should be done
> as a separate bug.

  That sounds great to me; I agree.
Comment 20 Frédéric Buclin 2011-05-29 05:46:51 PDT
reed: have time for the backports?
Comment 21 Frédéric Buclin 2011-07-06 08:02:42 PDT
Created attachment 544248 [details] [diff] [review]
patch for 3.x, v1

reed said he was too busy, so here is the backport for 3.4 and 3.6. Tested on both branches, works fine.
Comment 22 Byron Jones ‹:glob› [PTO until 2017-01-09] 2011-07-06 21:55:57 PDT
Comment on attachment 544248 [details] [diff] [review]
patch for 3.x, v1

r=glob
Comment 23 Daniel Veditz [:dveditz] 2011-08-01 16:30:20 PDT
Use CVE-2011-2381 for this bug.
Comment 24 Frédéric Buclin 2011-08-02 03:53:09 PDT
2.17.1 was the first release to have this problem, see bug 98801. At that time, this template was split into two pieces: request/created-email.txt.tmpl and request/fulfilled-email.txt.tmpl. Both templates had this problem already.
Comment 25 Reed Loden [:reed] (use needinfo?) 2011-08-04 12:25:47 PDT
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Template.pm
modified template/en/default/request/email.txt.tmpl
Committed revision 7887.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.0/
modified Bugzilla/Template.pm
modified template/en/default/request/email.txt.tmpl
Committed revision 7633.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/3.6/
modified Bugzilla/Template.pm
modified template/en/default/request/email.txt.tmpl
Committed revision 7250.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/3.4/
modified Bugzilla/Template.pm
modified template/en/default/request/email.txt.tmpl
Committed revision 6804.
Comment 26 Max Kanat-Alexander 2011-08-05 17:33:27 PDT
Security advisory sent, unlocking this bug.

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