1589407 - document.referrer leaks the full referrer URL even when the referrer URL has been trimmed due to ETP restrictions

Status: VERIFIED FIXED

(Core :: Privacy: Anti-Tracking, defect, P1)

Description

[(no longer active)]

It looks like mPrefHosts is now a write-only variable which isn't used anywhere.

STR:

1. Set the urlclassifier.trackingAnnotationTable.testEntries pref to trackertest.org.
2. Go to https://senglehardt.com/test/trackingprotection/test_pages/referrer_policy.html.

Expected: The second iframe must say: The referrer accessible via document.referrer is:
https://senglehardt.com/

Actual: It says: The referrer accessible via document.referrer is:
https://senglehardt.com/test/trackingprotection/test_pages/referrer_policy.html

Dimi, could you have a look please? Thanks!

Comment 1

[Thomas Nguyen (:tnguyen)]

I don't think it is a regression, ReevaluateReferrerAfterTrackingStatusIsKnown only applies to parent and missing an update referrer to child. Seems we are missing that part.

Comment 2

[Dimi Lee [:dimi]]

This bug is not related to blacklist/whitelist in the URL Classifier (blacklist/whitelist work normally).

This bug happens because of document.referrer loads ReferrerInfo stored in the content process.
While content creates the ReferrerInfo, it doesn't know if the URL is a tracker or not. The tracking URL is annotated as a tracker in the parent process, and then the parent recomputes its ReferrerInfo. Since we don't sync the new ReferrInfo back to the child process so the result of document.referrer is not trimmed.

Comment 3

[(no longer active)]

You're right! Taking.

Comment 4

[(no longer active)]

Attachment: Bug 1589407 - Part 2: Ensure that document.referrer sees the same URL as what we send in the Referer HTTP header per the ETP restrictions;

Comment 5

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

Is this something we might want in 70, post-release?

Comment 6

[(no longer active)]

