Closed Bug 587523 Opened 14 years ago Closed 6 years ago

Protect path of HTTP Referer Header when in Private Browsing

Categories

(Core :: Networking: HTTP, enhancement, P2)

Product:
Core
Component:
Networking: HTTP
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
relnote-firefox --- 59+
firefox59 --- verified
firefox60 --- verified

People

(Reporter: chris, Assigned: groovecoder)

References

Details

(Keywords: privacy, Whiteboard: [necko-next])

Attachments

(5 files)

Bug 587523 - strict-origin-when-cross-origin referer policy in pbmode
59 bytes, text/x-review-board-request
valentin
: review+
Details
Bug 587523 - rename referer.userControlPolicy to referer.defaultPolicy
59 bytes, text/x-review-board-request
valentin
: review+
Details
Bug 587523: update test_referrer.js to include pbmode tests
59 bytes, text/x-review-board-request
valentin
: review+
Details
Bug 587523: update Fetch API referrer to check for private browsing pref
59 bytes, text/x-review-board-request
valentin
: review+
Details
Bug 587523: remove branching logic; comment fixups
59 bytes, text/x-review-board-request
valentin
: review+
Details 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/534.3 (KHTML, like Gecko) Chrome/6.0.472.33 Safari/534.3
Build Identifier: 

The browser's http referer header is a source of significant amount of private data leakage. See http://www.cs.wpi.edu/~cew/papers/wosn08.pdf and http://online.wsj.com/article/NA_WSJ_PUB:SB10001424052748704513104575256701215465596.html as an example of something that was fixed by particular sites (Facebook).

One issue that has not been fixed yet, is the fact that users' search terms leak to 3rd party sites via the referer header, when they click on results from the search engine results page.

An example of this can be seen by searching for 'no knead bread' with Google, and clicking on the 4th search result, which takes you to www.breadtopia.com/basic-no-knead-method/, a page which "helpfully" lets you know that it is aware of the search terms that brought you to the site.

