Closed Bug 1304623 Opened 3 years ago Closed 3 years ago

Create a pref to control the default referrer

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: francois, Assigned: tnguyen)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [domsecurity-backlog])

Attachments

(3 files, 11 obsolete files)

15.69 KB, patch
tnguyen
: review+
Details | Diff | Splinter Review
12.96 KB, patch
tnguyen
: review+
Details | Diff | Splinter Review
51.26 KB, patch
tnguyen
: review+
Details | Diff | Splinter Review
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.
Whiteboard: [domsecurity-backlog]
See Also: → 587523
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.
Assignee: nobody → tnguyen
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla52
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
MozReview-Commit-ID: 7KBEIrrfOZw
Flags: needinfo?(josh)
Flags: needinfo?(franziskuskiefer)
Flags: needinfo?(francois)
Depends on: 1264792
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.
Flags: needinfo?(franziskuskiefer)
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.
Attachment #8804619 - Attachment is obsolete: true
Target Milestone: mozilla52 → mozilla53
Flags: needinfo?(josh)
Flags: needinfo?(francois)
Depends on: 1318376
MozReview-Commit-ID: JK1IeQ99nqX
Attachment #8818198 - Attachment is obsolete: true
MozReview-Commit-ID: Exju6vN8dRG
MozReview-Commit-ID: 2hZb5l87XJu
MozReview-Commit-ID: 7nf9NmmvLwQ
MozReview-Commit-ID: 7nf9NmmvLwQ
Attachment #8820166 - Attachment is obsolete: true
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
Attachment #8820163 - Flags: review?(bkelly)
Attachment #8820164 - Flags: review?(mcmanus)
Attachment #8820179 - Flags: review?(bkelly)
Attachment #8820164 - Flags: review?(mcmanus) → review+
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?
Attachment #8820163 - Flags: review?(bkelly) → review-
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.
Attachment #8820179 - Flags: review?(bkelly) → review+
(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.
MozReview-Commit-ID: LgYN693WNbD
Attachment #8820179 - Attachment is obsolete: true
MozReview-Commit-ID: 18CX3J3jYho
Attachment #8820164 - Attachment is obsolete: true
MozReview-Commit-ID: GyFKuzXnVp5
Attachment #8820163 - Attachment is obsolete: true
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?
Attachment #8821084 - Flags: review?(mcmanus)
Attachment #8821083 - Flags: review+
Attachment #8821085 - Flags: review?(bkelly)
Attachment #8821084 - Flags: review?(bkelly)
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.
Attachment #8821084 - Flags: review?(bkelly) → review-
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.
Attachment #8821085 - Flags: review?(bkelly) → review+
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
Attachment #8821084 - Flags: review?(mcmanus)
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.
Flags: needinfo?(bkelly)
Hmm, likely I am missing something. I still do not cover all the cases where content process calls xpcom registration, nsHttpHandler.Init
MozReview-Commit-ID: I3ZYh9rrvGx
Attachment #8821084 - Attachment is obsolete: true
MozReview-Commit-ID: GZcwncNCKgl
Attachment #8821085 - Attachment is obsolete: true
Attachment #8822373 - Flags: review+
Flags: needinfo?(bkelly)
Updated patch based on comment 20
Attachment #8822372 - Flags: review?(mcmanus)
Attachment #8822372 - Flags: review?(bkelly)
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
Attachment #8822372 - Flags: review?(mcmanus) → review+
Attachment #8822372 - Flags: review?(bkelly) → review+
Thanks you all for your review.
MozReview-Commit-ID: 6R7kLB6jvhP
Attachment #8822373 - Attachment is obsolete: true
MozReview-Commit-ID: GEUDZ8UZAT5
Attachment #8822372 - Attachment is obsolete: true
MozReview-Commit-ID: 1A6IHPeNYBQ
Attachment #8821083 - Attachment is obsolete: true
Attachment #8823926 - Flags: review+
Attachment #8823928 - Flags: review+
Comment on attachment 8823929 [details] [diff] [review]
Create a pref to control the default referrer policy - part 3

Rebased carry + and running try
Attachment #8823929 - Flags: review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
This seems have to be backed out. I ve just found that it may leak sensitive data in the case unset
Leave open until bug 1329065 is fixed
Status: RESOLVED → REOPENED
Depends on: 1329065
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Thomas, thanks for getting this done!  Good job!
Thanks to all the reviewers as well. :)
Depends on: 1340827
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!
You need to log in before you can comment on or make changes to this bug.