Closed Bug 1258033 Opened 4 years ago Closed 4 years ago

Fix the DNT loophole for tracking protection

Categories

(Toolkit :: Safe Browsing, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla49
Tracking Status
firefox49 --- verified

People

(Reporter: francois, Assigned: dimi)

References

(Depends on 1 open bug)

Details

User Story

If a site gets removed from the Disconnect list of trackers because they honor the DNT signal, TP stops blocking it, but it doesn't guarantee that tracking will be disabled since it relies on the user enabling DNT separately (as pointed out in bug 1225143).

We intend to send all sites a DNT header when the user is in TP mode.

Attachments

(4 files, 17 obsolete files)

6.07 KB, patch
dimi
: review+
Details | Diff | Splinter Review
5.70 KB, patch
dimi
: review+
Details | Diff | Splinter Review
4.94 KB, patch
dimi
: review+
Details | Diff | Splinter Review
7.78 KB, patch
dimi
: review+
Details | Diff | Splinter Review
Blocks: 1029886
Depends on: 1258037
Depends on: 1258038
Depends on: 1258039
Depends on: 1258041
No longer depends on: 1258037
Depends on: 1258043
Depends on: 1258037
Selectively sending the DNT header to sites on a DNT-compliant list wasn't going to work with the DOM API for DNT, so we decided to instead send all sites a DNT header when the user is in TP mode.
Assignee: nobody → dlee
Depends on: 1274654
There are two places where we send the DNT signal:

- the DoNotTrack header: https://dxr.mozilla.org/mozilla-central/rev/c67dc1f9fab86d4f2cf3224307809c44fe3ce820/netwerk/protocol/http/nsHttpHandler.cpp#492-498
- the DoNotTrack DOM API: https://dxr.mozilla.org/mozilla-central/rev/c67dc1f9fab86d4f2cf3224307809c44fe3ce820/dom/base/Navigator.cpp#760-770

They currently just look at privacy.donottrackheader.enabled and that needs
to be replaced with something like:

    enableDNT = getbool(privacy.donottrackheader.enabled) || TrackingProtectionEnabled()

where TrackingProtectionEnabled() essentially does this check:

    if (!Preferences::GetBool("privacy.trackingprotection.enabled", false) &&
        (!Preferences::GetBool("privacy.trackingprotection.pbmode.enabled",
                               false) || !NS_UsePrivateBrowsing(aChannel))) {
      return false;
    } else {
      return true;
    }

See: https://dxr.mozilla.org/mozilla-central/rev/c67dc1f9fab86d4f2cf3224307809c44fe3ce820/netwerk/base/nsChannelClassifier.cpp#69-73

Ideally, instead of duplicating that check, we should refactor it into a
separate function and then call it from both places. Maybe that function
should live in the URI Classifier or in the HttpChannel itself, I'm not
sure.

The four cases we need to test are:

- DNT turned on,  TP turned off               -> DNT signal sent both in private browsing and normal mode
- DNT turned off, TP turned on globally       -> DNT signal sent both in private browsing and normal mode
- DNT turned off, TP in Private Browsing only -> DNT signal sent in private browsing mode only
- DNT turned off, TP turned off               -> DNT signal is never sent

It would be good to test that the "DNT" request header is the right one and
that the "navigator.DoNotTrack()" JavaScript API:

https://developer.mozilla.org/en-US/docs/Web/API/Navigator/doNotTrack

returns the right value too.
Depends on: 1274712
Attached patch WIP patch (obsolete) — Splinter Review
User Story: (updated)
Status: NEW → ASSIGNED
Priority: -- → P2
Attachment #8755402 - Attachment is obsolete: true
Attachment #8756682 - Flags: review?(francois)
Hi Francois,
The reason i move set DNT header from |Init| to |begineConnect| is because now we will set 'DNT' based on private browsing mode and in the |Init| it is too early to check if this channel is in private browsing mode.

But I am not really sure where is the best place to put the header check, i guess we will need a necko peer to review this also.
Attachment #8756683 - Flags: review?(francois)
Attached patch P4. Testcase (obsolete) — Splinter Review
The testcase covers suggestions by francois in comment2
Attachment #8756685 - Flags: review?(francois)
Comment on attachment 8756683 [details] [diff] [review]
P2. Set DNT header based on preference and private browsing mode

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

Looks fine to me, but we should ask a necko peer to review this one. I don't know the implications of moving the place where we check the pref and set the header.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +7548,5 @@
> +nsHttpChannel::SetDoNotTrack()
> +{
> +  /**
> +   * 'DoNotTrack' header should be added if 'privacy.donottrackheader.enabled'
> +   * is true or tracking protection is enabled.

Let's reference this bug in the comment by adding "(see bug 1258033)" at the end of this comment.
Attachment #8756683 - Flags: review?(francois) → feedback+
Comment on attachment 8756684 [details] [diff] [review]
P3. Return navigator.doNotTrack based on preference and private browsing mode

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

::: dom/base/Navigator.cpp
@@ +777,5 @@
> +  nsCOMPtr<nsILoadContext> loadContext;
> +  win->GetInterface(NS_GET_IID(nsILoadContext), getter_AddRefs(loadContext));
> +
> +  bool isPrivateBrowsing = false;
> +  loadContext->GetUsePrivateBrowsing(&isPrivateBrowsing);

I wonder if we couldn't put the IsTrackingProtectionEnabled util function in the load context or the channel instead?

It would be good to avoid duplicating the pref checking if we can because there might be other ways for tracking protection to be enabled in the future.

This (if it's possible) would be quite nice:

  bool isTrackingProtectionEnable = false
  loadContext->GetTrackingProtectionEnabled(&isTrackingProtectionEnabled)
Attachment #8756684 - Flags: review?(francois)
Comment on attachment 8756685 [details] [diff] [review]
P4. Testcase

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

Very thorough test, thanks!

Only a small issue around pref cleanup at the end of the test.

::: toolkit/components/url-classifier/tests/mochitest/test_donottrack.html
@@ +73,5 @@
> +const contentPage =
> +  "http://mochi.test:8888/tests/toolkit/components/url-classifier/tests/mochitest/dnt.html";
> +
> +function executeTest(test) {
> +  SpecialPowers.setBoolPref(DNT_PREF, test.setting.dntPref);

Maybe use SpecialPowers.pushPrefEnv() and popPrefEnv() instead?

We should clean up the prefs at the end to avoid interfering with any other tests.
Attachment #8756685 - Flags: review?(francois) → review-
Comment on attachment 8756682 [details] [diff] [review]
P1. Support IsTrackingProtectionEnabled util function

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

This looks good, but it may need to move somewhere else if you find a way to address comment 9.

::: netwerk/base/nsChannelClassifier.cpp
@@ +587,5 @@
> +// static
> +bool
> +nsChannelClassifier::IsTrackingProtectionEnabled(nsIChannel *aChannel)
> +{
> +  if (!Preferences::GetBool("privacy.trackingprotection.enabled", false) &&

nit: it might be a good time to rewrite this check so that it's easier to read

For example:

if (privacy.trackingprotection.enabled) {
  return true
}
if (privacy.trackingprotection.pbmode && NS_UsePrivateBrowsing(channel)) {
  return true
}
return false
Attachment #8756682 - Flags: review?(francois)
Attachment #8756682 - Attachment is obsolete: true
Attachment #8756683 - Attachment is obsolete: true
Attachment #8756684 - Attachment is obsolete: true
Attachment #8756685 - Attachment is obsolete: true
Attachment #8757183 - Flags: review?(bugs)
hi Olli,
Could you help review if it is ok to add new API for nsILoadContext ? The purpose for this API is to know if tracking protection is on for the load context based on preference and private browsing mode.
Thanks!
Hi Patrick,
The patch is to add DNT header based on preference and tracking protection mode.
Comment 5 explains why i change the place to set DNT header.

Another question is that i keep |mDoNotTrackEnabled| for telemetry[1]
But should i also update how we record the Telemetry::DNT_USAGE since now if DNT header should be sent is changed.

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpHandler.cpp#2144
Attachment #8757189 - Flags: review?(mcmanus)
Attachment #8757192 - Flags: review?(francois)
Attached patch P5. Testcase (obsolete) — Splinter Review
Address Comment 10
Attachment #8757194 - Flags: review?(francois)
Comment on attachment 8757183 [details] [diff] [review]
P1. Add IsTrackingProtectionOn for nsILoadContext


>+
>+  /**
>+   * Returns true if tracking protection is enabeld for the load context.
enabled

>+   */
>+  [noscript] boolean IsTrackingProtectionOn();
Why noscript? A tad odd in an interface which isn't marked to be builtinclass

>+
>+%{C++
>+  /**
>+   * De-XPCOMed getter to make call-sites cleaner.
>+   */
>+  bool UseTrackingProtection() {
>+    bool usingTP;
>+    IsTrackingProtectionOn(&usingTP);
>+    return usingTP;
>+  }
>+%}
Ahaa, [infallible] readonly attribute wouldn't work since the interface isn't builtinclass.
Attachment #8757183 - Flags: review?(bugs) → review+
QA Contact: mwobensmith
Comment on attachment 8757190 [details] [diff] [review]
P3. navigator.doNotTrack should be determinded by preference and tracking protection

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

I would suggest getting a DOM peer to review this one too once you've added the null check.

::: dom/base/Navigator.cpp
@@ +757,5 @@
>  Navigator::GetDoNotTrack(nsAString &aResult)
>  {
> +  aResult.AssignLiteral("unspecified");
> +
> +  nsGlobalWindow* win = nsGlobalWindow::Cast(mWindow);

I would add a null check like NS_ENSURE_TRUE(win, NS_OK);
Attachment #8757190 - Flags: review?(francois)
Comment on attachment 8757192 [details] [diff] [review]
P4. Use LoadContext API for channel classifier

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

Looks good to me, perhaps roll it into the other necko patch so that mcmanus looks at both of them at once?
Attachment #8757192 - Flags: review?(francois)
Comment on attachment 8757194 [details] [diff] [review]
P5. Testcase

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

Looks great!
Attachment #8757194 - Flags: review?(francois) → review+
Merge P2 and P4 to review based on francois's suggestion.
Purpose of this patch is describe in Comment 5 and Comment 14
Attachment #8757189 - Attachment is obsolete: true
Attachment #8757189 - Flags: review?(mcmanus)
Attachment #8757536 - Flags: review?(mcmanus)
Hi Olli,
Could you also help review this patch ? thanks!
Attachment #8757190 - Attachment is obsolete: true
Attachment #8757537 - Flags: review?(bugs)
Comment on attachment 8757192 [details] [diff] [review]
P4. Use LoadContext API for channel classifier

These got merged (see comment 22).
Attachment #8757192 - Attachment is obsolete: true
Comment on attachment 8757537 [details] [diff] [review]
P3. navigator.doNotTrack should be determinded by preference and tracking protection v2

Could we figure out a way to not lose the boolvarcache part.
Maybe add AddBoolVarCache to nsContentUtils and use the same value in dochshell and in this code.
This is not hot code or anything, but one never knows what some silly DOM benchmarks do. And there isn't any reason to not use varcache.
And besides, that reduces code duplication when the reading of the pref is handled in one place.

I know, this is über-nit.
Attachment #8757537 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #18)
> >+   */
> >+  [noscript] boolean IsTrackingProtectionOn();
> Why noscript? A tad odd in an interface which isn't marked to be builtinclass

Yes, you are right, i shouldn't add noscript, will remove it in next patch
(In reply to Olli Pettay [:smaug] from comment #25)
> Could we figure out a way to not lose the boolvarcache part.
> Maybe add AddBoolVarCache to nsContentUtils and use the same value in
> dochshell and in this code.

The docshell did not use the "privacy.donottrackheader.enabled", but in both navigator and nsHttpChannel in patch P2 will use this preference.
I will add boolvarcache in nsContentUtils, thanks for you suggestion!
Address Comment 18
Attachment #8757183 - Attachment is obsolete: true
Attachment #8757771 - Flags: review+
The |nsContentUtils::DoNotTrackEnabled| should be also used by |nsHttpChannel| in patch Part2. I will add it after the patch is reviewed.
Attachment #8757537 - Attachment is obsolete: true
Attachment #8757772 - Flags: review?(bugs)
Comment on attachment 8757772 [details] [diff] [review]
P3. navigator.doNotTrack should be determinded by preference and tracking protection v3

> Navigator::GetDoNotTrack(nsAString &aResult)
> {
>-  if (sDoNotTrackEnabled) {
>+  aResult.AssignLiteral("unspecified");
>+
>+  nsGlobalWindow* win = nsGlobalWindow::Cast(mWindow);
>+  NS_ENSURE_TRUE(win, NS_OK);
So this is a change to the behavior, returning unspecified for disconnected Navigator.

>+
>+  if ((loadContext && loadContext->UseTrackingProtection()) ||
>+      nsContentUtils::DoNotTrackEnabled()) {
>     aResult.AssignLiteral("1");
>-  } else {
>-    aResult.AssignLiteral("unspecified");
>   }

Perhaps you could write this:

bool doNotTrack = nsContentUtils::DoNotTrackEnabled();
if (!doNotTrack) {
  nsCOMPtr<nsILoadContext> loadContext = do_GetInterface(mWindow);
  doNotTrack = loadContext && loadContext->UseTrackingProtection();
}

if (doNotTrack) {
  aResult.AssignLiteral("1");
} else {
  aResult.AssignLiteral("unspecified");
}

return NS_OK;


>+nsContentUtils::DoNotTrackEnabled()
>+{
Given that we're planning to use this in Necko, want to add 
MOZ_ASSERT(NS_IsMainThread());


with that all, r+
Attachment #8757772 - Flags: review?(bugs) → review+
Attachment #8757536 - Flags: review?(mcmanus) → review+
Hi Olli,
Sorry i missed the serialized load context part for review, could you help review again ? thanks!

One thing I am not sure is in |TabParent::GetLoadContext|[1], in this patch I didn't pass "tracking protection on/off" to LoadContext constructor, should I pass it ? If yes, do you have any suggestion what is a better way to do it. Because I am not sure if check private browsing mode and preference here is a good idea.

[1]https://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp#2776
Attachment #8758648 - Flags: review?(bugs)
(In reply to Dimi Lee[:dimi][:dlee] from comment #31)
> Because I am not sure if check private browsing mode and
> preference here is a good idea.
Why not?
That is what I'd expect. Then we'd get consistent handling with docshell.
Comment on attachment 8758648 [details] [diff] [review]
Missing serialized load context(This should be merged to P1)

So based on that I assume a new patch. Or if you think we for some reason should use the pb flag and/or pref in loadcontext, please explain why not.
Attachment #8758648 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #32)
> (In reply to Dimi Lee[:dimi][:dlee] from comment #31)
> > Because I am not sure if check private browsing mode and
> > preference here is a good idea.
> Why not?
> That is what I'd expect. Then we'd get consistent handling with docshell.

Since IsTrackingProtectionOn is based on pref and pb flag, and LoadContext already has pb flag(mUsePrivateBrowsing). I am thinking if we should implement the check directly in the LoadContext::IsTrackingProtectionOn instead of passing it in TabParent.
oh you mean that. Yes I was thinking too that it should be implemented in LoadContext::IsTrackingProtectionOn based on the existing pb flag and pref.
(In reply to Olli Pettay [:smaug] from comment #35)
> oh you mean that. Yes I was thinking too that it should be implemented in
> LoadContext::IsTrackingProtectionOn based on the existing pb flag and pref.

Sorry I didn't explain it clearly in previous comment.

In the TabParent case, it looks like a good idea implement it in the LoadContext::IsTrackingProtectionOn itself. But LoadContext is also used in ipc[1], in this case, it seems LoadContext::IsTrackingProtectionOn should represent the value passed in SerializedLoadContext, not pref + pb flag. I am not really familiar with loadContext, so not sure what is the best way here.

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/ipc/NeckoParent.cpp#200
Ask for review again based on Olli's suggestion in Comment 35.
Attachment #8758648 - Attachment is obsolete: true
Attachment #8758715 - Flags: review?(bugs)
Attachment #8758715 - Flags: review?(bugs) → review+
Merge P1 and "Implement IsTrackingProtection in LoadContext" patch
Attachment #8757771 - Attachment is obsolete: true
Attachment #8758715 - Attachment is obsolete: true
Attachment #8758999 - Flags: review+
This is original P3, but according to Comment 25 and Comment 27. Add |nsContentUtils::DoNotTrackEnabled| to this patch and it will also be used by necko in P2.
So reverse the order of p2 and p3.
Attachment #8757772 - Attachment is obsolete: true
Attachment #8759000 - Flags: review+
Move this to P3
Attachment #8757536 - Attachment is obsolete: true
Attachment #8759001 - Flags: review+
Attachment #8757194 - Attachment is obsolete: true
Attachment #8759002 - Flags: review+
fix a error that testcase may timeout
Attachment #8759002 - Attachment is obsolete: true
Attachment #8759081 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/e4a2e5f30099
Part 1: Add IsTrackingProtectionOn for nsILoadContext. r=smaug
https://hg.mozilla.org/integration/fx-team/rev/9df3db7f35d8
Part 2: navigator.doNotTrack should be determinded by preference and tracking protection. r=smaug
https://hg.mozilla.org/integration/fx-team/rev/1311e22e3ab4
Part 3: Set DNT header based on preference and tracking protection. r=mcmanus
https://hg.mozilla.org/integration/fx-team/rev/001698fcd787
Part 4: Testcase for DNT. r=francois
Keywords: checkin-needed
Tested using steps in comment 2. I looked at the various combinations of tracking protection, private browsing and DNT. I also verified the value of navigator.doNotTrack under the same conditions.

All seem to behave as expected. We are sending the DNT header in the cases we want to, and not sending it otherwise. In addition, the return value of that JS API reflects the same.

Verified on Nightly, 2016-06-08.
Status: RESOLVED → VERIFIED
Depends on: 1448235
See Also: → 1476156
And now how to disable DNT when TP
is enabled? Is there some way remained
at least via about:config?
The aforementioned loop-hole does not
exist if I set TP to always on (instead
of selected by the Disconnect list) and
DNT to always off. And there is no longer
a possibility to get such config?
(In reply to Stas Sergeev from comment #48)
> And now how to disable DNT when TP
> is enabled? Is there some way remained
> at least via about:config?

DNT cannot be disabled if TP is enabled. This is by design. See the "user story" field at the top of this bug.
(In reply to François Marier [:francois] from comment #49)

> DNT cannot be disabled if TP is enabled. This is by design. See the "user
> story" field at the top of this bug.

And how about seeing my msg instead?
---
The aforementioned loop-hole does not
exist if I set TP to always on (instead
of selected by the Disconnect list) and
DNT to always off.
---

Not sure if I can explain this even clearer.
As always, useful config was removed for
no good reason.
(In reply to Stas Sergeev from comment #50)
> The aforementioned loop-hole does not
> exist if I set TP to always on (instead
> of selected by the Disconnect list) and
> DNT to always off.

I don't understand the configuration you're describing. Tracking protection relies on the list of trackers curated by Disconnect. That's how it determines whether or not a third-party resource will be blocked.
(In reply to François Marier [:francois] from comment #51)
> I don't understand the configuration you're describing. Tracking protection
> relies on the list of trackers curated by Disconnect. That's how it
> determines whether or not a third-party resource will be blocked.

OK, let me explain the setup I look for.
https://disconnect.me/trackerprotection
---
Tracking is the collection of data regarding a particular user's activity across multiple websites or applications that aren’t owned by the data collector, and the retention, use or sharing of that data.
---

What I want is firefox to disallow the
aforementioned sharing of any data between
sites. For example the use of non-owned cookies.
In this case even if the tracker is removed
from Disconnect list, it will not be able to
track. So I can keep DNT turned off and there
is no aforementioned loop-hole.
Is such setup possible?
> ---
> Tracking is the collection of data regarding a particular user's activity across multiple websites
> or applications that aren’t owned by the data collector, and the retention, use or sharing of that data.
> ---
> What I want is firefox to disallow the aforementioned sharing of any data between sites.

That high-level goal is more-or-less what we are trying to do with our anti-tracking effort: https://blog.mozilla.org/futurereleases/2018/08/30/changing-our-approach-to-anti-tracking/

The hope is that once that work is complete, you'll be able to prevent tracking without turning ON tracking protection or DNT.
> The hope is that once that work is complete, you'll be able to prevent tracking without turning ON tracking protection

I was assuming this is (or should be)
a part of the tracking protection...
Thanks for the clear answer!
Waiting for ff-65 then.
See Also: → 1495192
You need to log in before you can comment on or make changes to this bug.