1113438 - <meta referrer> origin-when-crossorigin sets incorrect referrer

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Alex Verstak]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/40.0.2214.38 Safari/537.36

Steps to reproduce:

1. Open https://scholar.google.com/scholar?q=news&mref=origin-when-crossorigin .
Click on "Creating reality: How TV news distorts events".
Open the console and type document.referrer to see the Referer header.

2. Repeat 1, but via plain HTTP: http://scholar.google.com/scholar?q=news&mref=origin-when-crossorigin .

3. Open https://scholar.google.com/scholar?q=news&mref=origin-when-crossorigin .
Click on "Cited by 739" under the second result (same origin link).
Open the console and type document.referrer.


Actual results:

1. document.referrer is empty - this is incorrect, should be origin.

2. document.referrer = "http://scholar.google.com" - this is correct, so it works over plain HTTP.

3. document.referrer = "https://scholar.google.com" - this is incorrect, it should be the full URL.  (Same issue over plain HTTP.)


Expected results:

1. document.referrer = "https://scholar.google.com".

2. document.referrer = "http://scholar.google.com".

3. document.referrer = "https://scholar.google.com/scholar?q=news&mref=origin-when-crossorigin".

It looks like the implementation of meta-referrer in Bug 704320 has two separate issues with origin-when-crossorigin:

A. It drops Referer entirely for cross-origin HTTPS -> HTTP links.
B. It limits Referer to origin for same-origin links, in both HTTP and HTTPS.

The appears to disagree with the spec:

http://w3c.github.io/webappsec/specs/referrer-policy/

"""
3.4. Origin When Cross-Origin

The Origin When Cross-Origin policy specifies that a full URL, stripped for use as a referrer, is sent as referrer information when making same-origin requests from a particular global environment, and only the ASCII serialization of the origin of the global environment from which a request is made is sent as referrer information when making cross-origin requests from a particular global environment.
"""

Comment 1

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Alex Verstak from comment #0)
> It looks like the implementation of meta-referrer in Bug 704320 has two
> separate issues with origin-when-crossorigin:
> 
> A. It drops Referer entirely for cross-origin HTTPS -> HTTP links.
> B. It limits Referer to origin for same-origin links, in both HTTP and HTTPS.

Hm, it looks like somehow we missed covering origin-when-crossorigin in the tests:
http://mxr.mozilla.org/mozilla-central/source/dom/base/test/test_bug704320.html?force=1

I can understand why A may make sense.  It deviates from the spec, but the user agent may not want to leak any referrer data from https to http.  But if it's origin, it probably doesn't leak much and we should likely fix A.

B needs to be fixed.  Let me dig into this.

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Alex Verstak from comment #0)
> 1. document.referrer is empty - this is incorrect, should be origin.

I can reproduce the first case.  This is https to http, here's why the referrer gets dropped:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#1041

The key part of the spec is this:
"Note: The Origin When Cross-Origin policy causes the origin of HTTPS referrers to be sent over the network as part of unencrypted HTTP requests."

> 2. document.referrer = "http://scholar.google.com" - this is correct, so it
> works over plain HTTP.

Works as intended.

> 3. document.referrer = "https://scholar.google.com" - this is incorrect, it
> should be the full URL.  (Same issue over plain HTTP.)

I cannot reproduce this, I get a full referrer here on Nightly (seems to work as intended).

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

Attachment: fix

This seems to fix the https->http xorigin referrer issue.  Needs tests... if someone else wants to take a stab at testing this, go for it.  Otherwise, I will try to get to it in a week (taking some time off for the holidays).

Comment 4

[Alex Verstak]

Attachment: Tests that don't pass

Added tests, but some cases don't pass:
1. window.open with http->http and https->https - loadingURI is null, so it treats this as cross-origin and sends an origin referrer - http://example.com or https://example.com;
2. link with http->http and https->https - loadingURI is the top frame, http://mochi.test:8888/..., so it thinks this is cross-origin and sends an origin referrer - http://example.com or https://example.com.

Perhaps it should use the referrer, not the loading principal, for the origin-when-crossorigin check?  The referring page here is the iframe, not the top-level frame, and the Referer header is based on the iframe.  I'm not sure - it's best to ask someone competent.

(Without frames, this fix works just fine.)

Comment 5

[Alex Verstak]

Attachment: Fix and tests, using triggering principal for cross-origin check

The iframes work as expected (by me :) using the triggering principal for the cross-origin check, as suggested in bug 1091883.

Sid: back to you.

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

Thanks, Alex!  Just quickly, your patch looks right.  We landed the initial meta referrer patch before triggeringPrincipal was available, and you're right in switching this implementation to use it!  

I'll look closely and see if we can't get this landed early next week.  Do you mind if I assign the bug to you (for credit) since you did all the hard work?

Comment 7

[Alex Verstak]

Great, thanks for the quick responses, Sid!

Re bug ownership - either way is fine by me, though it'll probably be up to you to do a try run and to get it landed.  Your call.

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8541586 [details] [diff] [review]
Fix and tests, using triggering principal for cross-origin check

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

This looks great from my perspective.  The tests are what I expected and the use of triggering principal seems right.  I'm not a necko peer, so I'm gonna flag mcmanus for a review (he reviewed the other meta referrer stuff in necko) and flag Tanvi just to make sure the use of triggering principal makes sense here.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1083,3 @@
>      isCrossOrigin = NS_FAILED(rv);
>    } else {
> +    NS_WARNING("no triggering principal available via loadInfo, assumming load is cross-origin");

I know, I spelled this wrong originally, but assuming should only have one "m".  Please fix while you're modifying this.

Comment 9

[Tanvi Vyas[:tanvi]]

Comment on attachment 8541586 [details] [diff] [review]
Fix and tests, using triggering principal for cross-origin check

TriggeringPrincipal looks right here.  (And I've actually been looking at some behavior in nsDocShell that supports this change - https://etherpad.mozilla.org/docShellPrincipals line 38.)

How does this relate to bug 1091883?  I still think we should test what happens in the scenario described in that bug.

Comment 10

[Alex Verstak]

Yep, this also fixes the case in bug 1091883.  Added a test to that bug.

Comment 11

[Patrick McManus [:mcmanus]]

Comment on attachment 8541586 [details] [diff] [review]
Fix and tests, using triggering principal for cross-origin check

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

thanks!

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1075,5 @@
> +  if (triggeringURI) {
> +    if (LOG_ENABLED()) {
> +      nsAutoCString triggeringURISpec;
> +      rv = triggeringURI->GetAsciiSpec(triggeringURISpec);
> +      if (!NS_FAILED(rv)) LOG(("triggeringURI=%s\n", triggeringURISpec.get()));

nit:
if(!foo) {
 log();
}

Comment 12

[Alex Verstak]

Attachment: Fix and tests, using triggering principal for cross-origin check

Addressed both review comments. Ran mach test {dom,netwerk} and updated a similar test under CSP.

Sid: may I ask you to do a try bot run with this?  I don't have access.

Is there anything else I need to do to get this checked in?

Comment 13

[Sid Stamm [:geekboy or :sstamm]]

I don't think a try run will uncover problems with this patch (it's pretty straightforward), but since I can't stick around and help fix anything in case it does cause a regression, gonna push to try anyway:

(if the try run looks good, feel free to set the checkin-needed keyword to get someone's attention for landing).

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e7d2d94dc666

Great work, Alex!  This is really helpful!

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed280f6c7b39

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ed280f6c7b39