657158 - (CVE-2011-2381) [SECURITY] Request email headers for attachment containing newline are corrupt

Status: VERIFIED FIXED

(Bugzilla :: Email Notifications, defect)

Description

[[e-mail]]

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

[[e-mail]]

Attachment: 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]

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

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

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

Attachment: patch for 4.x - v1

Something like this, maybe? I haven't tested it yet.

Comment 6

[Frédéric Buclin]

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

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

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

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

  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]

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]

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

Moving to the Bugzilla product since this was acknowledged as an upstream bug.

Comment 14

[Dave Miller [:justdave]]

Interesting that it never prompted me to verify the version and component when I moved products...

Comment 15

[Frédéric Buclin]

drug is bad. It's already upstream.

Comment 16

[Frédéric Buclin]

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]

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]

Backports for 3.6 and 3.4 needed.

Comment 19

[Max Kanat-Alexander]

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

reed: have time for the backports?

Comment 21

[Frédéric Buclin]

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

[:glob ✱]

Comment on attachment 544248 [details] [diff] [review]
patch for 3.x, v1

r=glob

Comment 23

[Daniel Veditz [:dveditz]]

Use CVE-2011-2381 for this bug.

Comment 24

[Frédéric Buclin]

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

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]

Security advisory sent, unlocking this bug.