This bug (https://bugzilla.mozilla.org/show_bug.cgi?id=55477) has quite a bit of debate about the idea of stripping some info from the referer header. One of the good ideas in that bug is the idea of a REFERRER_3RDPARTY_NO_PREPATH option, which "Strip[s] off the path from the referrer for 3rd party requests, otherwise leave[s] it alone."

Under such a model, a user visiting wikipedia.com, and clicking on a link to another page on wikipedia would still have the full referer transmitted. However, a user clicking on a link from Google.com's results page to a 3rd party site would result in the referer of "http://www.google.com" being sent. 

Looking through that and other bug reports, the two main  positive use cases for the delivery of the referer header to 3rd party sites seem to be:

1. Stopping bandwidth leeching. E.g. Stopping other sites from including images from your site, which cost you bandwidth when users visit those sites.
2. Analytics: Helping webmasters to figure out where their traffic is coming from.

Item 1 is easy to solve, since even with just the domain in the referer header, it would still be easy to determine that a myspace.com user had embedded your content in their site. You wouldn't know which myspace user had done so, but you could still easily block such requests (or give them a different image).

With regard to item 2. Webmasters simply do not have any natural right to know where their users are coming from. Yes, this is how the web has always worked, but it doesn't need to stay this way, particularly when the user has expressed a desire to be private, by turning on private browsing mode.

By switching to a model of scrubbing the path, but not the domain, from the referer header, these 3rd party sites would still have a rough idea of where users are coming from, but wouldn't learn the exact page the user is on. For sites that include private info in the URL (for example: http://www.webmd.com/breast-cancer/default.htm), this would lead to a significant improvement in user privacy.

Furthermore, for webmasters that want to find out what search terms are drawing users, Google already offers aggregate stats for individual webmasters, which can be viewed at http://www.google.com/webmaster. These webmasters would merely be denied this info about individual users in real time, and would instead have to make do with aggregate info.

What I propose is adding this option to strip the path from the referer headers sent to third party sites. This option should not be enabled by default, but if a user wishes to go into about:config and enable it, so be it. However, this option would be enabled whenever the user goes into private browsing mode. Once the user leaves private browsing mode, their browser will back to sending full headers again.

Reproducible: Always
dupe of bug 467257 ?
This bug is more general about limiting the amount of information to 3rd parties. Something which is also useful for regular mode too (and better than blocking all http-referers).
This is definitely a dupe of bug 467257 as far as private browsing is concerned, as its out of the scope of that feature, however, Sid has been talking about an anonymity mode, which would be the perfect venue for this change.  Hence, I'm re-summarizing.
Summary: Protect path of HTTP Referer Header when in private browsing mode → Protect path of HTTP Referer Header when in a possible future anonymous mode
Keywords: privacy
Blocks: 822869
Blocks: 61660
comment 3 wrt PB.. potentially new bug for things that aren't captured by other referer changes - this one isn't very actionable
Status: UNCONFIRMED → RESOLVED
Closed: 8 years ago
Resolution: --- → INCOMPLETE
There is an about:config pref network.http.sendRefererHeader that users could change to restrict the referrer.  Users could select origin only or they could decide to not send any referrer.
Now that we have increased the scope of Private Browsing (i.e. by enabling Tracking Protection), it seems like we should consider tweaking the default referrer.

Given we've just implemented the referrer policy spec, we could simply set the default referrer policy to strict-origin-when-cross-origin [1] while in Private Browsing instead of no-referrer-when-downgrade [2]. It would reduce the amount of information that's leaked by that header.

[1] https://w3c.github.io/webappsec-referrer-policy/#referrer-policy-strict-origin-when-cross-origin
[2] https://w3c.github.io/webappsec-referrer-policy/#referrer-policy-no-referrer-when-downgrade
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: INCOMPLETE → ---
(In reply to François Marier [:francois] from comment #6)
> Now that we have increased the scope of Private Browsing (i.e. by enabling
> Tracking Protection), it seems like we should consider tweaking the default
> referrer.
> 
> Given we've just implemented the referrer policy spec, we could simply set
> the default referrer policy to strict-origin-when-cross-origin [1] while in
> Private Browsing instead of no-referrer-when-downgrade [2]. It would reduce
> the amount of information that's leaked by that header.
> 
> [1]
> https://w3c.github.io/webappsec-referrer-policy/#referrer-policy-strict-
> origin-when-cross-origin
> [2]
> https://w3c.github.io/webappsec-referrer-policy/#referrer-policy-no-referrer-
> when-downgrade

+1
Depends on: 1276836
Summary: Protect path of HTTP Referer Header when in a possible future anonymous mode → Protect path of HTTP Referer Header when in Private Browsing
Whiteboard: [necko-next]
Assignee: nobody → valentin.gosu
See Also: → 1304623
One way to do this would be to this, now that bug 1304623 has landed, would be to add a new `network.http.referer.userControlPolicy.pbmode` pref similar to `network.http.referer.userControlPolicy`.

We could set the default to `strict-origin-when-cross-origin` (i.e. only the origin is sent on cross-origin requests or navigation) and sites could override it via the referrer policy if they need to.

If we later find that sites are abusing this, then we can change to using something like     `network.http.referer.XOriginTrimmingPolicy.pbmode` instead, which would trim the referrer without any overrides.
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P2
Comment on attachment 8937909 [details]
Bug 587523 - strict-origin-when-cross-origin referer policy in pbmode

https://reviewboard.mozilla.org/r/208604/#review214510

::: netwerk/base/nsNetUtil.cpp:84
(Diff revision 1)
>  #include <limits>
>  
>  using namespace mozilla;
>  using namespace mozilla::net;
>  
> -#define DEFAULT_USER_CONTROL_RP 3
> +#define DEFAULT_CONTROL_RP 3

You could remove the word "CONTROL" here too, it's kind of weird to have "DEFAULT CONTROL".

::: netwerk/base/nsNetUtil.cpp:90
(Diff revision 1)
> +
> +static uint32_t sDefaultControlRp = DEFAULT_CONTROL_RP;
> +
> +#define DEFAULT_PRIVATE_CONTROL_RP 2
> +
> +static uint32_t sDefaultPrivateControlRp = DEFAULT_PRIVATE_CONTROL_RP;

nit: I would group the two `#define` together and the two static variables together so that this takes less vertical space.

::: netwerk/base/nsNetUtil.cpp:3012
(Diff revision 1)
> -                                          "network.http.referer.userControlPolicy",
> -                                          DEFAULT_USER_CONTROL_RP);
> +                                          "network.http.referer.defaultPolicy",
> +                                          DEFAULT_CONTROL_RP);
> +    preferencesInitialized = true;
> +  }
> +
> +  switch (sDefaultControlRp) {

The code duplication is a little annoying. This could get out of sync in the future.

Maybe we could have a single `NS_GetDefaultReferrerPolicy(bool privateBrowsing)` function?

::: netwerk/test/unit/test_referrer.js:17
(Diff revision 1)
>      contentPolicyType: Ci.nsIContentPolicy.TYPE_OTHER
>    });
>  
>    chan.QueryInterface(Components.interfaces.nsIHttpChannel);
> +  if (isPrivate) {
> +    dump("getTestReferrer, isPrivate was true, calling setPrivate ...\n");

Did you intend to leave these debug statements in the test?

::: netwerk/test/unit/test_referrer.js:132
(Diff revision 1)
>    prefs.setBoolPref("network.http.referer.spoofSource", true);
>    prefs.setIntPref("network.http.referer.XOriginPolicy", 2);
>    do_check_eq(getTestReferrer(dest_uri, combo_referer_uri), "http://blah.foo.com:9999/spoofedpath");
>    do_check_null(getTestReferrer(dest_uri, "http://gah.foo.com/anotherpath"));
> +
> +  // tests for referer.XOriginTrimmingPolicy.pbmode

Wrong pref name
Attachment #8937909 - Flags: review?(francois)
Comment on attachment 8937909 [details]
Bug 587523 - strict-origin-when-cross-origin referer policy in pbmode

https://reviewboard.mozilla.org/r/208604/#review214510

> Did you intend to leave these debug statements in the test?

No, I'm just still trying to figure out why the test is failing. Any ideas? :)
Comment on attachment 8937909 [details]
Bug 587523 - strict-origin-when-cross-origin referer policy in pbmode

https://reviewboard.mozilla.org/r/208604/#review214376

::: netwerk/test/unit/test_referrer.js:17
(Diff revision 1)
>      contentPolicyType: Ci.nsIContentPolicy.TYPE_OTHER
>    });
>  
>    chan.QueryInterface(Components.interfaces.nsIHttpChannel);
> +  if (isPrivate) {
> +    dump("getTestReferrer, isPrivate was true, calling setPrivate ...\n");

Oops, I'll need to remove these dump lines.

::: netwerk/test/unit/test_referrer.js:136
(Diff revision 1)
> +
> +  // tests for referer.XOriginTrimmingPolicy.pbmode
> +  // send referer across origins
> +  prefs.setIntPref("network.http.referer.XOriginPolicy", 0);
> +  // defaultPolicy.pbmode pref (2) should trim to origin when cross-origin
> +  do_check_eq(getTestReferrer(server_uri, referer_uri, true), "http://foo.example.com/");

These tests fail and I can't understand why. :(
Comment on attachment 8937909 [details]
Bug 587523 - strict-origin-when-cross-origin referer policy in pbmode

https://reviewboard.mozilla.org/r/208604/#review214662

> No, I'm just still trying to figure out why the test is failing. Any ideas? :)

Nothing jumps out at me. I'd have to trace through this code. Thomas might be able to help though since he added the original pref.

One thing I just thought of is that this works differently from the other pbmode prefs we have (like TP) in that the global setting doesn't actually influence pbmode. With TP, you don't have to set both prefs, you can only set the global one and then the value of the pbmode pref is irrelevant.

So perhaps we could have:

- defaultPolicy: defines the default referrer policy in both normal and pbmode
- defaultPolicy.pbmode: defines the default referrer policy in pbmode only

Then in pbmode, we set the default to `min(defaultPolicy, defaultPolicy.pbmode)`. So you can set the global one and it will take precedence over the pbmode one as long as it's more restrictive.
Attachment #8937909 - Flags: review?(francois)
Assignee: valentin.gosu → lcrouch
:jduell - can you help me find a reviewer for this?
Flags: needinfo?(jduell.mcbugs)
Comment on attachment 8943030 [details]
Bug 587523: update test_referrer.js to include pbmode tests

https://reviewboard.mozilla.org/r/213292/#review219036


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: netwerk/protocol/http/HttpBaseChannel.cpp:1572
(Diff revision 1)
>  NS_IMETHODIMP
>  HttpBaseChannel::SetReferrer(nsIURI *referrer)
>  {
> +  if (mLoadInfo->GetOriginAttributes().mPrivateBrowsingId > 0) {
> +    return SetReferrerWithPolicy(referrer, NS_GetDefaultReferrerPolicy(true));
> +  } else {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Valentin--can you take this review?  ni me again if not.
Flags: needinfo?(jduell.mcbugs) → needinfo?(valentin.gosu)
Comment on attachment 8943029 [details]
Bug 587523 - rename referer.userControlPolicy to referer.defaultPolicy

Sure. I'll try to review it tomorrow morning.
Attachment #8943029 - Flags: review?(valentin.gosu)
Comment on attachment 8937909 [details]
Bug 587523 - strict-origin-when-cross-origin referer policy in pbmode

https://reviewboard.mozilla.org/r/208604/#review219644

::: modules/libpref/init/all.js:1682
(Diff revision 3)
>  pref("network.http.sendRefererHeader",      2);
> -// Set the default Referrer Policy to be used unless overriden by the site
> +// Set the default Referrer Policy; to be used unless overriden by the site
>  // 0=no-referrer, 1=same-origin, 2=strict-origin-when-cross-origin,
>  // 3=no-referrer-when-downgrade
>  pref("network.http.referer.userControlPolicy", 3);
> +// Set the Private Browsing Defaul Referrer Policy;

typo: Default

::: netwerk/base/nsNetUtil.h:940
(Diff revision 3)
>  
>  /**
>   * Return default referrer policy which is controlled by user
> - * pref network.http.referer.userControlPolicy
> + * prefs:
> + * network.http.referer.userControlPolicy for regular mode
> + * network.http.referer.userControlPrivatePolicy for private mode

This is now called `network.http.referer.defaultPolicy.pbmode` right? Please update the comment.

::: netwerk/protocol/http/HttpBaseChannel.cpp:1631
(Diff revision 3)
>    if (mReferrerPolicy == REFERRER_POLICY_UNSET) {
> +    if (mLoadInfo->GetOriginAttributes().mPrivateBrowsingId > 0) {
> +      mReferrerPolicy = NS_GetDefaultReferrerPolicy(true);
> +    } else {
> -    mReferrerPolicy = NS_GetDefaultReferrerPolicy();
> +      mReferrerPolicy = NS_GetDefaultReferrerPolicy();
> -  }
> +    }

The code is we avoid branches:
```
bool isPb = mLoadInfo->GetOriginAttributes().mPrivateBrowsingId > 0;
mReferrerPolicy = NS_GetDefaultReferrerPolicy(isPb);
```
Attachment #8937909 - Flags: review+
Comment on attachment 8943031 [details]
Bug 587523: update Fetch API referrer to check for private browsing pref

https://reviewboard.mozilla.org/r/213294/#review219652

::: dom/base/test/referrer_testserver.sjs:32
(Diff revision 2)
>           "ACTION=" + aAction + "&" +
>           "policy=" + aPolicy + "&" +
>           "NAME=" + aName + "&" +
>           "type=" + aType + "&" +
>           "SCHEME_FROM=" + schemeFrom;
> +  return url

nit: `;` at the end of the line.

::: dom/fetch/FetchDriver.cpp:569
(Diff revision 2)
>      NS_ENSURE_SUCCESS(rv, rv);
>  
>      // Set the same headers.
>      SetRequestHeaders(httpChan);
>  
> -    net::ReferrerPolicy net_referrerPolicy = mRequest->GetEnvironmentReferrerPolicy();
> +    // Step 5 of

nit: should be `Step 5` or `Step 5 of {number}`

::: dom/fetch/FetchDriver.cpp:587
(Diff revision 2)
>      // If request’s referrer policy is the empty string,
> -    // then set request’s referrer policy to "no-referrer-when-downgrade".
> +    // then set request’s referrer policy to the user-set default policy.
>      if (mRequest->ReferrerPolicy_() == ReferrerPolicy::_empty) {
> -      net::ReferrerPolicy referrerPolicy =
> -        static_cast<net::ReferrerPolicy>(NS_GetDefaultReferrerPolicy());
> +      nsCOMPtr<nsILoadInfo> loadInfo = httpChan->GetLoadInfo();
> +      net::ReferrerPolicy referrerPolicy;
> +      if (loadInfo->GetOriginAttributes().mPrivateBrowsingId > 0){

Let's avoid the if branch
Attachment #8943031 - Flags: review+
Comment on attachment 8943030 [details]
Bug 587523: update test_referrer.js to include pbmode tests

https://reviewboard.mozilla.org/r/213292/#review219650

::: netwerk/protocol/http/HttpBaseChannel.cpp:1572
(Diff revision 2)
>  NS_IMETHODIMP
>  HttpBaseChannel::SetReferrer(nsIURI *referrer)
>  {
> +  if (mLoadInfo->GetOriginAttributes().mPrivateBrowsingId > 0) {
> +    return SetReferrerWithPolicy(referrer, NS_GetDefaultReferrerPolicy(true));
> +  }

nit: Avoid the branch by using a temporary bool

::: netwerk/test/unit/test_referrer.js:65
(Diff revision 2)
>    Assert.equal(getTestReferrer(server_uri, referer_uri), "http://foo.example.com/");
>    prefs.setIntPref("network.http.referer.defaultPolicy", 3);
>    Assert.equal(getTestReferrer(server_uri, referer_uri), referer_uri);
>    Assert.equal(null, getTestReferrer(server_uri_2, referer_uri_https));
>  
> +  // tests for referer.defaultPolicy

nit: the comment should say tests for referer.defaultPolicy.pbmode
Attachment #8943030 - Flags: review+
Comment on attachment 8943029 [details]
Bug 587523 - rename referer.userControlPolicy to referer.defaultPolicy

https://reviewboard.mozilla.org/r/213290/#review219648

::: netwerk/base/nsNetUtil.cpp:90
(Diff revision 1)
>  
> -#define DEFAULT_USER_CONTROL_RP 3
> -#define DEFAULT_USER_CONTROL_PRIVATE_RP 2
> +#define DEFAULT_RP 3
> +#define DEFAULT_PRIVATE_RP 2
>  
> -static uint32_t sUserControlRp = DEFAULT_USER_CONTROL_RP;
> -static uint32_t sUserControlPrivateRp = DEFAULT_USER_CONTROL_PRIVATE_RP;
> +static uint32_t sDefaultRp = DEFAULT_RP;
> +static uint32_t defaultPrivateRp = DEFAULT_PRIVATE_RP;

nit: keep naming convention for statics -> sDefaultPrivateRp
Attachment #8943029 - Flags: review?(valentin.gosu) → review+
Flags: needinfo?(valentin.gosu)
Comment on attachment 8943031 [details]
Bug 587523: update Fetch API referrer to check for private browsing pref

https://reviewboard.mozilla.org/r/213294/#review219652

> nit: should be `Step 5` or `Step 5 of {number}`

It's meant to be "Step 5 of {url}" ... I moved the url up to the same line to make it more clear.
Comment on attachment 8943667 [details]
Bug 587523: remove branching logic; comment fixups

https://reviewboard.mozilla.org/r/214054/#review219760

Looks good. Thanks!
Attachment #8943667 - Flags: review?(valentin.gosu) → review+
Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/36474234a918
strict-origin-when-cross-origin referer policy in pbmode r=valentin
https://hg.mozilla.org/integration/autoland/rev/127b3aee4a44
rename referer.userControlPolicy to referer.defaultPolicy r=valentin
https://hg.mozilla.org/integration/autoland/rev/3259f30e0fd5
update test_referrer.js to include pbmode tests r=valentin
https://hg.mozilla.org/integration/autoland/rev/a0d2b0334f09
update Fetch API referrer to check for private browsing pref r=valentin
https://hg.mozilla.org/integration/autoland/rev/6cb9d8cf1f00
remove branching logic; comment fixups r=valentin
Flags: qe-verify+
I would expect that my current `network.http.referer.userControlPolicy` get's migrated to `network.http.referer.defaultPolicy`, shouldn't it?
Luke, can you make sure all the about:config prefs for referrers are honored correctly.  For example...

If a user sets network.http.sendRefererHeader to 0, there should be no referrer sent ever...
* no referrer in pbmode
* no referrer in regular mode (this should already have a test)
* no referrer in pbmode even when a page tries to set its own policy
* no referrer in regular mode even when a page tries to set its own policy.

If a user sets network.http.sendRefererHeader to 1 (which I think is origin only), then when visiting a page that tries to set a referrer policy of full referrer in pbmode, we should still only send the origin.

I'm not sure what the network.http.referer.userControlPolicy pref is.
Release Note Request (optional, but appreciated)
[Why is this notable]:
[Affects Firefox for Android]:
[Suggested wording]:
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
If you need more info for release notes let me know. I'm writing a whole blog post for the /security blog for this.
sjw - when a pref (like userControlPolicy) is not exposed via UI, we don't typically migrate its values.

:tanvi - I spot-checked all of those cases. I can easily add a new test line for #1. Testing #3 and #4 may be a bit more complex.
relnote-firefox: ? → ---
Release Note Request (optional, but appreciated)
[Why is this notable]:
[Affects Firefox for Android]:
[Suggested wording]:
[Links (documentation, blog post, etc)]:

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1433116 for the follow-up bug to add the tests. (Also restoring the relnote-firefox? I accidentally cleared.)
relnote-firefox: --- → ?
I just updated https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Referrer-Policy with update from this bug.
A review/correction of that doc is appreciated by a dev-doc member :)
(In reply to ©TriMoon™ from comment #41)
> I just updated
> https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Referrer-Policy
> with update from this bug.
> A review/correction of that doc is appreciated by a dev-doc member :)

Looks good; thanks!
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0

I have verified this issue on Mac Os X 10.12, Windows 10 x64 and Ubuntu 14.04 x64 with the latest Nightly (60.0a1-20180207220120) and Dev Edition (59.0b7-20180205211730) and I can confirm the fix. I will mark this as verified fixed.
Status: RESOLVED → VERIFIED
status-firefox59: fixed → verified
status-firefox60: --- → verified
Adding a relnote for 59 with the wording from the MDN page: 

"To help prevent third party data leakage while browsing privately, Firefox Private Browsing Mode will remove path information from referrers sent to third parties."

Is this true for fennec as well as desktop? Emil or maybe Sorina, would you mind checking on that?
Flags: needinfo?(sorina.florean)
Flags: needinfo?(emil.pasca)
Tested with Nexus 6P(Android 8.1.0) and Honor 8(Android 7.0) on 59.0b13 and I can confirm the behavior from comment 43 for Fennec.
Flags: needinfo?(sorina.florean)
Flags: needinfo?(emil.pasca)
Depends on: 1457981
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: