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.

As you can see, the [Checked in: See comment 18] has been output on a separate line and things have gone downhill from there.

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.

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.

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.

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

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

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

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

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

There's no good reason to allow newlines into attachment descriptions, we can clean them out on input, that's fine with me.

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.

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

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

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

drug is bad. It's already upstream.

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

Backports for 3.6 and 3.4 needed.

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

reed: have time for the backports?

reed said he was too busy, so here is the backport for 3.4 and 3.6. Tested on both branches, works fine.

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

Use CVE-2011-2381 for this bug.

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.

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.

Security advisory sent, unlocking this bug.