968342 - News Protocol: Remote Content Loading Policy Bypass

Status: RESOLVED FIXED

(MailNews Core :: Networking: NNTP, defect)

Description

[l.pontorieri]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release)
Build ID: 20130911160237

Steps to reproduce:

«By default, Thunderbird blocks remote images and other content in messages from people you don't know. This protects your privacy because spammers can verify your email address by detecting if you viewed a remote image in a message from them.» 

Mozilla Thunderbird should ask for user authorization (or other user interaction) before loading remote content in an email. It has been discovered that Thunderbird automatically loads "news protocol urls" despite there's no user interaction.
An attacker could take advantage of this issue to steal user IP and/or verify if the email is active.

1) Start a program to listen on attacker server on port N (e.g. nc -lv 9999)
2) Write an HTML Email with content 

<iframe src="news://[ServerIp]:[Port]/Foo"></iframe>

3) Send email to Thunderbird user victim
4) When victim opens the email, a TCP request is automatically sent (and a prompt ask for authorization to suscribe to group "Foo")
5) Connection is received by the program at point 1 revealing victim's IP



Actual results:

Thunderbird loads "news:// urls" without user interaction.


Expected results:

Thunderbird should not send TCP requests without user interaction.

Comment 1

[l.pontorieri]

Note - Spammer scenario: 

