Implement CSP 1.1 referrer directive

RESOLVED FIXED in mozilla37

Status

()

defect
P1
normal
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: bruant.d, Assigned: geekboy)

Tracking

(Blocks 2 bugs, {dev-doc-complete})

unspecified
mozilla37
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [CSP 1.1], )

Attachments

(2 attachments, 5 obsolete attachments)

Reporter

Description

6 years ago
No description provided.
Assignee

Updated

6 years ago
See Also: → 704320
Assignee

Updated

5 years ago
Depends on: 966505
Looks like most of what we'll need in the backend is part of bug 704320.  The scope of work for this bug is to add on top of that: simply setting the referrer policy based on the CSP header when the CSP header gets parsed.
Depends on: 704320
No longer depends on: 966505
Assignee

Updated

5 years ago
Assignee: nobody → sstamm
Posted patch first whack (obsolete) — Splinter Review
Here's a first whack at the CSP bits for a referrer policy.

Garrett: does this look sane (before I start writing tests for it)?
Attachment #8372673 - Flags: feedback?(grobinson)
FYI, if you want to try it out, you'll have to apply the patch on bug 704320 first, since this sets mReferrerPolicy on nsDocument that doesn't exist without it.
Comment on attachment 8372673 [details] [diff] [review]
first whack

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

Other than comments, lgtm. Test away!

::: content/base/public/nsIContentSecurityPolicy.idl
@@ +197,5 @@
> +  const unsigned short REFERRER_POLICY_DEFAULT   = 2;
> +  const unsigned short REFERRER_POLICY_ORIGIN    = 3;
> +  const unsigned short REFERRER_POLICY_ALWAYS    = 4;
> +
> +   

trailing whitespace

