1411716 - (CVE-2017-7846) TBE-01-014: JavaScript Execution via RSS in mailbox:// origin

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, defect)

Description

[Ben Bucksch (:BenB)]

In case a user views RSS feeds as a website, e.g. via “View -> Feed article -> Website” or in the standard format of “View -> Feed article -> default format”, it is possible to execute JavaScript in the parsed RSS feed.

When either one of the aforementioned two settings is in place, Thunderbird will check for the presence of a <link> element in a RSS item. It will then fetch and display the specified origin. The remote web page is from then on allowed to execute JavaScript like any normal web page. This is not a straightforward security issue, as the default Same Origin Policy is applied. In case where no <link> element is present, Thunderbird will parse the description element but will forbid JavaScript execution. A possibility to bypass the restriction was uncovered with relation to specifying a data:text/html URL for a <link> element. Thunderbird parses the data: URL and displays the defined HTML page, which then operates in the mailbox:// origin and is allowed to execute JavaScript:

PoC RSS Feed:
<?xml version="1.0" encoding="UTF-8"?>
<rss version="2.0">
<channel>
<title>Feed1</title>
<link>http://example.com</link>
<item>
<title>Feed1</title>
<link>data:text/html,%3ch1%3easdf%3c/h1%3e%3cscript%3ealert(123)
%3c/script%3e</link>
</item>
</channel>
</rss>

It is recommended to only allow HTTP/HTTPS URLs in <link> tags. This ensures that a feed can exclusively specify real remote web pages, therefore being restricted to its own origin.

Comment 1

[Ben Bucksch (:BenB)]

For the original report as PDF; see bug 1411701.

Comment 2

[alta88]

Attachment: parserLink.patch

Disallow all non http* <link> type urls.

Comment 3

[alta88]

Attachment: parserLink.patch

tweaks.

Comment 4

[Magnus Melin [:mkmelin]]

