1408867 - Privacy Issue: preconnect bypasses remote content block

Status: RESOLVED FIXED

(Thunderbird :: Security, defect)

Description

[Jens Müller]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20170419042421

Steps to reproduce:

In the scope of academic research in cooperation with Ruhr-University Bochum and FH Münster, Germany we discovered a bypass for remote content blocking.

`<link rel="preconnect" href="http://track-id-12345.spammer.com">`


Actual results:

This triggers a TCP handshake in HTML emails in the default settings which can be abused by spammers to track if an email has actually been read by users.


Expected results:

Instead the user should be asked if remote content should be shown (as it is done for external images, etc).

Comment 1

[Daniel Veditz [:dveditz]]

Firefox 45 is super old and hasn't been supported (by us, at least) for a long time. This bug rings bells -- can you still reproduce on a current version?

Comment 2

[Jens Müller]

I reported it from an old Firefox version :D

But the issue has been tested in Thunderbird 52.4.0 (it's a Thunderbird privacy issue, should not be a problem for Firefox).

Comment 3

[Daniel Veditz [:dveditz]]

I believe the underlying issue is that rel=preconnect aren't sent through content policies, which would also affect Firefox's use of content policies (though as you say it may not be as pressing a concern). I filed bug 1412107 on that.

That might be hard to fix, or performance sensitive to the point that preconnecting would be useless in the browser if every content policy got to vote on the connection (particularly add-ons). Another approach Thunderbird could take would be to disable speculative connections entirely.

I believe setting the pref 'network.http.speculative-parallel-limit' to 0 will do this.

Comment 4

[Frederik Braun [:freddy]]

I like the idea of patching Thunderbird by setting 'network.http.speculative-parallel-limit' to 0.
Wayne, do you think we could find someone to do that? :)

Comment 5

[Frederik Braun [:freddy]]

Though, what I don't understand:
<link rel> seems to be disallowed with our sanitizer, looking at <https://searchfox.org/mozilla-central/rev/9f3bd430c2b132c86c46126a0548661de876799a/dom/base/nsTreeSanitizer.cpp#1234-1245>

Is that not true?

Comment 6

[Wayne Mery (:wsmwk)]

ref: comment 4, comment 5

Comment 7

[:aceman]

Disabling the pref could be useful, I just don't remember right now how we can override the value of a core pref. In out pref files we only list our mail specific prefs.

Comment 8

[Magnus Melin [:mkmelin]]

