1304623 - Create a pref to control the default referrer

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[François Marier [:francois]]

The default referrer is defined to be no-referrer-when-downgrade. It would be good to give users the ability to change this via an about:config pref. Currently users can _override_ various aspects of the referrer, but they can't change the default in a way that still respects the wishes of sites that provide a different policy.

I'm thinking of network.http.referer.defaultPolicy which would take these values:

0 - no-referrer
1 - same-origin
2 - strict-origin-when-cross-origin
3 - no-referrer-when-downgrade (default)

That pref would default to 3 and we should write tests to ensure that nothing actually changes as a result of adding this pref.

The default for the header is set here: https://dxr.mozilla.org/mozilla-central/rev/560b2c805bf7bebeb3ceebc495a81b2aa4c0c755/netwerk/protocol/http/HttpBaseChannel.cpp#1284

and we probably also need to change the meta referrer code too.

Once we have that pref, we can experiment with it in Shield.

Comment 1

[Thomas Nguyen (:tnguyen)]

Reference
https://www.w3.org/TR/referrer-policy/#user-controls

Nothing in this specification should be interpreted as preventing user agents from offering options to users which would change the information sent out via a `Referer` header. For instance, user agents MAY allow users to suppress the referrer header entirely, regardless of the active referrer policy on a page.

Comment 2

[Thomas Nguyen (:tnguyen)]

As discussed, we are handling the case that a site does not specify referrer policy and causes a fallback to a referrer policy defined as default preference.
After checking our code, the required change is going too far as I thought. We are using 3 kinds of aliases to define this default value (and the value of them is 0u):

- nsIHttpChannel.REFERRER_POLICY_NO_REFERRER_WHEN_DOWNGRADE (as search in [1])
- nsIHttpChannel.REFERRER_POLICY_DEFAULT (search result in [2])
- mozilla::net::RP_Default (search in [3])

I am looking back the specs and we now have "Empty string state" [4]:
The empty string corresponds to no referrer policy, causing a fallback to a referrer policy defined elsewhere, or in the case where no such higher-level policy is available, defaulting to "no-referrer-when-downgrade". We defined a REFERRER_POLICY_UNSET which corresponds this state.

The idea is that I am going to move all the 3 aliases above to set the default to REFERRER_POLICY_UNSET, then the policy will be replaced as default value in channel before sending out [5].

First approach (patch attached), does anybody have any thoughts about that?
[1] https://dxr.mozilla.org/mozilla-central/search?q=no_referrer_when_downgrade&redirect=false
[2] https://dxr.mozilla.org/mozilla-central/search?q=REFERRER_POLICY_DEFAULT&redirect=false
[3] https://dxr.mozilla.org/mozilla-central/search?q=RP_Default
[4] https://w3c.github.io/webappsec-referrer-policy/#referrer-policy-empty-string
[5] https://dxr.mozilla.org/mozilla-central/rev/560b2c805bf7bebeb3ceebc495a81b2aa4c0c755/netwerk/protocol/http/HttpBaseChannel.cpp#1273

Comment 3

[Thomas Nguyen (:tnguyen)]

Attachment: Create a pref to control the default referrer

MozReview-Commit-ID: 7KBEIrrfOZw

Comment 4

[Franziskus Kiefer [:franziskus]]

I didn't check all the textual replacements in your patch. But the idea and actual pref handling code look sensible to me. Though, we should probably put the pref under network.http.referer as Francois suggested.

Comment 5

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8804619 [details] [diff] [review]
Create a pref to control the default referrer

Thanks Franziskus.
I just tried to run some tests and this bug is blocked by bug 1264792.
I would like to set the milestone of this bug to mozilla53 because it requires longer time than I thought.

Comment 6

[Thomas Nguyen (:tnguyen)]

Attachment: Create a pref to control the default referrer

MozReview-Commit-ID: JK1IeQ99nqX

Comment 7

[Thomas Nguyen (:tnguyen)]

Attachment: Create a pref to control the default referrer policy - part 1

MozReview-Commit-ID: Exju6vN8dRG

Comment 8

[Thomas Nguyen (:tnguyen)]

Attachment: Create a pref to control the default referrer - part 2

MozReview-Commit-ID: 2hZb5l87XJu

Comment 9

[Thomas Nguyen (:tnguyen)]

Attachment: Create a pref to control the default referrer - part 3

MozReview-Commit-ID: 7nf9NmmvLwQ

Comment 10

[Thomas Nguyen (:tnguyen)]

Attachment: Create a pref to control the default referrer - part 3

MozReview-Commit-ID: 7nf9NmmvLwQ

Comment 11

[Thomas Nguyen (:tnguyen)]

dxr search
https://dxr.mozilla.org/mozilla-central/search?q=regexp%3Ano_referrer_when|rp_default|er_policy_default

I am changing all 3 defaults in several places
- nsIHttpChannel.REFERRER_POLICY_NO_REFERRER_WHEN_DOWNGRADE
- nsIHttpChannel.REFERRER_POLICY_DEFAULT
- mozilla::net::RP_Default

