Closed Bug 1408867 Opened 3 years ago Closed 2 years ago

Privacy Issue: preconnect bypasses remote content block

Categories

(Thunderbird :: Security, defect)

defect
Not set
normal

Tracking

(thunderbird_esr52 fixed, thunderbird59 fixed, thunderbird60 fixed)

RESOLVED FIXED
Thunderbird 60.0
Tracking Status
thunderbird_esr52 --- fixed
thunderbird59 --- fixed
thunderbird60 --- fixed

People

(Reporter: jens.a.mueller, Assigned: mkmelin)

References

Details

(Keywords: csectype-disclosure, privacy, sec-moderate, Whiteboard: TB 52.7 ESR)

Attachments

(3 files)

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).
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?
Flags: needinfo?(jens.a.mueller)
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).
Flags: needinfo?(jens.a.mueller)
Depends on: 1412107
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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? :)
Flags: needinfo?(vseerror)
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?
ref: comment 4, comment 5
Flags: needinfo?(vseerror)
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(acelists)
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.
Flags: needinfo?(acelists)
(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.
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Flags: needinfo?(mkmelin+mozilla)
(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
Attachment #8933927 - Flags: review?(acelists)
Run |netstat -pant| after opening this testcase. 
With the patch you don't get a connection to 151.101.36.193.
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.
Attachment #8933927 - Flags: review?(acelists) → review+
But let's make SM aware of the bug so they can make their decision on it.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/0e55b426edf3b577214e746e146df3ead5335f9e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 59.0
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
I guess you need to send a patch so those tests set the preference to the expected value first before re-landing the patch.
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.
Duplicate of this bug: 1415859
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #8944535 - Flags: review?(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
Attachment #8944535 - Flags: review?(mcmanus) → review+
Keywords: checkin-needed
Target Milestone: Thunderbird 59.0 → Thunderbird 60.0
https://hg.mozilla.org/comm-central/rev/b4b02a5f79ff2de6f74382e0e0ef8e4bfa18ac24
Status: REOPENED → RESOLVED
Closed: 3 years ago2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Group: mail-core-security → core-security-release
Comment on attachment 8944535 [details] [diff] [review]
bug1408867_preconn.m-c.patch

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

Test only changes
Attachment #8944535 - Flags: approval-mozilla-esr52?
Attachment #8944535 - Flags: approval-mozilla-beta?
Attachment #8933927 - Flags: approval-comm-esr52?
Attachment #8933927 - Flags: approval-comm-beta?
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.
Flags: needinfo?(aryx.bugmail)
Does this affect Firefox in any way? If not why do the tests exist in m-c?
Flags: needinfo?(mkmelin+mozilla)
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.
Flags: needinfo?(mkmelin+mozilla)
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.
Attachment #8944535 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(aryx.bugmail)
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.
Attachment #8933927 - Flags: approval-comm-beta? → approval-comm-beta+
Comment on attachment 8944535 [details] [diff] [review]
bug1408867_preconn.m-c.patch

This is test-only and doesn't really need approval, but sure :)
Attachment #8944535 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
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.
https://hg.mozilla.org/releases/mozilla-esr52/rev/d1d94dadd94dba03a8ac980e3ba9629d32c3d233 since on esr52 xpcshell does do_register_cleanup rather than registerCleanupFunction.
Thanks, Phil!
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).
Attachment #8944535 - Flags: approval-mozilla-esr52+
Attachment #8944535 - Flags: approval-mozilla-beta+
Attachment #8933927 - Flags: approval-comm-esr52? → approval-comm-esr52+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.