Closed Bug 1279420 Opened 3 years ago Closed 3 years ago

Add pref for require-sri-for

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox48 --- unaffected
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: ckerschb, Assigned: jkt)

References

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file)

No description provided.
Blocks: 1265318
Whiteboard: [domsecurity-backlog]
Summary: Add pref require-sri-for → Add pref for require-sri-for
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.
(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
Assignee: nobody → jkingston
Status: NEW → ASSIGNED
Whiteboard: [domsecurity-backlog] → [domsecurity-active]
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
Priority: -- → P1
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!
Attachment #8763979 - Flags: review?(gpascutto)
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/
Attachment #8763979 - Flags: review?(gpascutto)
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 :).
Attachment #8763979 - Flags: review?(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");
Attachment #8763979 - Flags: review?(ckerschb)
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 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 :)
Attachment #8763979 - Flags: review?(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?
Attachment #8763979 - Flags: review?(ckerschb) → review+
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/
Attachment #8763979 - Flags: review+
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.
Attachment #8763979 - Flags: review?(ckerschb)
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.
Flags: needinfo?(francois)
Attachment #8763979 - Flags: review?(ckerschb) → review+
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
(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.
Flags: needinfo?(francois)
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
Attachment #8763979 - Flags: approval-mozilla-aurora?
(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.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/259215eef842
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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.
Attachment #8763979 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.