::: content/base/src/contentSecurityPolicy.js
@@ +182,5 @@
> +   * otherwise NEVER.
> +   */
> +  get referrerPolicy() {
> +    let CSP = Ci.nsIContentSecurityPolicy;
> +    return this._policies.reduce(

This matches the conflict resolution defined in the spec (3.2.5.13.1). So we got that going for us, which is nice.

@@ +189,5 @@
> +          if (prev._referrerPolicy == CSP.REFERRER_POLICY_UNSET)
> +            return cur._referrerPolicy;
> +
> +          // if current CSP is unset, we can use the previous one.
> +          if (cur == CSP.REFERRER_POLICY_UNSET)

Did you mean cur._referrerPolicy?

@@ +191,5 @@
> +
> +          // if current CSP is unset, we can use the previous one.
> +          if (cur == CSP.REFERRER_POLICY_UNSET)
> +            return prev._referrerPolicy;
> +          

trailing whitespace

@@ +196,5 @@
> +          // if the two are the same, use it.
> +          if (cur._referrerPolicy == prev._referrerPolicy)
> +            return cur._referrerPolicy;
> +
> +          // if we get here, the policies disagree and neither is unset.

s/unset/set?
Attachment #8372673 - Flags: feedback?(grobinson) → feedback+
Assignee

Updated

5 years ago
Component: Security → DOM: Security
Assignee

Updated

5 years ago
Priority: -- → P1
Whiteboard: [CSP 1.1]
Comment on attachment 8372673 [details] [diff] [review]
first whack

This will no longer work due to the CSP rewrite.  Also, there's a speculative parser we have to seed with any referrer policy set on the document before the parser is triggered.

And tests.  This needs tests.
Attachment #8372673 - Attachment is obsolete: true
Posted patch bug965727-referrer-tests (obsolete) — Splinter Review
Tests for the CSP referrer directive implementation
Attachment #8527991 - Flags: review?(mozilla)
Posted patch bug965727-referrer (obsolete) — Splinter Review
Chris: I'd like you to do a pass on this implementation before I flag someone for the changes in dom (I'll need someone from necko and henri or bz to look at the changes to the way the html5 parser is set up to use CSP's referrer policy).
Attachment #8527994 - Flags: review?(mozilla)
Comment on attachment 8527994 [details] [diff] [review]
bug965727-referrer

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

The CSP parts look good. r=me on those bits. Please try to avoid c-style casts wherever possible and let a dom-peer look over the other changes in this patch.

::: dom/base/nsDocument.cpp
@@ +2990,5 @@
> +  uint32_t referrerPolicy = mozilla::net::RP_Default;
> +  rv = csp->GetReferrerPolicy(&referrerPolicy, &hasReferrerPolicy);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  if (hasReferrerPolicy) {
> +    // Referrer policy spec (section 6.1) says that once the referrer policy

Section 6.1 where? Probably worth adding the link to the section in the spec.

@@ +2993,5 @@
> +  if (hasReferrerPolicy) {
> +    // Referrer policy spec (section 6.1) says that once the referrer policy
> +    // is set, any future attempts to change it result in No-Referrer.
> +    if (!mReferrerPolicySet) {
> +      mReferrerPolicy = (ReferrerPolicy) referrerPolicy;

Even though a C-Style cast probably works correctly here, please rather use a static_cast here.

@@ +2996,5 @@
> +    if (!mReferrerPolicySet) {
> +      mReferrerPolicy = (ReferrerPolicy) referrerPolicy;
> +      mReferrerPolicySet = true;
> +    } else if (mReferrerPolicy != referrerPolicy) {
> +      mReferrerPolicy = mozilla::net::RP_No_Referrer;

Is it worth adding any kind of log here!

@@ +3000,5 @@
> +      mReferrerPolicy = mozilla::net::RP_No_Referrer;
> +    }
> +
> +    // Any Referrer Policy is set for the speculative parser in
> +    // nsHTMLDocument::StartDocumentLoad()

This comment is confusing, does that mean some code is missing here? Please clarify.

::: dom/html/nsHTMLDocument.cpp
@@ +679,5 @@
>      executor = static_cast<nsHtml5TreeOpExecutor*> (mParser->GetContentSink());
> +    if (mReferrerPolicySet) {
> +      // CSP may have set the referrer policy, so a speculative parser should
> +      // start with the new referrer policy.
> +      executor->SetSpeculationReferrerPolicy((mozilla::net::ReferrerPolicy) mReferrerPolicy);

Please try to avoid c-style casts.

@@ +1658,5 @@
> +    // start with the new referrer policy.
> +    nsHtml5TreeOpExecutor* executor = nullptr;
> +    executor = static_cast<nsHtml5TreeOpExecutor*> (mParser->GetContentSink());
> +    if (executor && mReferrerPolicySet) {
> +      executor->SetSpeculationReferrerPolicy((mozilla::net::ReferrerPolicy) mReferrerPolicy);

same here, please no c-style cast.

::: dom/security/nsCSPContext.cpp
@@ +258,5 @@
>  
>  NS_IMETHODIMP
> +nsCSPContext::GetReferrerPolicy(uint32_t* outPolicy, bool* outIsSet)
> +{
> +  *outIsSet = false;

Nit: unlikely but still possible that outPolicy not gets assigned - do you want to also assign a defaultvalue to outPolicy up here?

::: dom/security/nsCSPUtils.h
@@ +355,5 @@
>  
>      inline bool getReportOnlyFlag() const
>        { return mReportOnly; }
>  
> +    inline void setReferrerPolicy(nsAString& aValue)

Nit: To indicate that no one should ever change aValue you could add rewrite to:
setReferrerPolicy(const nsAString* aValue)
Attachment #8527994 - Flags: review?(mozilla) → review+
Comment on attachment 8527991 [details] [diff] [review]
bug965727-referrer-tests

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

Really nice tests - just some nits. r=me.

::: dom/base/test/csp/file_csp_referrerdirective.html
@@ +9,5 @@
> +var id = "unknown";
> +for (var i=0; i < args.length; i++) {
> +  var arg = unescape(args[i]);
> +  if (arg.indexOf('=') > 0 &&
> +      arg.indexOf('id') == 0) {

Nit: no need for a linebreak here.

@@ +21,5 @@
> +  'results': {
> +    'sameorigin': false,
> +    'crossorigin': false,
> +    'downgrade': false
> +  }};

Nit: I think the second } and the semicolon should go in the next line.

::: dom/base/test/csp/mochitest.ini
@@ +101,5 @@
>    file_multi_policy_injection_bypass_2.html
>    file_multi_policy_injection_bypass_2.html^headers^
>    file_form-action.html
> +  file_csp_referrerdirective.html
> +  referrerdirective.sjs

since every supporting file is prefixed with file_, probably it's worth changing the name to file_referrerdirective.sjs for consistency.

::: dom/base/test/csp/test_CSP_referrerdirective.html
@@ +5,5 @@
> +<!DOCTYPE HTML>
> +<html>
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=965727
> +-->

Nit: Usually we add/set the bugnumber in the <title>. If you decide to add the bugnumber to the title you could remove that comment.

@@ +30,5 @@
> +   the scripts are blocked, the test fails (they should run).  When loaded,
> +   each script updates a results object in the test page, and then when the
> +   test page has finished loading all the scripts, it postMessages back to this
> +   page.  Once all tests are done, the results are checked.
> + */

Nice description, really like that. Probably worth adding a leading '*' to every line which makes it easier to pinpoint the comment start and end.

@@ +33,5 @@
> +   page.  Once all tests are done, the results are checked.
> + */
> +
> +var testData = {
> +  'default': { 'csp': "script-src * 'unsafe-inline'; referrer default",

You could factor out
> "script-src * 'unsafe-inline';
and only keep the bits that vary in the test-setup here and add the rest where you assign 'elt.src'.

@@ +60,5 @@
> +                          'downgrade': 'none' }},
> +  };
> +
> +
> +var bug965727 = {

can we rename that variable? 'bug965727' is not very descriptive.

@@ +98,5 @@
> +    SimpleTest.finish();
> +  }
> +};
> +
> +SimpleTest.waitForExplicitFinish();

Nit: Move that up to the top to make it more explicit that we have to wait for an explicit finish().

@@ +112,5 @@
> +      // one iframe created for each test case
> +      for (var id in testData) {
> +        var elt = document.createElement("iframe");
> +        elt.src = "https://example.com/tests/dom/base/test/csp/" +
> +                  "file_csp_testserver.sjs?" +

No need for a line break before "file_csp_testserver.sjs".
Attachment #8527991 - Flags: review?(mozilla) → review+
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #9)
> Really nice tests - just some nits. r=me.

Thanks for the speedy review!  I disagree with some of your nits.

> > +  referrerdirective.sjs
> 
> since every supporting file is prefixed with file_, probably it's worth
> changing the name to file_referrerdirective.sjs for consistency.

I'd like to move away from using file_ in front of everything since it makes paths really long.  Can we start to reserve file_ for html files and make all the rest start with just whatever?  Kind of like using CSP in the file when it's already in dom/base/test/csp...

> You could factor out
> > "script-src * 'unsafe-inline';
> and only keep the bits that vary in the test-setup here and add the rest
> where you assign 'elt.src'.

This adds unnecessary complexity to the code and makes the policies harder to read at a quick glance.  I'd like to keep the CSP policies intact for easy reading.

> Nit: Move that up to the top to make it more explicit that we have to wait
> for an explicit finish().

I wanted to put the SimpleTest.waitForExplicitFinish() as close to the entry point as possible so when we're debugging a test failure, it's clear that we didn't forget it.  All the stuff above it is just function definitions (nothing executes before it).
Fixed some nits.  Carrying over r=ckerschb.
Attachment #8527991 - Attachment is obsolete: true
Attachment #8531003 - Flags: review+
Posted patch bug965727-referrer (obsolete) — Splinter Review
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #8)
> Section 6.1 where? Probably worth adding the link to the section in the spec.

Eugh... it's still a draft and I hate to link from the code to a draft that will move.  Should I drop the section reference or just give it a name?

Thanks for the review comments!  Carrying over r=ckerschb.
Attachment #8527994 - Attachment is obsolete: true
Attachment #8531022 - Flags: review+
Comment on attachment 8531022 [details] [diff] [review]
bug965727-referrer


jst: I know you're super busy, but since you reviewed some of the meta referrer stuff, can you look at the dom/ and parser/ stuff here too?  There are not many changes.  I had to add a method to the nsHtml5TreeOpExecutor to set the speculation referrer policy not based on a string, but on a ReferrerPolicy (simplistic overload), and I had to modify nsHTMLDocument to tell the speculation parser what if any referrer policy was set on the document before parsing time (needed for CSP-set policies, but not <meta>-set policies).
Attachment #8531022 - Flags: review?(jst)
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #12)
> Created attachment 8531022 [details] [diff] [review]
> bug965727-referrer
> 
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #8)
> > Section 6.1 where? Probably worth adding the link to the section in the spec.
> 
> Eugh... it's still a draft and I hate to link from the code to a draft that
> will move.  Should I drop the section reference or just give it a name?

What you have right now works for me. Was just another nit anyway :-)
Comment on attachment 8531022 [details] [diff] [review]
bug965727-referrer

Looks great, one thing caught my eye when reading through this:

- In nsCSPContext::GetReferrerPolicy():

+  for (uint32_t i = 0; i < mPolicies.Length(); i++) {
+    nsAutoString refpol;

Maybe hoist that nsAutoString outside of the loop to avoid calling its ctor/dtor every iteration.
Attachment #8531022 - Flags: review?(jst) → review+
updated for bitrot.
Attachment #8531003 - Attachment is obsolete: true
Attachment #8537870 - Flags: review+
Thanks jst!  Updated with your suggestion (and for bitrot).  Carrying over r=jst and r=ckerschb.
Attachment #8531022 - Attachment is obsolete: true
Attachment #8537871 - Flags: review+
Had to disable the tests on b2g:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8277e5192972

Was getting this in the logcat (and then a timeout because the https content doesn't load):

12-17 20:15:13.424 E/Mochitest(  796):     at initPage (about:neterror?e=proxyConnectFailure&u=https%3A//example.com/tests/dom/base/test/csp/file_csp_testserver.sjs%3Fid%3Ddefault%26csp%3Dscript-src%2520*%2520%2527unsafe-inline%2527%253B%2520referrer%2520default%26file%3Dtests/dom/base/test/csp/file_csp_referrerdirective.html&c=UTF-8&f=regular&m=http%3A//mochi.test%3A8888/manifest.webapp&d=Firefox%20is%20configured%20to%20use%20a%20proxy%20server%20that%20is%20refusing%20connections.:1231:6965)
12-17 20:15:13.424 E/Mochitest(  796):     at ee_emit (about:neterror?e=proxyConnectFailure&u=https%3A//example.com/tests/dom/base/test/csp/file_csp_testserver.sjs%3Fid%3Ddefault%26csp%3Dscript-src%2520*%2520%2527unsafe-inline%2527%253B%2520referrer%2520default%26file%3Dtests/dom/base/test/csp/file_csp_referrerdirective.html&c=UTF-8&f=regular&m=http%3A//mochi.test%3A8888/manifest.webapp&d=Firefox%20is%20configured%20to%20use%20a%20proxy%20server%20that%20is%20refusing%20connections.:991:88)
12-17 20:15:13.424 E/Mochitest(  796):     at setReady (about:neterror?e=proxyConnectFailure&u=https%3A//example.co
12-17 20:15:13.504 E/Mochitest(  796): Content JS ERROR: net-error
12-17 20:15:13.504 E/Mochitest(  796):     at initPage (about:neterror?e=proxyConnectFailure&u=https%3A//example.com/tests/dom/base/test/csp/file_csp_testserver.sjs%3Fid%3Dorigin%26csp%3Dscript-src%2520*%2520%2527unsafe-inline%2527%253B%2520referrer%2520origin%26file%3Dtests/dom/base/test/csp/file_csp_referrerdirective.html&c=UTF-8&f=regular&m=http%3A//mochi.test%3A8888/manifest.webapp&d=Firefox%20is%20configured%20to%20use%20a%20proxy%20server%20that%20is%20refusing%20connections.:1231:6965)
12-17 20:15:13.504 E/Mochitest(  796):     at ee_emit (about:neterror?e=proxyConnectFailure&u=https%3A//example.com/tests/dom/base/test/csp/file_csp_testserver.sjs%3Fid%3Dorigin%26csp%3Dscript-src%2520*%2520%2527unsafe-inline%2527%253B%2520referrer%2520origin%26file%3Dtests/dom/base/test/csp/file_csp_referrerdirective.html&c=UTF-8&f=regular&m=http%3A//mochi.test%3A8888/manifest.webapp&d=Firefox%20is%20configured%20to%20use%20a%20proxy%20server%20that%20is%20refusing%20connections.:991:88)
12-17 20:15:13.504 E/Mochitest(  796):     at setReady (about:neterror?e=proxyConnectFailure&u=https%3A//example.com/te
12-17 20:15:13.534 E/Mochitest(  796): Content JS ERROR: net-error
12-17 20:15:13.534 E/Mochitest(  796):     at initPage (about:neterror?e=proxyConnectFailure&u=https%3A//example.com/tests/dom/base/test/csp/file_csp_testserver.sjs%3Fid%3Dorigin-when-crossorigin%26csp%3Dscript-src%2520*%2520%2527unsafe-inline%2527%253B%2520referrer%2520origin-when-crossorigin%26file%3Dtests/dom/base/test/csp/file_csp_referrerdirective.html&c=UTF-8&f=regular&m=http%3A//mochi.test%3A8888/manifest.webapp&d=Firefox%20is%20configured%20to%20use%20a%20proxy%20server%20that%20is%20refusing%20connections.:1231:6965)
12-17 20:15:13.534 E/Mochitest(  796):     at ee_emit (about:neterror?e=proxyConnectFailure&u=https%3A//example.com/tests/dom/base/test/csp/file_csp_testserver.sjs%3Fid%3Dorigin-when-crossorigin%26csp%3Dscript-src%2520*%2520%2527unsafe-inline%2527%253B%2520referrer%2520origin-when-crossorigin%26file%3Dtests/dom/base/test/csp/file_csp_referrerdirective.html&c=UTF-8&f=regular&m=http%3A//mochi.test%3A8888/manifest.webapp&d=Firefox%20is%20configured%20to%20use%20a%20proxy%20server%20that%20is%20refusing%20connections.:991:88)
ckerschb pointed me here, same problem:
https://bugzilla.mozilla.org/show_bug.cgi?id=826805#c17
Assignee

Updated

4 years ago
Blocks: 1140638
Depends on: CVE-2016-2827
You need to log in before you can comment on or make changes to this bug.