(In reply to Liz Henry (:lizzard) from comment #5)

Is this something we might want in 70, post-release?

Yes, I think that would be very nice.

Comment 7

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

Is this ready to land?

Comment 8

[(no longer active)]

(In reply to Liz Henry (:lizzard) from comment #7)

Is this ready to land?

Not yet. There are some test failures on try and the patch hasn't been reviewed yet...

What's the timeline for this to make it to 70.0.1?

Comment 9

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

I don't have a solid date yet (or a strong driver, though I have enough to start planning and uplifting). Tentatively, mid next week. If you think I should hold back for this issue, let me know.

Comment 10

[(no longer active)]

(In reply to Liz Henry (:lizzard) from comment #9)

I don't have a solid date yet (or a strong driver, though I have enough to start planning and uplifting). Tentatively, mid next week. If you think I should hold back for this issue, let me know.

The patch is now green on try. I hope mid-next week would be doable.

Thomas, do you have an ETA for the review?

Comment 11

[Thomas Nguyen (:tnguyen)]

(In reply to :ehsan akhgari from comment #10)

(In reply to Liz Henry (:lizzard) from comment #9)

I don't have a solid date yet (or a strong driver, though I have enough to start planning and uplifting). Tentatively, mid next week. If you think I should hold back for this issue, let me know.

The patch is now green on try. I hope mid-next week would be doable.

Thomas, do you have an ETA for the review?

I will do it today, thanks a lot.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ad44c165c690
Ensure that document.referrer sees the same URL as what we send in the Referer HTTP header per the ETP restrictions; r=tnguyen,baku

Comment 13

[Cristina Coroiu [:ccoroiu]]

Backed out changeset ad44c165c690 (bug 1589407) for browser chrome failures at toolkit/components/antitracking/test/browser/browser_referrerDefaultPolicy.js

Backout: https://hg.mozilla.org/integration/autoland/rev/2345817eb7b7c8d4266ad93d99b791c0fc56ec10

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ad44c165c69050a02573fbfcb11af7797fc5dc7e

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=272977431&repo=autoland&lineNumber=17466

[task 2019-10-25T14:41:56.226Z] 14:41:56 INFO - GECKO(7779) | --DOMWINDOW == 9 (0x7f8b3e503f20) [pid = 7935] [serial = 1] [outer = (nil)] [url = about:blank]
[task 2019-10-25T14:41:56.343Z] 14:41:56 INFO - [7758, Main Thread] WARNING: 'result.isErr()', file /builds/worker/workspace/build/src/startupcache/StartupCache.cpp, line 173
[task 2019-10-25T14:41:56.399Z] 14:41:56 INFO - [7758, Main Thread] WARNING: 'result.isErr()', file /builds/worker/workspace/build/src/startupcache/StartupCache.cpp, line 173
[task 2019-10-25T14:41:56.409Z] 14:41:56 INFO - GECKO(7779) | ++DOCSHELL 0x7f2b045e4000 == 2 [pid = 8455] [id = {40d318af-14a8-420b-bdf5-3d79d06959db}]
[task 2019-10-25T14:41:56.409Z] 14:41:56 INFO - GECKO(7779) | ++DOMWINDOW == 4 (0x7f2b05ab4a60) [pid = 8455] [serial = 4] [outer = (nil)]
[task 2019-10-25T14:41:56.409Z] 14:41:56 INFO - GECKO(7779) | ++DOMWINDOW == 5 (0x7f2b05ae3c00) [pid = 8455] [serial = 5] [outer = 0x7f2b05ab4a60]
[task 2019-10-25T14:41:56.492Z] 14:41:56 INFO - [7758, Main Thread] WARNING: 'result.isErr()', file /builds/worker/workspace/build/src/startupcache/StartupCache.cpp, line 173
[task 2019-10-25T14:41:56.588Z] 14:41:56 INFO - GECKO(7779) | ### XPCOM_MEM_BLOAT_LOG defined -- logging bloat/leaks to /tmp/tmpE3eY4r.mozrunner/runtests_leaks_tab_pid8484.log
[task 2019-10-25T14:41:56.616Z] 14:41:56 INFO - GECKO(7779) | [Child 8484, Main Thread] WARNING: No CID found when attempting to map contract ID: file /builds/worker/workspace/build/src/xpcom/components/nsComponentManager.cpp, line 732
[task 2019-10-25T14:41:56.631Z] 14:41:56 INFO - GECKO(7779) | [Child 8455, Main Thread] WARNING: Trying to request nsIHttpChannel from DocumentChannelChild, this is likely broken: file /builds/worker/workspace/build/src/netwerk/ipc/DocumentChannelChild.cpp, line 42
[task 2019-10-25T14:41:56.631Z] 14:41:56 INFO - GECKO(7779) | [Child 8455, Main Thread] WARNING: Trying to request nsIHttpChannel from DocumentChannelChild, this is likely broken: file /builds/worker/workspace/build/src/netwerk/ipc/DocumentChannelChild.cpp, line 42
[task 2019-10-25T14:41:56.632Z] 14:41:56 INFO - GECKO(7779) | [Child 8455, Main Thread] WARNING: NS_ENSURE_TRUE(request) failed: file /builds/worker/workspace/build/src/netwerk/base/nsLoadGroup.cpp, line 575
[task 2019-10-25T14:41:56.771Z] 14:41:56 INFO - GECKO(7779) | Couldn't convert chrome URL: chrome://branding/locale/brand.properties
[task 2019-10-25T14:41:56.900Z] 14:41:56 INFO - GECKO(7779) | [Child 8484, Main Thread] WARNING: could not set real-time limit at process startup: file /builds/worker/workspace/build/src/dom/ipc/ContentChild.cpp, line 1786
[task 2019-10-25T14:41:56.914Z] 14:41:56 INFO - GECKO(7779) | ++DOCSHELL 0x7fd4badb9000 == 1 [pid = 8484] [id = {3b912933-68d7-46b6-b91d-63fb4cac05dd}]
[task 2019-10-25T14:41:56.975Z] 14:41:56 INFO - GECKO(7779) | ++DOMWINDOW == 1 (0x7fd4bad45d40) [pid = 8484] [serial = 1] [outer = (nil)]
[task 2019-10-25T14:41:56.975Z] 14:41:56 INFO - GECKO(7779) | ++DOMWINDOW == 2 (0x7fd4bad71c00) [pid = 8484] [serial = 2] [outer = 0x7fd4bad45d40]
[task 2019-10-25T14:41:57.064Z] 14:41:57 INFO - GECKO(7779) | [Child 8484, Main Thread] WARNING: This content process hasn't received the permissions for http://example.net^privateBrowsingId=1 yet: file /builds/worker/workspace/build/src/extensions/permissions/nsPermissionManager.cpp, line 3318
[task 2019-10-25T14:41:57.064Z] 14:41:57 INFO - GECKO(7779) | Assertion failure: PermissionAvailable(aPrincipal, EmptyCString()), at /builds/worker/workspace/build/src/extensions/permissions/nsPermissionManager.cpp:2605
[task 2019-10-25T14:41:57.065Z] 14:41:57 INFO - GECKO(7779) | #01: (anonymous namespace)::CheckAntiTrackingPermission(nsIPrincipal*, nsTAutoStringN<char, 64ul> const&, bool, unsigned int*, unsigned int, nsIURI*) [toolkit/components/antitracking/AntiTrackingCommon.cpp:798]
[task 2019-10-25T14:41:57.066Z] 14:41:57 INFO -
[task 2019-10-25T14:41:57.067Z] 14:41:57 INFO - GECKO(7779) | #02: mozilla::AntiTrackingCommon::IsFirstPartyStorageAccessGrantedFor(nsIChannel*, nsIURI*, unsigned int*) [toolkit/components/antitracking/AntiTrackingCommon.cpp:1776]
[task 2019-10-25T14:41:57.068Z] 14:41:57 INFO -
[task 2019-10-25T14:41:57.068Z] 14:41:57 INFO - GECKO(7779) | #03: mozilla::(anonymous namespace)::ChooseOriginAttributes(nsIChannel*, mozilla::OriginAttributes&) [toolkit/components/antitracking/StoragePrincipalHelper.cpp:36]
[task 2019-10-25T14:41:57.069Z] 14:41:57 INFO -
[task 2019-10-25T14:41:57.069Z] 14:41:57 INFO - GECKO(7779) | #04: mozilla::StoragePrincipalHelper::Create(nsIChannel*, nsIPrincipal*, nsIPrincipal**) [toolkit/components/antitracking/StoragePrincipalHelper.cpp:80]
[task 2019-10-25T14:41:57.070Z] 14:41:57 INFO -
[task 2019-10-25T14:41:57.070Z] 14:41:57 INFO - GECKO(7779) | #05: nsScriptSecurityManager::GetChannelResultPrincipals(nsIChannel*, nsIPrincipal**, nsIPrincipal**) [caps/nsScriptSecurityManager.cpp:283]
[task 2019-10-25T14:41:57.070Z] 14:41:57 INFO -
[task 2019-10-25T14:41:57.071Z] 14:41:57 INFO - GECKO(7779) | #06: mozilla::dom::Document::Reset(nsIChannel*, nsILoadGroup*) [dom/base/Document.cpp:2310]
[task 2019-10-25T14:41:57.071Z] 14:41:57 INFO -
[task 2019-10-25T14:41:57.071Z] 14:41:57 INFO - GECKO(7779) | #07: nsHTMLDocument::Reset(nsIChannel*, nsILoadGroup*) [dom/html/nsHTMLDocument.cpp:180]

task 2019-10-25T14:53:54.676Z] 14:53:54 INFO - Buffered messages logged at 14:47:54
[task 2019-10-25T14:53:54.676Z] 14:53:54 INFO - Longer timeout required, waiting longer... Remaining timeouts: 4
[task 2019-10-25T14:53:54.676Z] 14:53:54 INFO - Buffered messages logged at 14:49:24
[task 2019-10-25T14:53:54.677Z] 14:53:54 INFO - Longer timeout required, waiting longer... Remaining timeouts: 3
[task 2019-10-25T14:53:54.677Z] 14:53:54 INFO - Buffered messages logged at 14:50:54
[task 2019-10-25T14:53:54.677Z] 14:53:54 INFO - Longer timeout required, waiting longer... Remaining timeouts: 2
[task 2019-10-25T14:53:54.680Z] 14:53:54 INFO - Buffered messages logged at 14:52:24
[task 2019-10-25T14:53:54.680Z] 14:53:54 INFO - Longer timeout required, waiting longer... Remaining timeouts: 1
[task 2019-10-25T14:53:54.680Z] 14:53:54 INFO - Buffered messages finished
[task 2019-10-25T14:53:54.681Z] 14:53:54 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/antitracking/test/browser/browser_referrerDefaultPolicy.js | Test timed out -
[task 2019-10-25T14:53:54.681Z] 14:53:54 INFO - GECKO(7779) | MEMORY STAT | vsize 2996MB | residentFast 373MB | heapAllocated 130MB
[task 2019-10-25T14:53:54.681Z] 14:53:54 INFO - TEST-OK | toolkit/components/antitracking/test/browser/browser_referrerDefaultPolicy.js | took 720225ms
[task 2019-10-25T14:53:54.682Z] 14:53:54 INFO - GECKO(7779) | ++DOCSHELL 0x7fe14c9b2000 == 1 [pid = 8060] [id = {0de14ce3-5c78-4ff5-a13f-d6a58de6137e}]
[task 2019-10-25T14:53:54.682Z] 14:53:54 INFO - GECKO(7779) | ++DOMWINDOW == 1 (0x7fe14c509100) [pid = 8060] [serial = 3] [outer = (nil)]
[task 2019-10-25T14:53:54.683Z] 14:53:54 INFO - GECKO(7779) | ++DOMWINDOW == 2 (0x7fe14d819400) [pid = 8060] [serial = 4] [outer = 0x7fe14c509100]
[task 2019-10-25T14:53:54.683Z] 14:53:54 INFO - checking window state
[task 2019-10-25T14:53:54.683Z] 14:53:54 INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-10-25T14:53:54.684Z] 14:53:54 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/antitracking/test/browser/browser_referrerDefaultPolicy.js | Found a browser window after previous test timed out -
[task 2019-10-25T14:53:54.685Z] 14:53:54 INFO - GECKO(7779) | ++DOMWINDOW == 3 (0x7fe14d820000) [pid = 8060] [serial = 5] [outer = 0x7fe14c509100]
[task 2019-10-25T14:53:54.685Z] 14:53:54 INFO - GECKO(7779) | must wait for focus
[task 2019-10-25T14:53:54.707Z] 14:53:54 INFO - GECKO(7779) | JavaScript error: resource://specialpowers/SpecialPowersParent.jsm, line 622: TypeError: this.manager.rootFrameLoader is null
[task 2019-10-25T14:53:54.711Z] 14:53:54 INFO - GECKO(7779) | [Parent 7779, Main Thread] WARNING: 'error.Failed()', file /builds/worker/workspace/build/src/dom/ipc/JSWindowActor.cpp, line 198
[task 2019-10-25T14:53:54.711Z] 14:53:54 INFO - GECKO(7779) | JavaScript error: , line 0: NS_ERROR_UNEXPECTED:

Comment 14

[(no longer active)]

Attachment: Bug 1589407 - Part 1: Avoid calling nsPermissionManager::GetAllForPrincipal() if no permissions are available in the current process;

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2df29a2401a9
Part 1: Avoid calling nsPermissionManager::GetAllForPrincipal() if no permissions are available in the current process; r=baku
https://hg.mozilla.org/integration/autoland/rev/0ec9e4f71228
Part 2: Ensure that document.referrer sees the same URL as what we send in the Referer HTTP header per the ETP restrictions; r=tnguyen,baku

Comment 16

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/2df29a2401a9
https://hg.mozilla.org/mozilla-central/rev/0ec9e4f71228

Comment 17

[(no longer active)]

Comment on attachment 9102587 [details]
Bug 1589407 - Part 2: Ensure that document.referrer sees the same URL as what we send in the Referer HTTP header per the ETP restrictions;

Beta/Release Uplift Approval Request

• User impact if declined: Third-party tracker iframes can receive the full (untrimmed) referrer URL through document.referrer, which potentially leaks information about the website the user is visiting.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: High
• Why is the change risky/not risky? (and alternatives if risky): This is a fairly high risk uplift, since the patch touches the code involved in initiating HTTP connections, as well as the IPC between the content and the parent process involved in setting up the HTTP channel. The patch has an automated test and passes all of the existing ones we have, but that being said it is possible for it to cause crashes, or even worse regressions in the functionality of Necko in unforeseeable ways. We should watch out for any fall out from this after the uplift, but exposing it to more users is also a good way to get the testing that it needs.
• String changes made/needed: Nonee

Comment 18

[(no longer active)]

(In reply to Liz Henry (:lizzard) from comment #9)

I don't have a solid date yet (or a strong driver, though I have enough to start planning and uplifting). Tentatively, mid next week. If you think I should hold back for this issue, let me know.

Liz, after some more thought about this, I'm no longer comfortable to crash-land this into the first dot-release, mostly due to the potential risk since I believe this is a pretty high risk uplift... I've nominated it for beta uplift for now. I think once it's exposed to the beta population perhaps we should consider it for a future dot release if we decide to have one, otherwise maybe even wait until 71, I'm a bit nervous about regressing something while trying to rush this into the release and cause more problems than this would solve.

Comment 19

[Pascal Chevrel:pascalc]

Ehsan, since this is a high risk uplift and it landed recently in nightly, is there an urgency to not let it a week on Nightly before taking it to beta? I am guessing this is a P1/critical bug given that you were considering an uplift to a dot release but before taking it on beta, I would prefer seeing it on nightly more than 2 days.

Comment 20

[(no longer active)]

(In reply to Pascal Chevrel:pascalc from comment #19)

Ehsan, since this is a high risk uplift and it landed recently in nightly, is there an urgency to not let it a week on Nightly before taking it to beta? I am guessing this is a P1/critical bug given that you were considering an uplift to a dot release but before taking it on beta, I would prefer seeing it on nightly more than 2 days.

This is a pretty critical bug indeed, and I'd like to keep the option open to backport it in the next (0.2?) dot release to the release branch. That being said I'm not sure about the timeline of that release and I also agree that it would be nice to get more than a couple of days of Nightly testing before backporting it to beta. So I'll leave the decision on when exactly to approve it for beta uplift to you based on this information. Please let me know if you need more details from me to help make the decision, thanks!

Comment 21

[Pascal Chevrel:pascalc]

I will let it bake on Nightly a few days before reevaluating an uplift to beta.

Comment 22

[Pascal Chevrel:pascalc]

Comment on attachment 9102587 [details]
Bug 1589407 - Part 2: Ensure that document.referrer sees the same URL as what we send in the Referer HTTP header per the ETP restrictions;

P1 critical, high risk but has tests and we need in in Beta first to allow a potential dot release, uplift approved for 71 beta 7, thanks.

Comment 23

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d891880b6b84
https://hg.mozilla.org/releases/mozilla-beta/rev/a986a016e8c5

Comment 24

[Daniel Bodea [:danibodea]]

I am attempting to verify this fix in Nightly v72.0a1 and Beta v71.0b7 on Windows 10. Both versions appear somewhat fixed, BUT I am not sure about the steps to reproduce because of the "urlclassifier.trackingAnnotationTable.testEntries" pref does not exist and I created it with the requested value and it does not influence the reproduction/non-reproduction of the issue. Furthermore, there is another pref called "urlclassifier.trackingAnnotationTable" and I attempted adding the requested value to this pref's list, but it still made no influence in the reproduction of the issue. This being said, I am uncertain how the first step from the bug's description is necessary or relevant.

If the modification in about:config would actually be irrelevant, then it seems fixed:

• Nightly v71.0a1 (2019-10-17), Beta v71.0b6 (64-bit):
  The string displayed in the second frame "Tracker (trackertest.org)" from the test page is:
  "The referrer accessible via document.referrer is:
  https://senglehardt.com/test/trackingprotection/test_pages/referrer_policy.html"
  -> The issue is reproduced.
• Nightly v72.0a1 (2019-11-04), Beta v71.0b7 (64-bit):
  The string displayed in the second frame "Tracker (trackertest.org)" from the test page is:
  "The referrer accessible via document.referrer is:
  https://senglehardt.com/"
  -> The issue appears fixed.

@ehsan: Is the first step of reproduction from comment 0 really necessary? Can this bug be verified with the test results I reported?
Thanks!

Comment 25

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

I'd like to let this wait till 71 (only 3 weeks away) if you don't object.

Comment 26

[(no longer active)]

(In reply to Bodea Daniel [:danibodea] from comment #24)

I am attempting to verify this fix in Nightly v72.0a1 and Beta v71.0b7 on Windows 10. Both versions appear somewhat fixed, BUT I am not sure about the steps to reproduce because of the "urlclassifier.trackingAnnotationTable.testEntries" pref does not exist

Yes it is a hidden preference.

and I created it with the requested value and it does not influence the reproduction/non-reproduction of the issue.

Using about:url-classifier to test, entering https://trackertest.org shows that domain is now considered as a tracker so step 1 of the STR in comment 0 is no longer needed (that wasn't the case when the bug was filed). That explains why it doesn't influence the reproduction/non-reproduction of the issue.

Furthermore, there is another pref called "urlclassifier.trackingAnnotationTable" and I attempted adding the requested value to this pref's list, but it still made no influence in the reproduction of the issue.

Of course, that is a totally unrelated pref. :-)

This being said, I am uncertain how the first step from the bug's description is necessary or relevant.

It's now irrelevant, the only step needed is step 2.

If the modification in about:config would actually be irrelevant, then it seems fixed:

• Nightly v71.0a1 (2019-10-17), Beta v71.0b6 (64-bit):
  The string displayed in the second frame "Tracker (trackertest.org)" from the test page is:
  "The referrer accessible via document.referrer is:
  https://senglehardt.com/test/trackingprotection/test_pages/referrer_policy.html"
  -> The issue is reproduced.
• Nightly v72.0a1 (2019-11-04), Beta v71.0b7 (64-bit):
  The string displayed in the second frame "Tracker (trackertest.org)" from the test page is:
  "The referrer accessible via document.referrer is:
  https://senglehardt.com/"
  -> The issue appears fixed.

Then this looks verified to me. Thanks a lot!

(In reply to Liz Henry (:lizzard) from comment #25)

I'd like to let this wait till 71 (only 3 weeks away) if you don't object.

This is good to go when you're ready from my perspective. Thanks!

Comment 27

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

With only 2 weeks to go till 71 release, another 70 dot release is unlikely. Thanks for getting this in place for 71!

Comment 28

[Alin Ilea, Desktop Test Engineering]

Based on comment 26 and also given the fact that the issue is not reproducible on latest Nightly 75.0a1 (Build id: 20200210213347), I will change the status accordingly.
Thanks.