949533 - (csp-legacy-removal) Remove non-standard CSP parser and X-Content-Security-Policy header support

Status: RESOLVED FIXED

(Core :: Security, enhancement)

Description

[Sid Stamm [:geekboy or :sstamm]]

Once roll-out and adoption of CSP 1.0 has begun, we should remove support for the legacy implementation.  Ideally, we could do this at the same time as fixing bug 925004 (rewriting it in C++).

Deprecation warnings have been in the product since releasing CSP 1.0 (per the W3C spec), so it's time to start removing it.

Comment 1

[Sid Stamm [:geekboy or :sstamm]]

Attachment: remove-x-support

Well, here's a first whack.

Still needs to delete the x- header mochitests (requires bug 988616) and requires the new backend to land with reporting support.

Comment 2

[Garrett Robinson [:grobinson]]

Comment on attachment 8408617 [details] [diff] [review]
remove-x-support

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

You should also remove the security.csp.speccompliant pref definition from browser/app/profile/firefox.js and mobile/android/app/mobile.js.

For the unit tests: this bug should have a test to remove all of the x- unit tests (once they've been split, so bug 988616 has to land first). Additionally, some tests will be removed completely because they test the behavior when sites serve both headers (e.g. test_dual_headers_warning.html).

::: content/base/src/contentSecurityPolicy.js
@@ +80,5 @@
>    /* shouldn't see this one */
>    csp._MAPPINGS[cp.TYPE_REFRESH]           =  null;
>  
>    /* categorized content types */
> +  /* NOTE: order in this array is important !!! */

Why? (include in comment)

@@ +85,1 @@
>    // These are the same in old and new CSP's so just use the new mappings.

Remove this comment

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

When we remove the x-header support, these files should go (see bug 988616 comment 35):
> browser/devtools/webconsole/test/test-bug-821877-csperrors.html
> browser/devtools/webconsole/test/test-bug-821877-csperrors.html^headers^
> browser/devtools/webconsole/test/browser_webconsole_bug_821877_csp_errors.js
> content/base/test/unit/test_csputils.js
> content/base/test/unit/test_bug558431.js
> browser/devtools/webconsole/test/browser_webconsole_bug_770099_bad_policyuri.js
> browser/devtools/webconsole/test/test_bug_770099_bad_policy_uri.html
> browser/devtools/webconsole/test/test_bug_770099_bad_policy_uri.html^headers^

And these test files should be updated to test CSP:
> dom/workers/test/csp_worker.js
> dom/workers/test/test_csp.html
> dom/workers/test/test_csp.html^headers^
> dom/workers/test/test_csp.js
> content/base/test/unit/test_cspreports.js
> browser/devtools/webconsole/test/test_bug_770099_violation.html
> browser/devtools/webconsole/test/browser_webconsole_bug_770099_violation.js
> browser/devtools/webconsole/test/test_bug_770099_violation.html^headers^

We should consider porting test_csputils.js and test_csp_ignores_path.js to the new parser (though looks like a lot of that is already in TestCSPParser.cpp).

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

Now that bug 858787 has landed and the CSP tests have been split into xcsp and csp directories (thanks, Garrett!), nothing should require us keeping around X- header support (except the headers themselves).  We can now begin removing the legacy header support (but not yet the js implementation of CSP 1.0).

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

Attachment: remove-x-support

Added a bunch more test removal stuff to the patch (thanks grobinson for splitting the tests!) and pushed to try with new backend enabled (we should probably try it without the new backend too before long):
https://tbpl.mozilla.org/?tree=Try&rev=b52a8d35b36a

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

Looks like this b-c test broke:
browser/devtools/webconsole/test/browser_webconsole_bug_1010953_cspro.js, possibly because the warning posted to the console does not equal the warning expected (maybe default port isn't printed out).

And potentially a B2G ICS Emulator crash in Mochitest 9, though that could be a random orange.  Retriggering.

After those fixes, I'll run it on try again and then flag for review.

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

And for ease of review, I'll split the "delete this file" changes out into a separate patch.

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #6)
> Looks like this b-c test broke:
> browser/devtools/webconsole/test/browser_webconsole_bug_1010953_cspro.js,
> possibly because the warning posted to the console does not equal the
> warning expected (maybe default port isn't printed out).

Had to patch CSPUtils.jsm to avoid printing the default port when it's used (:80, for example) and had to update the new backend to use a different string for report-only violations by threading the report-only flag to the CSPReportSenderRunnable.  Pretty minor changes.

> And potentially a B2G ICS Emulator crash in Mochitest 9, though that could
> be a random orange.  Retriggering.

It was random.  New patches coming shortly.

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

Attachment: remove-x-support-changes

Removing code paths for x-csp support (stop reading the header and stop using the non-spec-compliant bits).

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

Attachment: remove-x-support-deletions

Apply this patch with the -changes patch (this patch is just file removals).

Comment 11

[Sid Stamm [:geekboy or :sstamm]]

These new patches are on try, using the old backend (security.csp.newbackend.enable is set to false as on current mozilla-central).
https://tbpl.mozilla.org/?tree=Try&rev=5d3558e8ad27

Comment 12

[Sid Stamm [:geekboy or :sstamm]]

Looks like I missed some xpcshell tests that assume default ports would be created by CSPSource.toString() (which this patch changes so they don't).  That's an easy fix in test_csputils.js and test_csp_ignores_path.js to just remove :443 and :80 from the "expected output" strings.

Like this:
-      var parsed = "http://foo.bar:21 https://ras.bar:443";
+      var parsed = "http://foo.bar:21 https://ras.bar";

Our new backend (nsCSP*.cpp) should not return the port with stringified sources when the port is equal to the default port.  Also, these two xpcshell tests are not run with the new backend since they test CSPUtils.jsm directly.

I'll update the patches with these two xpcshell test changes after Garrett's review.

Comment 13

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8440871 [details] [diff] [review]
remove-x-support-changes

Hey Johnny: flagging you for review on the nsDocument.cpp changes.  Feel free to review other stuff too if you feel so inclined.

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 8440871 [details] [diff] [review]
remove-x-support-changes

r=jst for the dom pieces here.

Comment 15

[Garrett Robinson [:grobinson]]

Comment on attachment 8440871 [details] [diff] [review]
remove-x-support-changes

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

You also need to remove the reference to the speccompliant pref from b2g/app/b2g.js. r+ with that fixed and the comments addressed.

::: content/base/src/CSPUtils.jsm
@@ +1249,5 @@
>      if (this.scheme)
>        s = s + this.scheme + "://";
>      if (this._host)
>        s = s + this._host;
> +    if (this.port && gIoService.getProtocolHandler(this.scheme).defaultPort != this.port)

I'm assuming this is for spec-compliance, because CSP 1.0 4.11 [0] says the report should use URI-reference from RFC 3986, 3.2.3 [1], which says the default port should be omitted. I also think this improves readability.

Just making a note here since the reason for this change was non-obvious.

[0] http://www.w3.org/TR/CSP/#report-uri
[1] http://www.ietf.org/rfc/rfc3986.txt

::: content/base/src/contentSecurityPolicy.js
@@ +105,4 @@
>    // TODO : EventSource will be here and also will use connect-src
>    // after we fix Bug 802872 - CSP should restrict EventSource using the connect-src
>    // directive. For background see Bug 667490 - EventSource should use the same
>    // nsIContentPolicy type as XHR (which is fixed)

Side note: we should remove this comment, bug 802872 has been resolved.

It's a little confusing: EventSource triggers Content Policy checks with type TYPE_DATAREQUEST, but that's an alias for TYPE_XMLHTTPREQUEST [0], so we use the same logic as for XHR and everything works.

We can remove this as part of this patch (might as well, IMO), or do it in a follow up.

[0] http://dxr.mozilla.org/mozilla-central/source/content/base/public/nsIContentPolicy.idl#108

::: content/base/src/nsCSPContext.cpp
@@ +887,5 @@
>          nsString blockedDataChar16 = NS_ConvertUTF8toUTF16(blockedDataStr);
>          const char16_t* params[] = { mViolatedDirective.get(),
>                                       blockedDataChar16.get() };
> +        CSP_LogLocalizedStr(mReportOnly ? NS_LITERAL_STRING("CSPROViolationWithURI").get()
> +                                        : NS_LITERAL_STRING("CSPViolationWithURI").get(),

All of the changes to this file are equivalent to the patch for bug 1011058. Did you mean to include them here? I don't think these are appropriate to include in this patch, as they are unrelated to the description. Instead, we should just land bug 1011058 independently.

Comment 16

[Garrett Robinson [:grobinson]]

Comment on attachment 8440872 [details] [diff] [review]
remove-x-support-deletions

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

This patch doesn't apply cleanly:

1 out of 1 hunks FAILED -- saving rejects to file content/base/test/xcsp/test_CSP_frameancestors.html.rej
patch failed, unable to continue (try -v)

It's look pretty good, but I'm going to wait until it applies cleanly to review it again.

Comment 17

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Garrett Robinson [:grobinson] from comment #16)
> Comment on attachment 8440872 [details] [diff] [review]
> remove-x-support-deletions
> 
> Review of attachment 8440872 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch doesn't apply cleanly:
> 
> 1 out of 1 hunks FAILED -- saving rejects to file
> content/base/test/xcsp/test_CSP_frameancestors.html.rej
> patch failed, unable to continue (try -v)

test_CSP_frameancestors.html changed, so the removal failed (file is different).  I'll update the patch, but basically the patch just removes that file.

Comment 18

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Garrett Robinson [:grobinson] from comment #15)
> Comment on attachment 8440871 [details] [diff] [review]
> 
> ::: content/base/src/CSPUtils.jsm
> I'm assuming this is for spec-compliance, because CSP 1.0 4.11 [0] says the
> report should use URI-reference from RFC 3986, 3.2.3 [1], which says the
> default port should be omitted. I also think this improves readability.
> 
> Just making a note here since the reason for this change was non-obvious.

I put a note in a comment so it's clearer in the code.  Nice explanation!

> ::: content/base/src/contentSecurityPolicy.js
> @@ +105,4 @@
> >    // TODO : EventSource will be here and also will use connect-src
...
> We can remove this as part of this patch (might as well, IMO), or do it in a
> follow up.

Good call!  Will do it in this bug.

> ::: content/base/src/nsCSPContext.cpp
> All of the changes to this file are equivalent to the patch for bug 1011058.
> Did you mean to include them here? I don't think these are appropriate to
> include in this patch, as they are unrelated to the description. Instead, we
> should just land bug 1011058 independently.

Ah, yeah, I spaced.  We need those changes for tests to succeed with the new backend enabled (see comment 8).  I'll remove those changes from this patch, though we'll have to fix the new backend before turning it on or the same tests will fail.

Comment 19

[Sid Stamm [:geekboy or :sstamm]]

Attachment: remove-x-support-changes

Fixed as per Garrett's most excellent comments and re-exported.  Carrying over r=jst,grobinson.

Comment 20

[Sid Stamm [:geekboy or :sstamm]]

Attachment: remove-x-support-deletions

This should apply cleanly.  Garrett?

Comment 21

[Sid Stamm [:geekboy or :sstamm]]

Looks good: https://tbpl.mozilla.org/?tree=Try&rev=824b5446963b

Comment 22

[Sid Stamm [:geekboy or :sstamm]]

Attachment: remove-x-support-deletions

Added r=grobinson to commit message.

Comment 24

[Sid Stamm [:geekboy or :sstamm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f70027b9330b
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a3662513dd7

Comment 25

[Ian Melven :imelven]

GREAT SUCCESS ! :D

Comment 26

[Johnny Stenback (:jst)]

ZOMG, amazing :)

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2a3662513dd7
https://hg.mozilla.org/mozilla-central/rev/f70027b9330b