to nsIHttpChannel.REFERRER_POLICY_UNSET ( and mozilla::net::RP_Unset)
- Changing in necko: before sending out a request, the referrer policy then will be checked if it equals Unset and will cause a fallback to the user control default policy defined by Preference.
- Change in Fetch: If request’s referrer policy is the empty string, then set request’s referrer policy to "no-referrer-when-downgrade". Should change to the user control value here.
- Others: mostly change the above 3 defaults to Unset value
Hi Patrick, bkelly, may I get your review in these patches?
Thanks

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8820163 [details] [diff] [review]
Create a pref to control the default referrer policy - part 1

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

Looks reasonable, but I think we should have a single source of truth on the pref.  Please expose the default policy from necko using a nsNetUtil function.

::: dom/fetch/FetchDriver.cpp
@@ +107,5 @@
>  // Necko HTTP implementation.
>  nsresult
>  FetchDriver::HttpFetch()
>  {
> +  workers::AssertIsOnMainThread();

nit: Can you just use MOZ_ASSERT(NS_IsMainThread()) here?  We typically don't use the worker AssertIsOnMainThread() outside of worker-specific code.

@@ +286,5 @@
> +      // 2 - strict-origin-when-cross-origin
> +      // 3 - no-referrer-when-downgrade (default)
> +
> +      uint8_t rpUserControl = Preferences::GetInt("network.http.referer.userControlPolicy", 3);
> +      net::ReferrerPolicy referrerPolicy = net::RP_No_Referrer_When_Downgrade;

Please create a nsNetUtil function that returns the default value here.  It can then internally use gHttpHandler and reuse the same code the channel code uses.

::: dom/workers/ScriptLoader.cpp
@@ +205,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    if (nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(channel)) {
>      mozilla::net::ReferrerPolicy referrerPolicy = parentDoc ?
> +      parentDoc->GetReferrerPolicy() : mozilla::net::RP_Unset;

Are you removing the RP_Default value completely?  Seems like we are effectively saying it should not be used, right?

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8820179 [details] [diff] [review]
Create a pref to control the default referrer - part 3

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

r=me with comments addressed

::: dom/base/nsContentUtils.cpp
@@ +8521,5 @@
>    net::ReferrerPolicy referrerPolicy = mozilla::net::RP_Unset;
>    while (tokenizer.hasMoreTokens()) {
>      token = tokenizer.nextToken();
> +    // If token is a referrer policy and token is not the empty string,
> +    // then set policy to token

I think it would be clearer to just do:

if (token.IsEmpty()) {
  continue;
}

And then remove the comment and the extra conditional below.

::: image/imgLoader.cpp
@@ +2009,5 @@
>      nsresult rv = LoadImage(aURI,
>                              aInitialDocumentURI,
>                              aReferrerURI,
>                              refpol == mozilla::net::RP_Unset ?
> +                              mozilla::net::RP_Unset : refpol,

I think you can replace the ternary with just refpol.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Thomas Nguyen[:tnguyen] (use ni? plz) from comment #11)
> I am changing all 3 defaults in several places
> - nsIHttpChannel.REFERRER_POLICY_NO_REFERRER_WHEN_DOWNGRADE
> - nsIHttpChannel.REFERRER_POLICY_DEFAULT
> - mozilla::net::RP_Default

I think we should probably also remove the DEFAULT constants as well to avoid people accidentally using them in new places.

Comment 15

[Thomas Nguyen (:tnguyen)]

Attachment: Create a pref to control the default referrer policy - part 3

MozReview-Commit-ID: LgYN693WNbD

Comment 16

[Thomas Nguyen (:tnguyen)]

Attachment: Create a pref to control the default referrer policy- part 2

MozReview-Commit-ID: 18CX3J3jYho

Comment 17

[Thomas Nguyen (:tnguyen)]

Attachment: Create a pref to control the default referrer policy - part 1

MozReview-Commit-ID: GyFKuzXnVp5

Comment 18

[Thomas Nguyen (:tnguyen)]

You are right, should remove the unnecessary constants to avoid people from mistakenly using them later.
Thanks for your review.
Updated the patch based on the comment 10 (adding netUtil function and removing default constants)
Could you please take a look again?

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8821084 [details] [diff] [review]
Create a pref to control the default referrer policy- part 2

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

::: netwerk/base/nsNetUtil.cpp
@@ +2378,5 @@
> +NS_GetDefaultReferrerPolicy(uint32_t& aDefaultPolicy)
> +{
> +  aDefaultPolicy = RP_No_Referrer_When_Downgrade;
> +
> +  if (!gHttpHandler) {

Oh, hmm.  I didn't consider that you might not have a gHttpHandler, but I guess thats the case in the content process.

Rather than spin up the full nsHttpHandler in the child process its probably better just to create a static variable here that contains the pref value.  gNetDefaultReferrer or something.  You can then use the preference APIs to keep it updated.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8821085 [details] [diff] [review]
Create a pref to control the default referrer policy - part 1

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

r=me with comments addressed

::: dom/fetch/FetchDriver.cpp
@@ +279,5 @@
>      // If request’s referrer policy is the empty string,
>      // then set request’s referrer policy to "no-referrer-when-downgrade".
>      if (mRequest->ReferrerPolicy_() == ReferrerPolicy::_empty) {
> +      // Bug 1304623 : default referrer policy could be controlled by user
> +      // pref network.http.referer.userControlPolicy

I don't think we need this comment here any more.  Its clear we are pulling the default referrer from somewhere else.

@@ +281,5 @@
>      if (mRequest->ReferrerPolicy_() == ReferrerPolicy::_empty) {
> +      // Bug 1304623 : default referrer policy could be controlled by user
> +      // pref network.http.referer.userControlPolicy
> +      uint32_t referrerPolicy;
> +      Unused << NS_GetDefaultReferrerPolicy(referrerPolicy);

It would be nice if this method just returned the referrerPolicy.  Return our old default if it would have failed.

Comment 21

[Patrick McManus [:mcmanus]]

Comment on attachment 8821084 [details] [diff] [review]
Create a pref to control the default referrer policy- part 2

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

::: netwerk/base/nsNetUtil.cpp
@@ +2378,5 @@
> +NS_GetDefaultReferrerPolicy(uint32_t& aDefaultPolicy)
> +{
> +  aDefaultPolicy = RP_No_Referrer_When_Downgrade;
> +
> +  if (!gHttpHandler) {

+1 to bkelly.. you can use that same logic in parent and child

Comment 22

[Thomas Nguyen (:tnguyen)]

I took a look at this a gain. If we have a httpchannel, I think HttpBaseChannel keeps reference of nsHttpHandler in both parent and child case
https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#104
(and nsHttpConnection.cpp also do) 
Basically we will not care about the referrer/referrer policy with out having a http channel/connection, so probably, may I just assert sth like?
    MOZ_ASSERT(gHttpHandler);
    if (!gHttpHandler) return default;

Thanks Patrick, Ben for your help. Happy holidays to you.

Comment 23

[Thomas Nguyen (:tnguyen)]

Hmm, likely I am missing something. I still do not cover all the cases where content process calls xpcom registration, nsHttpHandler.Init

Comment 24

[Thomas Nguyen (:tnguyen)]

Attachment: Create a pref to control the default referrer policy- part 2

MozReview-Commit-ID: I3ZYh9rrvGx

Comment 25

[Thomas Nguyen (:tnguyen)]

Attachment: Create a pref to control the default referrer policy - part 1

MozReview-Commit-ID: GZcwncNCKgl

Comment 26

[Thomas Nguyen (:tnguyen)]

Updated patch based on comment 20

Comment 27

[Patrick McManus [:mcmanus]]

Comment on attachment 8822372 [details] [diff] [review]
Create a pref to control the default referrer policy- part 2

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

lgtm - thanks

Comment 28

[Thomas Nguyen (:tnguyen)]

Thanks you all for your review.

Comment 29

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=11fb2105bf03

Comment 30

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08445c744e75

Comment 31

[Thomas Nguyen (:tnguyen)]

Attachment: Create a pref to control the default referrer policy - part 1

MozReview-Commit-ID: 6R7kLB6jvhP

Comment 32

[Thomas Nguyen (:tnguyen)]

Attachment: Create a pref to control the default referrer policy- part 2

MozReview-Commit-ID: GEUDZ8UZAT5

Comment 33

[Thomas Nguyen (:tnguyen)]

Attachment: Create a pref to control the default referrer policy - part 3

MozReview-Commit-ID: 1A6IHPeNYBQ

Comment 34

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8823929 [details] [diff] [review]
Create a pref to control the default referrer policy - part 3

Rebased carry + and running try

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a042126872f4
Create a pref to control the default referrer policy - part 1. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/73d0bfecc06b
Create a pref to control the default referrer policy- part 2. r=mcmanus, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f60ec0d460a
Create a pref to control the default referrer policy - part 3. r=bkelly

Comment 36

[Thomas Nguyen (:tnguyen)]

This seems have to be backed out. I ve just found that it may leak sensitive data in the case unset

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a042126872f4
https://hg.mozilla.org/mozilla-central/rev/73d0bfecc06b
https://hg.mozilla.org/mozilla-central/rev/6f60ec0d460a

Comment 38

[Thomas Nguyen (:tnguyen)]

Leave open until bug 1329065 is fixed

Comment 39

[Ethan Tseng [:ethan]]

Thomas, thanks for getting this done!  Good job!
Thanks to all the reviewers as well. :)

Comment 40

[Chris Mills [:cmills]]

I've added a note to the Referrer-Policy page about this:

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Referrer-Policy#Browser_compatibility

And a note to the Fx53 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/53#HTTP

Let me know if there are any more changes you think I should make. cheers!