1279420 - Add pref for require-sri-for

Status: RESOLVED FIXED

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

Comment 1

[François Marier [:francois]]

I would also suggest it should be disabled by default and that we should uplift this to ensure we don't ship it and that developers start using it.

It would be unfortunate to have to maintain compatibility with this still-changing spec.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

(In reply to François Marier [:francois] from comment #1)
> I would also suggest it should be disabled by default and that we should
> uplift this to ensure we don't ship it and that developers start using it.

Indeed it should be disabled by default. Jonathan volunteered to add that pref. I think it's fine if we just disable the feature within the CSP parser.

We also have to update the test added within Bug 1265318 to switch the pref on before running the test.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPParser.cpp#1161

Comment 3

[Jonathan Kingston [:jkt] he/him]

So there is an old pref that exists in about:config which looks like it is no longer used. It probably makes most sense to reuse that: security.csp.experimentalEnabled

Comment 4

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=798c1ad49bc8

Comment 5

[Jonathan Kingston [:jkt] he/him]

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

Comment 6

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0253d4be8928

Comment 7

[Jonathan Kingston [:jkt] he/him]

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

Comment 8

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e4055476b9b

Comment 9

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1279420 - Adding in security.csp.experimentalEnabled pref check to require-sri-for directive in CSP

Review commit: https://reviewboard.mozilla.org/r/60052/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60052/

Comment 10

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8763979 [details]
Bug 1279420 - Adding in security.csp.experimentalEnabled pref check to require-sri-for directive in CSP

Hi :gcp not sure if you are the most relevant person to assign to review however feel free to pass it along and let me know etc :). Thanks!

Comment 11

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8763979 [details]
Bug 1279420 - Adding in security.csp.experimentalEnabled pref check to require-sri-for directive in CSP

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60052/diff/1-2/

Comment 12

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8763979 [details]
Bug 1279420 - Adding in security.csp.experimentalEnabled pref check to require-sri-for directive in CSP

Are you free to review this :ckerschb?
I updated based upon your feedback :).

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8763979 [details]
Bug 1279420 - Adding in security.csp.experimentalEnabled pref check to require-sri-for directive in CSP

https://reviewboard.mozilla.org/r/60052/#review57372

Hey jkt, that all looks good but I just realized something. In fact there is no need for having a special case handling for require-sri within directiveValue at all, because the parser already bails out of directive() when encountering require-sri. Can you please remove that part, that shouldn't have been there in the first place.