(In reply to Frederik Braun [:freddyb] from comment #5)
> Though, what I don't understand:
> <link rel> seems to be disallowed with our sanitizer, looking at
> <https://searchfox.org/mozilla-central/rev/
> 9f3bd430c2b132c86c46126a0548661de876799a/dom/base/nsTreeSanitizer.cpp#1234-
> 1245>
> 
> Is that not true?

The sanitizer is only used for composition, and if you opt to see the message as Simple HTML.

Comment 9

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #7)
> Disabling the pref could be useful, I just don't remember right now how we
> can override the value of a core pref. In out pref files we only list our
> mail specific prefs.

Doesn't matter from where the pref is. We apply all.js mailnews.js all-thunderbird.js - last applied wins. So put it in all-thunderbird.js

Comment 10

[Magnus Melin [:mkmelin]]

Attachment: bug1408867_preconnect.patch

Comment 11

[Magnus Melin [:mkmelin]]

Attachment: preconnect-http.eml (test case)

Run |netstat -pant| after opening this testcase. 
With the patch you don't get a connection to 151.101.36.193.

Comment 12

[:aceman]

Comment on attachment 8933927 [details] [diff] [review]
bug1408867_preconnect.patch

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

I assume if we put this into mailnews.js where Seamonkey would pick it up, it would affect their browser component too, which may be unwanted.

Comment 13

[:aceman]

But let's make SM aware of the bug so they can make their decision on it.

Comment 14

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/0e55b426edf3b577214e746e146df3ead5335f9e

Comment 15

[Jorg K (CEST = GMT+2)]

Looks like this caused a few test failures, so I'm afraid I have to back this out:

TEST-UNEXPECTED-TIMEOUT | netwerk/test/unit/test_speculative_connect.js | Test timed out
TEST-UNEXPECTED-FAIL | toolkit/mozapps/update/tests/unit_aus_update/downloadInterruptedRecoveryChannel.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | toolkit/mozapps/update/tests/unit_aus_update/downloadInterruptedRecoveryChannel.js | onStatus - [onStatus : 43] the download status should equal the expected value - 2152398851 == 2152398858
TEST-UNEXPECTED-FAIL | toolkit/mozapps/update/tests/unit_aus_update/downloadInterruptedRecoveryChannel.js | onStatus - [onStatus : 43] the download status should equal the expected value - 2152398859 == 2152398858
TEST-UNEXPECTED-FAIL | toolkit/mozapps/update/tests/unit_aus_update/downloadInterruptedRecoveryChannel.js | onStatus - [onStatus : 43] the download status should equal the expected value - 2152398855 == 2152398858
TEST-UNEXPECTED-FAIL | toolkit/mozapps/update/tests/unit_aus_update/downloadInterruptedRecoveryChannel.js | onStatus - [onStatus : 43] the download status should equal the expected value - 2152398852 == 2152398858

Comment 16

[Jorg K (CEST = GMT+2)]

I guess you need to send a patch so those tests set the preference to the expected value first before re-landing the patch.

Comment 17

[Frank-Rainer Grahl (:frg)]

Fpor SM I see no way to easily distinguish between browser and mail so as like the image authentication prompting we will probbaly only document it and let the user decide if he/she wants to switch it on.

Comment 18

[Jorg K (CEST = GMT+2)]

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

Comment 20

[Magnus Melin [:mkmelin]]

Attachment: bug1408867_preconn.m-c.patch

These two tests need speculative connections to make sense.

That the status is not as expected is ok. Without speculative connections, it's NS_NET_STATUS_WAITING_FOR instead of what the tests expect.

Comment 21

[Patrick McManus [:mcmanus]]

Comment on attachment 8944535 [details] [diff] [review]
bug1408867_preconn.m-c.patch

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

if they're going to change the default, they need to reset the pref value on exiting too. thanks

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/073f1a1a1b41e44ba981c5b232a0a463b31daaa0
https://hg.mozilla.org/mozilla-central/rev/073f1a1a1b41

Comment 23

[Jorg K (CEST = GMT+2)]

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

Comment 24

[Magnus Melin [:mkmelin]]

Comment on attachment 8944535 [details] [diff] [review]
bug1408867_preconn.m-c.patch

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

Test only changes

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

I think we normally wait to land tests until after we ship a fix for security related issues.
Since we already landed the tests here... is there any particular way this might reveal the issue? 
It can be more noticeable when we land sec issues on beta, vs. m-c.

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Does this affect Firefox in any way? If not why do the tests exist in m-c?

Comment 27

[Magnus Melin [:mkmelin]]

It depends on how you look at it. The comm-central fix changes a pref for thunderbird. But since the mozilla-central tests (which tb also run) assume the default value they would begin to fail once the Thunderbird part lands. So from Firefox's point of view, nothing really changed. For Thunderbird it makes it possible to uplift the comm-central part of the fix.

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, I think I see the reasoning, my worry was just that we might change the test and then fail to catch a problem with Firefox.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2e091a38d41c

Comment 30

[Jorg K (CEST = GMT+2)]

Comment on attachment 8933927 [details] [diff] [review]
bug1408867_preconnect.patch

This won't go into TB 59 beta 1 since the Mozilla core part landed on M-B after the revision I'm going to use.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8944535 [details] [diff] [review]
bug1408867_preconn.m-c.patch

This is test-only and doesn't really need approval, but sure :)

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/17511c52fd92bf7155b9c63710c248aa50157939

This revision should be fine to merge to your VERBRANCH too if needed. Nothing important has landed for the 52.7 release yet.

Comment 33

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/releases/mozilla-esr52/rev/d1d94dadd94dba03a8ac980e3ba9629d32c3d233 since on esr52 xpcshell does do_register_cleanup rather than registerCleanupFunction.

Comment 34

[Jorg K (CEST = GMT+2)]

Thanks, Phil!

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8944535 [details] [diff] [review]
bug1408867_preconn.m-c.patch

Clearing the Gecko approval flags to get this off the needs-uplift radar now that the Beta/ESR52 uplifts are done (and we can't set the status flags properly for them).

Comment 36

[Jorg K (CEST = GMT+2)]

TB 59 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/6e229daf13cc2713c2e8af857bda63c430164558

Comment 37

[Jorg K (CEST = GMT+2)]

TB 52.7 ESR:
https://hg.mozilla.org/releases/comm-esr52/rev/d8617031b264db93dcc4ad65a73fdc442eb34676