Closed Bug 657158 (CVE-2011-2381) Opened 9 years ago Closed 8 years ago

[SECURITY] Request email headers for attachment containing newline are corrupt

Categories

(Bugzilla :: Email Notifications, defect)

2.17.1
defect
Not set

Tracking

()

VERIFIED FIXED
Bugzilla 3.4

People

(Reporter: neil, Assigned: reed)

References

Details

(Whiteboard: [infrasec:input][ws:low])

Attachments

(3 files)

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.
Assignee: email-notifications → nobody
Component: Email Notifications → General
Product: Bugzilla → bugzilla.mozilla.org
QA Contact: default-qa → general
Version: 4.0 → Current
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.
Assignee: nobody → email-notifications
No longer blocks: bmo-regressions
Group: bugzilla-security
Component: General → Email Notifications
Product: bugzilla.mozilla.org → Bugzilla
QA Contact: general → default-qa
Target Milestone: --- → Bugzilla 4.0
Version: Current → 4.0
Flags: blocking4.2?
Flags: blocking4.0.2?
Summary: Request email headers for attachment containing newline are corrupt → [SECURITY] Request email headers for attachment containing newline are corrupt
Version: 4.0 → 4.0.1
Keywords: regression
Whiteboard: [infrasec:input][ws:low]
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.
Assignee: email-notifications → reed
Status: NEW → ASSIGNED
Attachment #532492 - Flags: review?(mkanat)
(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.
Group: bugzilla-security
Flags: blocking4.2?
Flags: blocking4.2+
Flags: blocking4.0.2?
Flags: blocking4.0.2+
Version: 4.0.1 → 3.2.10
Summary: [SECURITY] Request email headers for attachment containing newline are corrupt → Request email headers for attachment containing newline are corrupt
(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.
Group: bugzilla-security
Summary: Request email headers for attachment containing newline are corrupt → [SECURITY] Request email headers for attachment containing newline are corrupt
  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.
Assignee: reed → administration
Component: Email Notifications → Administration
Version: 3.2.10 → 2.10
Interesting that it never prompted me to verify the version and component when I moved products...
Version: 2.10 → 3.2
drug is bad. It's already upstream.
Assignee: administration → reed
Component: Administration → Email Notifications
Version: 3.2 → 3.2.10
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. :)
Flags: blocking3.6.6+
Flags: blocking3.4.12+
Target Milestone: Bugzilla 4.0 → Bugzilla 3.4
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.
Attachment #532492 - Attachment description: patch - v1 (untested) → patch for 4.x - v1
Attachment #532492 - Attachment filename: 657158 → bug657158.diff
Attachment #532492 - Flags: review?(mkanat) → review+
Backports for 3.6 and 3.4 needed.
Flags: approval?
Flags: approval4.0?
(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?
Blocks: 660528
reed said he was too busy, so here is the backport for 3.4 and 3.6. Tested on both branches, works fine.
Attachment #544248 - Flags: review?(glob)
Comment on attachment 544248 [details] [diff] [review]
patch for 3.x, v1

r=glob
Attachment #544248 - Flags: review?(glob) → review+
Flags: approval3.6?
Flags: approval3.4?
Use CVE-2011-2381 for this bug.
Alias: CVE-2011-2381
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.
Version: 3.2.10 → 2.17.1
Flags: approval?
Flags: approval4.0?
Flags: approval4.0+
Flags: approval3.6?
Flags: approval3.6+
Flags: approval3.4?
Flags: approval3.4+
Flags: approval+
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.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Security advisory sent, unlocking this bug.
Group: bugzilla-security
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.