And I suppose we only need to check for the experimetnal flag (as you already correctly do) within directiveName(). If directiveName() returns a nullptr, then directive() already returns before making it all the way down to |cspDir->equals(nsIContentSecurityPolicy::REQUIRE_SRI_FOR)) {|.

Everything else looks great, can you just update that part and flag me again?


+++ b/dom/security/nsCSPParser.cpp
@@ -1002,37 +1002,32 @@ nsCSPParser::directiveValue(nsTArray<nsC
 
   // special case handling of the referrer directive (since it doesn't contain
   // source lists)
   if (CSP_IsDirective(mCurDir[0], nsIContentSecurityPolicy::REFERRER_DIRECTIVE)) {
     referrerDirectiveValue();
     return;
   }
 
-  // special case handling of the require-sri-for directive (since it doesn't
-  // contain a source lists but rather types, e.g., style or script)
-  if (CSP_IsDirective(mCurDir[0], nsIContentSecurityPolicy::REQUIRE_SRI_FOR)) {
-    // handled in directive()
-    return;
-  }
-
   // Otherwise just forward to sourceList
   sourceList(outSrcs);
 }
 
 // directive-name = 1*( ALPHA / DIGIT / "-" )
 nsCSPDirective*
 nsCSPParser::directiveName()
 {
   CSPPARSERLOG(("nsCSPParser::directiveName, mCurToken: %s, mCurValue: %s",
                NS_ConvertUTF16toUTF8(mCurToken).get(),
                NS_ConvertUTF16toUTF8(mCurValue).get()));
 
   // Check if it is a valid directive
-  if (!CSP_IsValidDirective(mCurToken)) {
+  if (!CSP_IsValidDirective(mCurToken) ||
+       (!sCSPExperimentalEnabled &&
+         CSP_IsDirective(mCurToken, nsIContentSecurityPolicy::REQUIRE_SRI_FOR))) {
     const char16_t* params[] = { mCurToken.get() };
     logWarningErrorToConsole(nsIScriptError::warningFlag, "couldNotProcessUnknownDirective",
                              params, ArrayLength(params));
     return nullptr;
   }
 
   // The directive 'reflected-xss' is part of CSP 1.1, see:
   // http://www.w3.org/TR/2014/WD-CSP11-20140211/#reflected-xss

::: dom/security/test/sri/test_require-sri-for_csp_directive_disabled.html:37
(Diff revision 2)
> +        var blackTextColor = frame.contentWindow.getComputedStyle(blackText, null).getPropertyValue('color');
> +        ok(blackTextColor != 'rgb(0, 0, 0)', "The second part should still be black.");
> +        removeEventListener('message', handler);
> +        SimpleTest.finish();
> +        break;
> +      default:

I suppose we never get down to default:, right? Would it make sense to add a:
ok(false, "something's wrog here");

Comment 14

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8763979 [details]
Bug 1279420 - Adding in security.csp.experimentalEnabled pref check to require-sri-for directive in CSP

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60052/diff/2-3/

Comment 15

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8763979 [details]
Bug 1279420 - Adding in security.csp.experimentalEnabled pref check to require-sri-for directive in CSP

Should have addressed your points :)

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8763979 [details]
Bug 1279420 - Adding in security.csp.experimentalEnabled pref check to require-sri-for directive in CSP

https://reviewboard.mozilla.org/r/60052/#review57412

Looks fine except my nit underneath; thanks; r=me

::: dom/security/nsCSPParser.cpp:1164
(Diff revision 3)
>    }
>  
>    // special case handling for require-sri-for, which has directive values that
>    // are well-defined tokens but are not sources
> -  if (cspDir->equals(nsIContentSecurityPolicy::REQUIRE_SRI_FOR)) {
> +  if (sCSPExperimentalEnabled && cspDir->equals(nsIContentSecurityPolicy::REQUIRE_SRI_FOR)) {
>      requireSRIForDirectiveValue(static_cast<nsRequireSRIForDirective*>(cspDir));

if directiveName() returns a nullptr, then we don't even get here, no need to add the sCSPExperimentalEnabled check here, right?

Comment 17

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8763979 [details]
Bug 1279420 - Adding in security.csp.experimentalEnabled pref check to require-sri-for directive in CSP

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60052/diff/3-4/

Comment 18

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8763979 [details]
Bug 1279420 - Adding in security.csp.experimentalEnabled pref check to require-sri-for directive in CSP

Sorry missed your comment about the directive name.

Comment 19

[Jonathan Kingston [:jkt] he/him]

This needs uplifting also right?
Should be ready to go, is there anything we have to do for that to happen. I have not uplifted a patch yet.

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8763979 [details]
Bug 1279420 - Adding in security.csp.experimentalEnabled pref check to require-sri-for directive in CSP

https://reviewboard.mozilla.org/r/60052/#review57510

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonathan Kingston [:jkt] from comment #19)
> This needs uplifting also right?
> Should be ready to go, is there anything we have to do for that to happen. I
> have not uplifted a patch yet.

If you hit "Details" right next to the patch, you find the flag 'approval‑mozilla‑aurora'. Then just fill out the uplift request.

Comment 22

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8763979 [details]
Bug 1279420 - Adding in security.csp.experimentalEnabled pref check to require-sri-for directive in CSP

Approval Request Comment
[Feature/regressing bug #]: 1265318
[User impact if declined]:
Websites will be able to use an unspecified require-sri-for CSP feature in the browser before it is complete.
[Describe test coverage new/current, TreeHerder]:
Added a new test to ensure the pref flag doesn't make feature work: dom/security/test/sri/test_require-sri-for_csp_directive_disabled.html
[Risks and why]:
Websites start using a feature that potentially could be changed which would cause breakages within websites.
[String/UUID change made/needed]:
None

Comment 23

[François Marier [:francois]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #21)
> (In reply to Jonathan Kingston [:jkt] from comment #19)
> > This needs uplifting also right?
> > Should be ready to go, is there anything we have to do for that to happen. I
> > have not uplifted a patch yet.
> 
> If you hit "Details" right next to the patch, you find the flag
> 'approval‑mozilla‑aurora'. Then just fill out the uplift request.

It probably needs to land on mozilla-central before the uplift can be approved.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/259215eef842
Adding in security.csp.experimentalEnabled pref check to require-sri-for directive in CSP. r=ckerschb

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/259215eef842

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8763979 [details]
Bug 1279420 - Adding in security.csp.experimentalEnabled pref check to require-sri-for directive in CSP

We don't want to ship this enabled by accident, please uplift to aurora.

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6683dd1bf7fd