(In reply to Ben Bucksch (:BenB) from comment #0)
> In case where no <link> element is present, Thunderbird will parse
> the description element but will forbid JavaScript execution. A possibility
> to bypass the restriction was uncovered with relation to specifying a
> data:text/html URL for a <link> element. Thunderbird parses the data: URL
> and displays the defined HTML page, which then operates in the mailbox://
> origin and is allowed to execute JavaScript:

data uris are treated as unique opaque origins and do not inherit their origin, so whatever is happening here, there's some misunderstanding about why it's happening.

Comment 5

[alta88]

Regardless of whether there is a principal/origin problem for a data: url load in this case, I would like the patch to go in.

The parser should not store junk for <link> nor do the feed specs anywhere mention data: as a valid url for <link>, (not even atom's iri spec reference for links), so data: is neither within the law and certainly not within the spirit for what a <link> type tag value should be.

Comment 6

[Magnus Melin [:mkmelin]]

Ah, the data uris shouldn't inherit origin is for v57 and above (bug 1324406). So, this bug is not a problem for trunk, we just need to handle it in 52.

Comment 7

[Magnus Melin [:mkmelin]]

And to be clear, this isn't about the principal but only origin. The problem it can cause is potential information disclosure in other mailbox:// uris if the attacker manages to the exact URLs.

Comment 8

[Magnus Melin [:mkmelin]]

(In reply to alta88 from comment #5)
> The parser should not store junk for <link> nor do the feed specs anywhere
> mention data: as a valid url for <link>, (not even atom's iri spec reference
> for links), so data: is neither within the law and certainly not within the
> spirit for what a <link> type tag value should be.

data uris are perfectly valid iris, so I do think the spec allows them (implicitly).

Comment 9

[Magnus Melin [:mkmelin]]

Attachment: bug1411716_feed_data_disable_js.patch - For demonstration only, not to be landed.

We could also just disable JavaScript for data: links. 
For 57 and above, it's not needed since there the origin is not inherited. Any script executed there can be compared to one executed for a remote origin.

Comment 10

[alta88]

Without getting into the weeds of either the feed specs or spec for the now long obsolete Content-Base header in the coerced mail message from feed entry, "implicitly" is a big stretch. There is no reason to support data: uris in <link> type tags stored as mail headers.

Allowing a data: uri potentially laden with js, and then creating a policy for it at load time, is not a better practice than simply not allowing data: or other non http to be stored in the first place.

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 8922969 [details] [diff] [review]
parserLink.patch

Review of attachment 8922969 [details] [diff] [review]:
-----------------------------------------------------------------

Ok let's go with this one. r=mkmelin

::: mailnews/extensions/newsblog/content/feed-parser.js
@@ +908,5 @@
>  
>      return matchingChildren.length ? matchingChildren : null;
>    },
>  
> +  validLink: function(link)

Please add a short documentation for the method.

@@ +910,5 @@
>    },
>  
> +  validLink: function(link)
> +  {
> +    if (link && link.startsWith("http"))

Simpler and more bullet proof as

if(/https?:/.test(link))

@@ +1012,5 @@
>      options.updates.updateBase = updateBase;
>      aFeed.options = options;
>    },
>  
> +  removeUnprintableASCII: function(s)

Here too, add documentation that you're removing ascii control chars such as \n

Comment 12

[alta88]

Attachment: parserLink.patch

updated for comments.

Comment 13

[Al Billings [:abillings - ex-MoCo]]

Does sec-approval even apply to TB only code? This is a gray area.

Comment 14

[Al Billings [:abillings - ex-MoCo]]

Also, alta88, you need to fill out the sec-approval? template when you set that flag anyway.

Comment 15

[alta88]

Comment on attachment 8926442 [details] [diff] [review]
parserLink.patch

The flag was set because it was requested at first diagnosis.  I don't think this patch rises to sec review level.  It merely sanitizes a string to prevent non http urls.

Sec can decide if it's review worthy and any input is appreciated. Clearing for now.

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 8926442 [details] [diff] [review]
parserLink.patch

No indication on what to land here, so I assume it's this one.

Comment 17

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/98e543540fb6478308915be0849a5217ad2d0db1

Comment 18

[:aceman]

(In reply to Magnus Melin from comment #11)
> @@ +910,5 @@
> >    },
> >  
> > +  validLink: function(link)
> > +  {
> > +    if (link && link.startsWith("http"))
> 
> Simpler and more bullet proof as
> 
> if(/https?:/.test(link))

It seems to me this will match also any "http:" in the middle of the string (e.g. in the Javascript we wanted to disallow).

Should it have been
if (/^https?:\/\//.test(link)) or link.startsWith("http://") || link.startsWith("https://") ?

Comment 19

[alta88]

Attachment: parserLinkRegex.patch

Of course, thanks for checking aceman.

Comment 20

[alta88]

change status for sheriff search..

Comment 21

[Jorg K (CEST = GMT+2)]

Hi the "sheriff developer" here:
Do you really mean: |if (/^https?:\/\//.test(link))|? What is the ? meant to match? Or do you mean |if (/^http?:\/\//.test(link))| where the ? can match an "s"? Looking at the original patch, you mean the latter.

Comment 22

[Magnus Melin [:mkmelin]]

? means optionally having the preceding s, that is matching http and https

Comment 23

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #22)
> ? means optionally having the preceding s, that is matching http and https
Sorry, that just occurred to me, too. Here we go:

https://hg.mozilla.org/comm-central/rev/3c4fb63620220649e337cf826b3f1afc39824075

Comment 24

[Daniel Veditz [:dveditz]]

(In reply to Al Billings [:abillings] from comment #13)
> Does sec-approval even apply to TB only code? This is a gray area.

I'm going to guess not because Tbird isn't supporting multiple branches and don't need to worry as much about 0-daying themselves.

Comment 25

[Jorg K (CEST = GMT+2)]

Why are TB 57 and TB 58 "unaffected" and why did nobody request TB 52 uplift?

Comment 26

[Magnus Melin [:mkmelin]]

Because for 57 and above the origin is not inherited (mozilla-central tightened security).

Comment 27

[Jorg K (CEST = GMT+2)]

Comment on attachment 8927088 [details] [diff] [review]
parserLinkRegex.patch

Sadly these
https://hg.mozilla.org/comm-central/rev/98e543540fb6478308915be0849a5217ad2d0db1
https://hg.mozilla.org/comm-central/rev/3c4fb63620220649e337cf826b3f1afc39824075
don't apply to c-esr52 at all, many merge conflicts.

I could try to hand-craft patches for TB 52, but feeds might just break in the middle if I get something wrong.

Comment 28

[Jorg K (CEST = GMT+2)]

Attachment: 92e89bd8daf85910e04d9c44098ba9279735a8a7.patch

Hand-made comm-esr52 patch merging the two patches.

Comment 29

[Jorg K (CEST = GMT+2)]

Attachment: 98e543540fb6478308915be0849a5217ad2d0db1.patch

Oops, wrong patch attached.

Comment 30

[Jorg K (CEST = GMT+2)]

Attachment: 98e543540fb6478308915be0849a5217ad2d0db1.patch (v2)

Removed some contentBase stuff which slipped in from the TB 58 version.

Comment 31

[alta88]

Not sure what the needinfo action is. Looks like you have in under control.

Comment 32

[Jorg K (CEST = GMT+2)]

You could check the patch while I'm doing bug 1411699 and bug 1418666.

Comment 33

[alta88]

It looks fine, working around the xmlbase patch is annoying.. The patch in the other bug, on top of this one, also is fine.

Comment 34

[Jorg K (CEST = GMT+2)]

TB 52.5.2 ESR:
https://hg.mozilla.org/releases/comm-esr52/rev/01f2c40a0ce93adac921688b151f66a2cead8ca5