The only element the attacker have to simultaneously analyze multiple mails is port (e.g. connection received on 8888 -> it's [e-mail] because I sent an email which auto-connects to port 8888 only to him).
Port maximum number is 2^16.
At first glance it seems the attacker is limited to analyze 65535 email accounts per attack.
But a spammer isn't limited in the "amount of forced TCP connections". 
If he forces his victims to do more than one request, he could analyze way more emails simultaneously.

Comment 2

[Mark Banner (:standard8)]

Pretty much identical to bug 968334

Comment 3

[Daniel Veditz [:dveditz]]

Valid bug, but privacy leaks don't meet the severity criteria for the bug bounty. bug 968334 is possibly more interesting on that front

Comment 5

[Mark Banner (:standard8)]

Neil, is this fixed by bug 968334 ?

Comment 6

[[e-mail]]

(In reply to Mark Banner from comment #5)
> Neil, is this fixed by bug 968334 ?

No, it's unrelated.

Comment 8

[Alexander Klink]

I am the reporter of 1212138. Any intent to fix this in the near or medium term? Otherwise I would read the disinterest in my bug as well as apparently this bug (given that it was reported nearly two years ago) as WONTFIX and would proceed to disclose it publically. Thoughts, anyone?

Comment 9

[l.pontorieri]

Hello Alexander! I'm the original reporter, I already have an article ready to be published about this issue. 

Could you contact me via email?

Comment 10

[Philipp Kewisch [:Fallen][📱📆]]

Hello Alexander, L.pontorieri. Do you think you could hold off a little while longer? This bug seems to have fallen under the radar. I'll see if I can find an owner for it quickly.

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

Joshua, any insight into this bug? Do you think you could take a look at it?

Comment 12

[Alexander Klink]

I am fine with holding off if there's work being done/an intenttion to fix it.

Comment 13

[Joshua Cranmer [:jcranmer]]

So probably the easiest way I see to fix this bug is to see if a mail URL is trying to load another mail URL, and if so, enforce that they both point to the same location. This should work for news: since all news:// and nntp:// URLs (including group URLs) get mapped to the mailnews URL interface anyways, and GetMessageHeader should return null in that scenario.

Where this gets really tricky is the weird corner cases that crop up. Dummy message headers could cause this check to fail, and I'm seeing the compose code logic and that is really causing the most--there's a few putative origins to be checking, and I'm not 100% sure what the right way is to go about there. Neil's comments in the older bug also suggest potential issues with RSS and detached attachments.

The basic logic is:
if (source instanceof nsIMsgMessageUrl and target instanceof nsIMsgMessageUrl)
  if source.messageHeader == target.messageHeader: accept
  else: reject
else:
  do normal processing

I'm not quite sure what variable constitutes the source, or where exactly in the content policy lookup this belongs. Neil, do you have more experience with content policy here?

Comment 14

[Philipp Kewisch [:Fallen][📱📆]]

Neil, any update on this? I'd really like to get a plan into place to fix this issue before it is made public.

Comment 15

[Wayne Mery (:wsmwk)]

(In reply to Philipp Kewisch [:Fallen] from comment #14)
> Neil, any update on this? I'd really like to get a plan into place to fix
> this issue before it is made public.

We'll need someone else. perhaps Iann?

Comment 16

[[e-mail]]

Attachment: Possible approach

Sorry for the delay; my mother went into hospital for cancer treatment two days after the request and due to complications she was in for three months :-(

I'm not sure what happens with messages referring to attachments in other messages - it might be better to make the check against the same account rather than the same message. I've written a patch to demonstrate that approach. Obviously I've added the code in what I think is the best place for it ;-)

Comment 17

[Jorg K (CEST = GMT+2)]

Interesting bug, I'll take a look at the existing patch. Since it's access restricted, I don't think "patchlove" will attract anyone here ;-)

Comment 18

[Jorg K (CEST = GMT+2)]

I've played around with the patch, looks OK to me, I didn't find any problem. Try run here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=36882c5432b0d97e150b343e82a9f3e34a1f7ec1

Boris, can I get your opinion this patch. The basic idea is not to load all mailnews protocols (as per the removed code block) but only to allow loading if the pre-path matches. Basically this bug is about not loading, for example
  <iframe src="news://news.mozilla.org:119/mozilla.moss"></iframe>
from a message. So it's basically a poor-man's "same origin" check.

I've added some debug to see what those pre-path are, this is how they look:

IMAP:
=== content imap://[e-mail]:993
=== request imap://[e-mail]:993

NNTP:
=== content news://news.mozilla.org:119
=== request news://news.mozilla.org:119

Mailbox:
=== content mailbox://
=== request mailbox://
full spec for example (as we know from other bugs):
mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/testfolder?number=1

Also, aRequestingLocation is tested to be not null be the time we get to the new code block, so this shouldn't crash.

Another option would be to take the aRequestPrincipal passed to ShouldLoad(), QI the aContentLocation to an nsIURIWithPrincipal (assuming we finally land bug 1347598 where parts will be given a principal) and then check whether the request principal subsumes(?) the content principal, if any. If none, don't load.

What do you think? - a phrase :roc frequently added at the end ;-)

Comment 19

[Jorg K (CEST = GMT+2)]

Since my Mac try never started, I did another one in Linux:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fffac54f9ae3a5e6bb1f07c33484962decafb5a1

And sure enough, there are some test failures:

TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/content-policy/test-compose-mailto.js | test-compose-mailto.js::test_openComposeFromMailToLink
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/content-policy/test-compose-mailto.js | test-compose-mailto.js::test_checkInsertImage
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/content-policy/test-compose-mailto.js | test-compose-mailto.js::test_closeComposeWindowAndTab

But before I look into fixing the test failures, some mostly likely since we no longer blindly accept
-  if (MsgLowerCaseEqualsLiteral(contentScheme, "mailto") ||
mailto, let's hear what Boris thinks. Well, if we went for the alternate approach with the principals, I think I'd have a hard time to make the mailto click work, so perhaps a poor-man's "same origin" check is good enough.

Comment 20

[Jorg K (CEST = GMT+2)]

Attachment: 968342.patch - Neil's approach enhanced to fix test failures.

OK, when still blindly allowing the "mailto" scheme, and also "about" and "addbook", which shouldn't do any harm, the test passes. In fact, the test passes when leaving "mailto", but I see no need to remove the other two as long as the other protocols are removed.

I'd be happy to take the patch in this form. Any objections, Boris? I guess we could still do the principal approach as long as we leave the special cases for "mailto", etc.

Comment 21

[Boris Zbarsky [:bzbarsky]]

I don't have useful thoughts here, sorry.  I'm not up on what the actual security invariants around the various mailnews URLs are, nor do I remember offhand what the IsExposedProtocol() check is doing or why.

If no one actively involved in Thunderbird is willing to step up and sort through this, I can try, but my time is _very_ constrained right now so I can't promise anything about timeframes, for an auditing/research task that is likely to take at least an entire day.

That said, I'm not sure why the comment about all-thunderbird.js is being removed, or why the relevant preferences do not need changing.  The latter could at the very least use some info in the commit message, if not outright comments in all-thunderbird.js.

Comment 22

[Jorg K (CEST = GMT+2)]

Attachment: 968342.patch - Neil's approach enhanced to fix test failures, enhanced comments.

I've taken Boris' comments on board and restored the removed comment with an adapted wording and improved comments throughout.

This solution is what both Joshua (comment #13) and Neil (comment #16) suggested. It fixes the case presented here and doesn't seem to cause any failures.

All it does is instead of blindly accepting loads for news, snews, nntp, imap, pop and mailbox schemes, it tightens what is accepted by comparing content and requesting location. That only tightens security and doesn't create any new holes.

Comment 23

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/c322afd9bf99c3b9c168e1319ccd4b419c209564

Comment 24

[Jorg K (CEST = GMT+2)]

Comment on attachment 8853360 [details] [diff] [review]
968342.patch - Neil's approach enhanced to fix test failures, enhanced comments.

Looks like we should uplift this ;-)

Comment 25

[Jorg K (CEST = GMT+2)]

Kent, please take a look at the changes here:
https://hg.mozilla.org/comm-central/rev/c322afd9bf99c3b9c168e1319ccd4b419c209564#l1.58

The content policy check had some hard-coded protocol names, which obviously didn't include JS-Account. I believe you had trouble at some stage with some embedded images not showing. For composition that got fixed in "our favourite bug" where in compose windows all references to other message parts are now replaced by data: URIs which always get displayed. And we do that for all protocols which are instance of nsIMsgMessageFetchPartService.

Coming back to the bug here, I noticed that in all-thunderbird.js there is a list of exposed protocols and JS-Account isn't included there. Maybe you want to investigate whether that's needed.

Comment 26

[Jorg K (CEST = GMT+2)]

Comment on attachment 8853360 [details] [diff] [review]
968342.patch - Neil's approach enhanced to fix test failures, enhanced comments.

Philipp pointed out the I changed the original patch from Neil to fix test failures.

He had this hunk:

-  if (MsgLowerCaseEqualsLiteral(contentScheme, "mailto") ||
-      MsgLowerCaseEqualsLiteral(contentScheme, "news") ||
-      MsgLowerCaseEqualsLiteral(contentScheme, "snews") ||
-      MsgLowerCaseEqualsLiteral(contentScheme, "nntp") ||
-      MsgLowerCaseEqualsLiteral(contentScheme, "imap") ||
-      MsgLowerCaseEqualsLiteral(contentScheme, "addbook") ||
-      MsgLowerCaseEqualsLiteral(contentScheme, "pop") ||
-      MsgLowerCaseEqualsLiteral(contentScheme, "mailbox") ||
-      MsgLowerCaseEqualsLiteral(contentScheme, "about"))
-    return true;

but in fact we can't remove "mailto". I also left "addbook" and "about" in place, since the cause no harm.

So the hunk was changed to:

   if (MsgLowerCaseEqualsLiteral(contentScheme, "mailto") ||
-      MsgLowerCaseEqualsLiteral(contentScheme, "news") ||
-      MsgLowerCaseEqualsLiteral(contentScheme, "snews") ||
-      MsgLowerCaseEqualsLiteral(contentScheme, "nntp") ||
-      MsgLowerCaseEqualsLiteral(contentScheme, "imap") ||
       MsgLowerCaseEqualsLiteral(contentScheme, "addbook") ||
-      MsgLowerCaseEqualsLiteral(contentScheme, "pop") ||
-      MsgLowerCaseEqualsLiteral(contentScheme, "mailbox") ||
       MsgLowerCaseEqualsLiteral(contentScheme, "about"))
     return true;

Kent is already NI'ed here, so he can look it over.

As noted earlier, due to bug 1351966 we currently don't see any embedded images, so not a good time to test this.

Comment 27

[Jorg K (CEST = GMT+2)]

Aurora (TB 54):
https://hg.mozilla.org/releases/comm-aurora/rev/d37f0a1d5fbb6fb4d71f886fd45489f6a361342c

Uplifted to Aurora to give this some exposure:
- Embedded (cid:) images currently don't work on trunk, bug 1351966.
- There are no Daily Windows builds, bug 1352456.
It has been policy to land on Aurora if Trunk is broken.

Comment 28

[Jorg K (CEST = GMT+2)]

Beta (TB 53):
https://hg.mozilla.org/releases/comm-beta/rev/15b8cc91c73ae49c7ccb2c2bdc705e9931f7c44e

Comment 29

[Kent James (:rkent)]

(In reply to Jorg K (GMT+2) from comment #25)
> Kent, please take a look at the changes here:
> https://hg.mozilla.org/comm-central/rev/
> c322afd9bf99c3b9c168e1319ccd4b419c209564#l1.58
> 
> The content policy check had some hard-coded protocol names, which obviously
> didn't include JS-Account. I believe you had trouble at some stage with some
> embedded images not showing. For composition that got fixed in "our
> favourite bug" where in compose windows all references to other message
> parts are now replaced by data: URIs which always get displayed. And we do
> that for all protocols which are instance of nsIMsgMessageFetchPartService.
> 
> Coming back to the bug here, I noticed that in all-thunderbird.js there is a
> list of exposed protocols and JS-Account isn't included there. Maybe you
> want to investigate whether that's needed.

JS-Account is not a protocol, but a method to allow new protocols to be added to mailnews. I use it for the "exquilla" protocol for example. ExQuilla has this:

pref("network.protocol-handler.expose.exquilla", true);

In bug 764987 the issue of exposed protocols in addons was added, but exquilla did a work around earlier so we never took advantage of that.

So I don't see an issue here around JsAccount specifically.

Comment 30

[Kent James (:rkent)]

Comment on attachment 8853360 [details] [diff] [review]
968342.patch - Neil's approach enhanced to fix test failures, enhanced comments.

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

LGTM

Comment 31

[Jorg K (CEST = GMT+2)]

Thanks, this landed on a few branches already, so I'll uplift to TB 52 ESR without a bad conscience ;-)

Comment 32

[Jorg K (CEST = GMT+2)]

TB 52 ESR:
https://hg.mozilla.org/releases/comm-esr52/rev/cd16709e68e83891e85419e05ce9713c4294db2d