SRI: implement require-sri-for resources (enforce Subresource Integrity)

RESOLVED FIXED in Firefox 49

Status

()

Core
DOM: Security
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: freddyb, Assigned: freddyb)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla49
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [domsecurity-active], URL)

Attachments

(3 attachments, 8 obsolete attachments)

6.80 KB, patch
freddyb
: review+
Details | Diff | Splinter Review
22.63 KB, patch
freddyb
: review+
Details | Diff | Splinter Review
774 bytes, patch
mrbkap
: review+
Details | Diff | Splinter Review
(Assignee)

Description

a year ago
This should be relatively simple, using the `enforceSRI` plumbing introduced in bug 1235572.
(Assignee)

Updated

a year ago
Blocks: 1231788
(Assignee)

Updated

a year ago
Assignee: nobody → fbraun
Whiteboard: [domsecurity-active]
(Assignee)

Updated

a year ago
(Assignee)

Comment 1

a year ago
This has moved from the CSP3 spec to SRI, changing the title to reflect that.
Summary: CSP3: implement block-non-sri-resources (enforce Subresource Integrity) → SRI: implement require-sri-for resources (enforce Subresource Integrity)
(Assignee)

Comment 2

a year ago
Created attachment 8751283 [details] [diff] [review]
0001-Bug-1265318-add-require-sri-for-CSP-directive.patch
Attachment #8751283 - Flags: feedback?(ckerschb)
(Assignee)

Comment 3

a year ago
Created attachment 8751284 [details] [diff] [review]
0002-need-to-add-more-than-1-contentPolicyType-per-token.patch
Attachment #8751284 - Flags: feedback?(ckerschb)
(Assignee)

Comment 4

a year ago
Created attachment 8751285 [details] [diff] [review]
0003-test-for-require-sri-for-script-style-still-missing.patch
Attachment #8751285 - Flags: feedback?(ckerschb)
Comment on attachment 8751283 [details] [diff] [review]
0001-Bug-1265318-add-require-sri-for-CSP-directive.patch

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

Looks already pretty good!

::: dom/security/nsCSPContext.cpp
@@ +1133,5 @@
> +        return NS_OK;
> +      }
> +    }
> +  }
> +  *outRequiresSRIForType = false;

Nit: please set *outRequiresSRIForType to false before the for loop.

::: dom/security/nsCSPParser.cpp
@@ +17,4 @@
>  #include "nsServiceManagerUtils.h"
>  #include "nsUnicharUtils.h"
>  #include "mozilla/net/ReferrerPolicy.h"
> +#include "../../mfbt/Assertions.h"

Is this really needed? I suppose we can remove that inclusion.

@@ +60,5 @@
>  static const char *const kHashSourceValidFns [] = { "sha256", "sha384", "sha512" };
>  static const uint32_t kHashSourceValidFnsLen = 3;
>  
> +static const char kStyle  []             = "style";
> +static const char kScript []             = "script";

you can use:
static const char* const kStyle = "style";

@@ +916,5 @@
>    mPolicy->setReferrerPolicy(&mCurDir[1]);
>  }
>  
> +inline bool
> +nsCSPParser::isValidRequireSRIKeyword(const nsAString& content)

please prefix arguments with 'a' -> aKeyword..

@@ +919,5 @@
> +inline bool
> +nsCSPParser::isValidRequireSRIKeyword(const nsAString& content)
> +{
> +  return content.LowerCaseEqualsLiteral(kScript)
> +      || content.LowerCaseEqualsLiteral(kStyle);

nit: since we use the || at the end of the line within this file, please also do so here.

return aKeyword.LowerCaseEqualsASCII(kScript) ||
       aKeyword.LowerCaseEqualsASCII(kStyle);

@@ +930,5 @@
> +      return nsIContentPolicy::TYPE_INTERNAL_SCRIPT_PRELOAD;
> +  } else if (keyword.LowerCaseEqualsLiteral(kStyle)) {
> +      return nsIContentPolicy::TYPE_INTERNAL_STYLESHEET;
> +  } else {
> +    MOZ_CRASH("requireSRIKeywordToType called without proper keyword!");

Please use early returns, e.g.
if (keyword.LowerCaseEqualsLiteral(kScript)) {
  return nsIContentPolicy::TYPE_INTERNAL_SCRIPT_PRELOAD;
}

if (keyword.LowerCaseEqualsLiteral(kStyle)) {
  return nsIContentPolicy::TYPE_INTERNAL_STYLESHEET;
}

and I suppose we don't want to use MOZ_CRASH, can we use something else?

Also, I think you should use the extneral content policyType everywhere to avoid any potential bugs, so use:
TYPE_SCRIPT and TYPE_STYLESHEET everywhere please.

@@ +939,5 @@
> +nsCSPParser::requireSRIForDirectiveValue()
> +{
> +  // XXX (TBD) directive-value   = "style" / "script"
> +  // directive name is token 0, we need to examine the remaining tokens (and
> +  // there should only be one token in the value).

nit: move that comment right before the 'for' loop.

@@ +963,5 @@
> +                 "(valid), mCurValue: %s",
> +                 NS_ConvertUTF16toUTF8(mCurToken).get(),
> +                 NS_ConvertUTF16toUTF8(mCurValue).get()));
> +
> +    mPolicy->addRequiredSRIType(requireSRIKeywordToType(mCurToken));

Does that really need to live on the policy? I suppose it would make more sense to add that to the directive, no? Just add the directive. Once you move that into ::dirctive() you have access to the dir |cspDir|.

@@ +1022,5 @@
> +  // contain source lists)
> +  if (CSP_IsDirective(mCurDir[0], nsIContentSecurityPolicy::REQUIRE_SRI_FOR)) {
> +    requireSRIForDirectiveValue();
> +    return;
> +  }

move that part into ::directive()

@@ +1105,5 @@
>    }
>  
> +  if (CSP_IsDirective(mCurToken, nsIContentSecurityPolicy::REQUIRE_SRI_FOR)) {
> +    return new nsRequireSRIForDirective(CSP_StringToCSPDirective(mCurToken));
> +  }

keep that.

@@ +1171,5 @@
> +  // are well-defined tokens but are not sources
> +  if (cspDir->equals(nsIContentSecurityPolicy::REQUIRE_SRI_FOR)) {
> +    mPolicy->addRequireSRIDir(static_cast<nsRequireSRIForDirective*>(cspDir));
> +  }
> +

since it's such a special directive, we should do all of the handling within here, so call requireSRIForDirectiveValue and return early.

::: dom/security/nsCSPParser.h
@@ +119,5 @@
> +    void                directiveValue(nsTArray<nsCSPBaseSrc*>& outSrcs);
> +    void                referrerDirectiveValue();
> +    inline bool         isValidRequireSRIKeyword(const nsAString& content);
> +    nsContentPolicyType requireSRIKeywordToType(const nsAString& keyword);
> +    void                requireSRIForDirectiveValue();

I suppose only requireSRIDirectiveValue needs to be a method of nsCSPParser, right? The other two functions isValidRequireSRIKeyword and requireSRIKeywordToType are only helper functions and don't need access to class internals and hence can live only the the cpp file. Please define them as static.

::: netwerk/base/LoadInfo.cpp
@@ +149,5 @@
> +    // If the CSP has the directive to require SRI, set this here
> +    if (mEnforceSRI == false) { // no need to peek into the CSP if already true
> +      if ((aContentPolicyType == nsIContentPolicy::TYPE_INTERNAL_SCRIPT) ||
> +          (aContentPolicyType == nsIContentPolicy::TYPE_INTERNAL_SCRIPT_PRELOAD) ||
> +          (aContentPolicyType == nsIContentPolicy::TYPE_STYLESHEET)) {

Please use nsContentUtils::InternalContentPolicyTypeTOExernal(), then you only have to check for TYPE_SCRIPT or TYPE_STYLESHEET.

@@ +152,5 @@
> +          (aContentPolicyType == nsIContentPolicy::TYPE_INTERNAL_SCRIPT_PRELOAD) ||
> +          (aContentPolicyType == nsIContentPolicy::TYPE_STYLESHEET)) {
> +        nsCOMPtr<nsIContentSecurityPolicy> csp;
> +        //XXX do we *need* to pass a document into EnsureCSP?
> +        nsresult rv = aLoadingPrincipal->EnsureCSP(nullptr, getter_AddRefs(csp));

We should already have a CSP available at this point, so please use:
readonly attribute nsIContentSecurityPolicy csp

aLoadingPrincipal->GetCsp(...)

@@ +153,5 @@
> +          (aContentPolicyType == nsIContentPolicy::TYPE_STYLESHEET)) {
> +        nsCOMPtr<nsIContentSecurityPolicy> csp;
> +        //XXX do we *need* to pass a document into EnsureCSP?
> +        nsresult rv = aLoadingPrincipal->EnsureCSP(nullptr, getter_AddRefs(csp));
> +        if ((rv == NS_OK) && (csp != nullptr)) {

then you can do
if (csp) {
...
}

@@ +157,5 @@
> +        if ((rv == NS_OK) && (csp != nullptr)) {
> +          // csp could be null if loading principal is system principal
> +          bool sriRequired = false;
> +          csp->RequireSRIForType(aContentPolicyType, &sriRequired);
> +          mEnforceSRI = sriRequired;

csp->RequireSRIForTYpe(&mEnforceSRI);

@@ +160,5 @@
> +          csp->RequireSRIForType(aContentPolicyType, &sriRequired);
> +          mEnforceSRI = sriRequired;
> +        }
> +      }
> +    }

Please keep all the CSP code (dom/security) in one patch and move the LoadInfo changes to a separate patch, because we potentially need review from a dom/ peer in the end.
Attachment #8751283 - Flags: feedback?(ckerschb)
Comment on attachment 8751284 [details] [diff] [review]
0002-need-to-add-more-than-1-contentPolicyType-per-token.patch

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

::: dom/security/nsCSPParser.cpp
@@ +958,5 @@
> +      mPolicy->addRequiredSRIType(nsIContentPolicy::TYPE_SCRIPT);
> +    } else if (keyword.LowerCaseEqualsLiteral(kStyle)) {
> +      mPolicy->addRequiredSRIType(nsIContentPolicy::TYPE_STYLESHEET);
> +      mPolicy->addRequiredSRIType(nsIContentPolicy::TYPE_INTERNAL_STYLESHEET);
> +    }

Please move that part into the CSP patch and use the external type everywhere.
Attachment #8751284 - Flags: feedback?(ckerschb)
Comment on attachment 8751285 [details] [diff] [review]
0003-test-for-require-sri-for-script-style-still-missing.patch

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

Looks good from a first glance, would be great to have one test for style and one for source and please also add tests for the Parser within TestCSPParser.cpp.
Thanks!

::: dom/security/test/sri/iframe_require-sri-for-script_main.html
@@ +15,5 @@
> +
> +
> +<script>
> +  window.onload = function() {
> +    parent.postMessage("finish", '*');

are you sure that doesn't fire before the script is completely loaded?

::: dom/security/test/sri/test_require-sri-for_csp_directive_script.html
@@ +29,5 @@
> +
> +  addEventListener("message", function(event) {
> +    switch (event.data) {
> +      case 'good_sriLoaded':
> +        good_sriLoaded();

nit: since it's only one line you could in the 'ok(...)' part here.
Attachment #8751285 - Flags: feedback?(ckerschb)
(Assignee)

Comment 8

a year ago
Created attachment 8752182 [details] [diff] [review]
0001-Bug-1265318-add-require-sri-for-CSP-directive-r-cker.patch

New iteration.
This is Part 1, for all the CSP changes and the tests
Attachment #8751283 - Attachment is obsolete: true
Attachment #8751284 - Attachment is obsolete: true
Attachment #8751285 - Attachment is obsolete: true
Attachment #8752182 - Flags: review?(ckerschb)
(Assignee)

Comment 9

a year ago
Created attachment 8752183 [details] [diff] [review]
0002-Bug-1265318-look-for-require-sri-for-CSP-directive-i.patch

This is part 2, which makes LoadInfo look into the current CSP.
Attachment #8752183 - Flags: review?(ckerschb)
(Assignee)

Comment 10

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbe1b75f33dc
Comment on attachment 8752182 [details] [diff] [review]
0001-Bug-1265318-add-require-sri-for-CSP-directive-r-cker.patch

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

Freddy, really getting there, two questions:
*) You are logging a message to the console within ScriptLoader.cpp, don't we want to do the same within style/Loader.cpp?
*) What about reporting? Should require-sri send a CSP report? Potentially yes.


Please create a separate patch where you bundle all the tests together, so actual code and tests are separate patches.

::: dom/base/nsScriptLoader.cpp
@@ +2320,5 @@
> +      nsContentUtils::ReportToConsole(nsIScriptError::errorFlag,
> +                                      NS_LITERAL_CSTRING("Sub-resource Integrity"),
> +                                      mDocument,
> +                                      nsContentUtils::eSECURITY_PROPERTIES,
> +                                      "RequiredMetadataMissing");

Probably we can log a little more here, e.g. from the scriptloadrequest you should be able to query the element and from that element you can query the scripttext and linenumber, see [1] for example.
I am pretty sure webdevs would appreciate the extra info.


[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#1241

::: dom/interfaces/security/nsIContentSecurityPolicy.idl
@@ +206,5 @@
>  
> +
> +  /*
> +   *
> +  */

Please add a comment explaining the functionality.
nit: indendation of last line of the comment.

::: dom/security/nsCSPParser.cpp
@@ +166,5 @@
>            (aHexDig >= 'a' && aHexDig <= 'f'));
>  }
>  
> +static bool
> +isValidRequireSRIKeyword(const nsAString& aContent)

nit: capitalize IsValidRequireSRIKeyword(

@@ +974,5 @@
>      return;
>    }
>  
> +  // special case handling of the require-sri-for directive (since it doesn't
> +  // contain source lists)

nit: (since it's not containing a common source-list but rather types, e.g. style or script).

@@ +1124,5 @@
>    }
>  
> +  // 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)) {

call requireSRIForDirectiveValue() here and move the following code in within that method.

@@ +1134,5 @@
> +      mCurToken = mCurDir[i];
> +      resetCurValue();
> +      if (!isValidRequireSRIKeyword(mCurToken)) {
> +        const char16_t* params[] = { mCurToken.get() };
> +        logWarningErrorToConsole(nsIScriptError::warningFlag, "ignoringUnknownOption",

I suppose you could use "ignoreSrcForDirective" instead of "ignoringUnkonwOption".

@@ +1148,5 @@
> +                    NS_ConvertUTF16toUTF8(mCurToken).get(),
> +                    NS_ConvertUTF16toUTF8(mCurValue).get()));
> +      // add contentPolicyTypes to the CSP's required-SRI list for this token
> +      if (mCurToken.LowerCaseEqualsASCII(kScript)) {
> +        (static_cast<nsRequireSRIForDirective*>(cspDir))->addType(nsIContentPolicy::TYPE_SCRIPT);

add before the for loop:
nsRequireSRIForDirective* requireSRIDir = static_cast<nsRequireSRIForDirective*>(cspDir);

@@ +1149,5 @@
> +                    NS_ConvertUTF16toUTF8(mCurValue).get()));
> +      // add contentPolicyTypes to the CSP's required-SRI list for this token
> +      if (mCurToken.LowerCaseEqualsASCII(kScript)) {
> +        (static_cast<nsRequireSRIForDirective*>(cspDir))->addType(nsIContentPolicy::TYPE_SCRIPT);
> +          continue;

remove continue and add requireqSRIDir->attType(...);

@@ +1151,5 @@
> +      if (mCurToken.LowerCaseEqualsASCII(kScript)) {
> +        (static_cast<nsRequireSRIForDirective*>(cspDir))->addType(nsIContentPolicy::TYPE_SCRIPT);
> +          continue;
> +      }
> +      if (mCurToken.LowerCaseEqualsASCII(kStyle)) {

else if

@@ +1153,5 @@
> +          continue;
> +      }
> +      if (mCurToken.LowerCaseEqualsASCII(kStyle)) {
> +        (static_cast<nsRequireSRIForDirective*>(cspDir))->addType(nsIContentPolicy::TYPE_STYLESHEET);
> +        continue;

remove continue;

@@ +1155,5 @@
> +      if (mCurToken.LowerCaseEqualsASCII(kStyle)) {
> +        (static_cast<nsRequireSRIForDirective*>(cspDir))->addType(nsIContentPolicy::TYPE_STYLESHEET);
> +        continue;
> +      }
> +    }

What happens if someone adds the following policy:
CSP: require-sri-for styles

I suppose we log a warning that 'styles' is not valid keyword, but we would still add the directive to the policy right? I suppose we should log another warning and return before calling addRequireSRIDir().

::: dom/security/nsCSPParser.h
@@ +118,5 @@
> +    nsCSPPolicy*        policy();
> +    void                directive();
> +    nsCSPDirective*     directiveName();
> +    void                directiveValue(nsTArray<nsCSPBaseSrc*>& outSrcs);
> +    void                referrerDirectiveValue();

add back requireSRIForDirectiveValue()

::: dom/security/nsCSPUtils.cpp
@@ +1091,5 @@
> +{
> +  outStr.AppendASCII(CSP_CSPDirectiveToString(
> +    nsIContentSecurityPolicy::REQUIRE_SRI_FOR));
> +  for (uint32_t i = 0; i < mTypes.Length(); i++) {
> +    //XXX need to turn types back into strings to append them here.

you have already implemented that part.

@@ +1094,5 @@
> +  for (uint32_t i = 0; i < mTypes.Length(); i++) {
> +    //XXX need to turn types back into strings to append them here.
> +    if (mTypes[i] == nsIContentPolicy::TYPE_SCRIPT) {
> +      outStr.AppendASCII(" script");
> +      continue;

remove continue;

@@ +1096,5 @@
> +    if (mTypes[i] == nsIContentPolicy::TYPE_SCRIPT) {
> +      outStr.AppendASCII(" script");
> +      continue;
> +    }
> +    if (mTypes[i] == nsIContentPolicy::TYPE_STYLESHEET) {

else if

@@ +1098,5 @@
> +      continue;
> +    }
> +    if (mTypes[i] == nsIContentPolicy::TYPE_STYLESHEET) {
> +      outStr.AppendASCII(" style");
> +      continue;

remove continue;

@@ +1101,5 @@
> +      outStr.AppendASCII(" style");
> +      continue;
> +    }
> +  }
> +

nit: remove empty line

@@ +1257,5 @@
>        outStr.AppendASCII(CSP_CSPDirectiveToString(nsIContentSecurityPolicy::REFERRER_DIRECTIVE));
>        outStr.AppendASCII(" ");
>        outStr.Append(mReferrerPolicy);
> +    } else if (mDirectives[i]->equals(nsIContentSecurityPolicy::REQUIRE_SRI_FOR)) {
> +      mRequire_SRI->toString(outStr);

remove

@@ +1361,5 @@
> +
> +bool
> +nsCSPPolicy::requireSRIForType(nsContentPolicyType aContentType)
> +{
> +  return mRequire_SRI->hasType(aContentType);

just loop through the directives here to find the right directive.

::: dom/security/nsCSPUtils.h
@@ +492,5 @@
> +
> +    void addType(nsContentPolicyType aType)
> +      { mTypes.AppendElement(aType); }
> +    bool hasType(nsContentPolicyType aType);
> +  private:

nit: please add empty line above private:

@@ +554,5 @@
> +    inline void addRequireSRIDir(nsRequireSRIForDirective* aDir)
> +      {
> +        mRequire_SRI = aDir;
> +        addDirective(aDir);
> +      }

remove that method and just call addDirective() from within the csp-parser.

@@ +566,4 @@
>  
>    private:
>      nsUpgradeInsecureDirective* mUpgradeInsecDir;
> +    nsRequireSRIForDirective*   mRequire_SRI;

no need to add a member here.

::: dom/security/test/TestCSPParser.cpp
@@ +509,5 @@
>        "connect-src 'none'" },
>      { "script-src https://foo.com/%$",
>        "script-src 'none'" },
> +    { "require-SRI-for script elephants",
> +        "require-sri-for script"}

require-sri-for paul

what should be the result? please add a test for that
Attachment #8752182 - Flags: review?(ckerschb)
Comment on attachment 8752183 [details] [diff] [review]
0002-Bug-1265318-look-for-require-sri-for-CSP-directive-i.patch

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

::: netwerk/base/LoadInfo.cpp
@@ +155,5 @@
> +        nsCOMPtr<nsIContentSecurityPolicy> csp;
> +        aLoadingPrincipal->GetCsp(getter_AddRefs(csp));
> +        // csp could be null if loading principal is system principal
> +        if (csp) {
> +          csp->RequireSRIForType(loadType, &mEnforceSRI);

what happens if you call csp->RequireSRIForType(TYPE_OTHER, ...)??
I think mEnforceSRI would be set to false, right?

To make this patch future proof I suppose we can even skip the if (loadType==TYPE_SCRIPT || TYPE_STYLESHEET), right?

What do you think?
Attachment #8752183 - Flags: review?(ckerschb)
(Assignee)

Comment 13

a year ago
Created attachment 8758223 [details] [diff] [review]
0001-Bug-1265318-add-require-sri-for-CSP-directive-r-cker.patch
Attachment #8752182 - Attachment is obsolete: true
Attachment #8752183 - Attachment is obsolete: true
Attachment #8758223 - Flags: review?(ckerschb)
(Assignee)

Comment 14

a year ago
Created attachment 8758224 [details] [diff] [review]
0002-Bug-1265318-tests-for-require-sri-for-CSP-directive-.patch
Attachment #8758224 - Flags: review?(ckerschb)
Comment on attachment 8758223 [details] [diff] [review]
0001-Bug-1265318-add-require-sri-for-CSP-directive-r-cker.patch

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

Getting really close, I would like to see it one more time though - thanks!

::: dom/base/nsScriptLoader.cpp
@@ +2325,5 @@
>               ("nsScriptLoader::OnStreamComplete, required SRI not found"));
> +      nsCOMPtr<nsIPrincipal> loadingPrincipal;
> +      loadInfo->GetLoadingPrincipal(getter_AddRefs(loadingPrincipal));
> +      nsCOMPtr<nsIContentSecurityPolicy> csp;
> +      loadingPrincipal->GetCsp(getter_AddRefs(csp));

You can simplify that
nsCOMPtr<nsIContentSecurityPolicy> csp;
loadinfo->LoadingPrincipal()->GetCSP(getter_AddRefs(csp));

@@ +2328,5 @@
> +      nsCOMPtr<nsIContentSecurityPolicy> csp;
> +      loadingPrincipal->GetCsp(getter_AddRefs(csp));
> +      nsCOMPtr<nsIURI> violationURI = mDocument->GetDocumentURI();
> +      nsAutoCString spec;
> +      violationURI->GetAsciiSpec(spec);

same here
nsAutoCString violationURISpec;
mDocument->GetDocumentURI()->GetAsciiSpec(violationURISPec);

@@ +2336,5 @@
> +      }
> +      csp->LogViolationDetails(
> +        nsIContentSecurityPolicy::VIOLATION_TYPE_REQUIRE_SRI_FOR_SCRIPT,
> +        NS_ConvertUTF8toUTF16(spec),
> +        EmptyString(), lineNo, EmptyString(), EmptyString());

and you can inline the linenumber:

...,
request->mElement ? request->mElement->GetScriptLineNumber : 0,
...,

::: dom/locales/en-US/chrome/security/csp.properties
@@ +72,5 @@
>  # %1$S is the URL of the blocked resource load.
>  blockAllMixedContent = Blocking insecure request ‘%1$S’.
> +# LOCALIZATION NOTE (ignoringDirectiveWithNoValues):
> +# %1$S is the name of a CSP directive that requires additional values (e.g., 'require-sri-for')
> +ignoringDirectiveWithNoValues = Directive ‘%1$S‘ should come with paramters, but it does not. It will be ignored.

can we change that to:
Ignoring ‘%1$S’ since it does not contain any parameters.

::: dom/locales/en-US/chrome/security/security.properties
@@ +69,5 @@
>  UnsupportedHashAlg=Unsupported hash algorithm in the integrity attribute: “%1$S”
>  # LOCALIZATION NOTE: Do not translate "integrity"
>  NoValidMetadata=The integrity attribute does not contain any valid metadata.
> +# LOCALIZATION NOTE: Do not translate "Content Security Policy" or "Subresource Integrity"
> +# “%1$S” is a URL

??? is that intentional? please remove

::: dom/security/nsCSPParser.cpp
@@ +924,4 @@
>  }
>  
>  void
> +nsCSPParser::requireSRIForDirectiveValue(nsCSPDirective* aDir) {

can you change the argument to nsRequireSRIForDirective* and do the cast on the callsite?

@@ +924,5 @@
>  }
>  
>  void
> +nsCSPParser::requireSRIForDirectiveValue(nsCSPDirective* aDir) {
> +  //  directive-value   = "style" / "script"

nit: only one space of indendation

@@ +952,5 @@
> +      requireSRIDir->addType(nsIContentPolicy::TYPE_SCRIPT);
> +    }
> +    else if (mCurToken.LowerCaseEqualsASCII(kStyle)) {
> +      requireSRIDir->addType(nsIContentPolicy::TYPE_STYLESHEET);
> +    }

I am confused, why would you need isValidRequireSRIKeyword if you check for kScript or kStyle underneath. My suggestion, remove isValidRequireSRIKeyword and have an else branch here where you can log to the console, makes sense?

@@ +960,5 @@
> +    mPolicy->addDirective(requireSRIDir);
> +  } else {
> +    logWarningErrorToConsole(nsIScriptError::warningFlag, "ignoringDirectiveWithNoValues",
> +                          directiveName, ArrayLength(directiveName));
> +  }

can you rewrite that to check if the directive does not contain styleSheet or srcipt, then log a warning an return early.

at the end you can just have:

mPolicy->AddDirective(requireSRIDir);
}

::: dom/security/nsCSPUtils.cpp
@@ +1121,5 @@
> +bool
> +nsRequireSRIForDirective::allows(enum CSPKeyword aKeyword, const nsAString& aHashOrNonce) const
> +{
> +  // can only disallow CSP_REQUIRE_SRI_FOR.
> +  return !(aKeyword == CSP_REQUIRE_SRI_FOR);

return aKeyword != CSP_REQURIE_SRI_FOR

::: layout/style/Loader.cpp
@@ +978,5 @@
> +      // line number unknown. mRequestingNode doesn't bear this info.
> +      csp->LogViolationDetails(
> +        nsIContentSecurityPolicy::VIOLATION_TYPE_REQUIRE_SRI_FOR_STYLE,
> +        NS_ConvertUTF8toUTF16(spec), EmptyString(),
> +        0, EmptyString(), EmptyString());

same as in nsScriptLoader, please simplify.

::: netwerk/base/LoadInfo.cpp
@@ +148,5 @@
>      }
>    }
>  
> +    // If the CSP has the directive to require SRI, set this here
> +    if (mEnforceSRI == false) {

if (!mEnforceSRI) {

@@ +151,5 @@
> +    // If the CSP has the directive to require SRI, set this here
> +    if (mEnforceSRI == false) {
> +      // do not look into the CSP if already true:
> +      // a CSP saying SRI isn't needed shouldnt overrule GetVerifySignedContent
> +      uint32_t loadType = nsContentUtils::InternalContentPolicyTypeToExternal(aContentPolicyType);

please move that line down right before you call csp->RequireSRIForType
Attachment #8758223 - Flags: review?(ckerschb)
Comment on attachment 8758224 [details] [diff] [review]
0002-Bug-1265318-tests-for-require-sri-for-CSP-directive-.patch

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

r=me, but please address my nits.

::: dom/security/test/TestCSPParser.cpp
@@ +271,5 @@
>        "referrer No-refeRRer" },
>      { "upgrade-INSECURE-requests",
> +      "upgrade-insecure-requests" },
> +      { "require-SRI-for sCript stYle",
> +        "require-sri-for script style"}

nit: indendation

@@ +509,5 @@
>        "connect-src 'none'" },
>      { "script-src https://foo.com/%$",
>        "script-src 'none'" },
> +    { "require-SRI-for script elephants",
> +        "require-sri-for script"},

nit: indendation

::: dom/security/test/sri/iframe_require-sri-for_main.html
@@ +20,5 @@
> +
> +<link rel="stylesheet" href="style3.css"
> +      onload="parent.postMessage('bad_nonsriLoaded', '*');"
> +      onerror="parent.postMessage('good_nonsriBlocked', '*');">
> +

can you please also add the comment whether it's going to be blocked or load for the style loads

::: dom/security/test/sri/test_require-sri-for_csp_directive.html
@@ +15,5 @@
> +</body>
> +<script type="application/javascript">
> +  SimpleTest.waitForExplicitFinish();
> +
> +  addEventListener("message", function(event) {

I am not sure actually, but don't you also have to remove the eventListener before calling finish()?

@@ +27,5 @@
> +      case 'good_nonsriBlocked':
> +        ok(true, "Eligible non-SRI resources was correctly blocked by the CSP.");
> +        break;
> +      case 'finish':
> +        var frame = document.getElementById("test_frame");

you can define that on top then you can also use it further down and just call:
frame.src =

@@ +39,5 @@
> +    }
> +  })
> +
> +
> +  //not needed, I guess good_inlineScriptLoaded();

what about this comment? is this needed? if not, please remove
!

@@ +42,5 @@
> +
> +  //not needed, I guess good_inlineScriptLoaded();
> +</script>
> +<script>
> +document.getElementById("test_frame").src = "iframe_require-sri-for_main.html";

why is this within separate <script> tags? can you simplify?
Attachment #8758224 - Flags: review?(ckerschb) → review+
(Assignee)

Comment 17

a year ago
Created attachment 8758326 [details] [diff] [review]
0002-Bug-1265318-tests-for-require-sri-for-CSP-directive-.patch

addressed test nits. carrying over r+
Attachment #8758224 - Attachment is obsolete: true
Attachment #8758326 - Flags: review+
(Assignee)

Comment 18

a year ago
Created attachment 8758327 [details] [diff] [review]
0001-Bug-1265318-add-require-sri-for-CSP-directive-r-cker.patch

Thank you for the speedy reviews.
and another round :)
Attachment #8758223 - Attachment is obsolete: true
Attachment #8758327 - Flags: review?(ckerschb)
(Assignee)

Comment 19

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7ad585c33f2
Comment on attachment 8758327 [details] [diff] [review]
0001-Bug-1265318-add-require-sri-for-CSP-directive-r-cker.patch

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

r=me, but please address my nits.

Can you please also exercose all the fuzzy tests for the parser before landing? Just flip the switch here [1] and run the tests locally, but don't commit with the fuzzy tests enabled.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/security/test/TestCSPParser.cpp#60

::: dom/security/nsCSPParser.cpp
@@ +919,5 @@
>  void
> +nsCSPParser::requireSRIForDirectiveValue(nsRequireSRIForDirective* aDir) {
> +  // directive-value = "style" / "script"
> +  // directive name is token 0, we need to examine the remaining tokens
> +  const char16_t* directiveName[] = { mCurToken.get() };

Now that I think about it, I would actually prefer if you instantiate that char array only if it's really needed. Can't we do:
const char16_t* directiveName[] = { mCurDir[0].get() };
right before you log to the console?

@@ +949,5 @@
> +  if (!(aDir->hasType(nsIContentPolicy::TYPE_STYLESHEET))
> +      && !(aDir->hasType(nsIContentPolicy::TYPE_SCRIPT))) {
> +      logWarningErrorToConsole(nsIScriptError::warningFlag, "ignoringDirectiveWithNoValues",
> +                               directiveName, ArrayLength(directiveName));
> +  } else {

if (!(aDir->hasType(nsIContentPolicy::TYPE_STYLESHEET)) &&
    !(aDir->hasType(nsIContentPolicy::TYPE_SCRIPT))) {

  logWarning(...);
  return;
}
mPolicy->addDirective(aDir);

::: dom/webidl/CSPDictionaries.webidl
@@ +28,4 @@
>    sequence<DOMString> upgrade-insecure-requests;
>    sequence<DOMString> child-src;
>    sequence<DOMString> block-all-mixed-content;
> +  sequence<DOMString> require-sri-for;

Ah, sorry, I forgot about that, you have to put that line into a separate patch and ask someone (e.g. bholly) to sign off on it, otherwise you can't land webidl changes. Make sure it has the appropriate name of the reviewer in the commit message.

::: netwerk/base/LoadInfo.cpp
@@ +148,4 @@
>      }
>    }
>  
> +    // If the CSP has the directive to require SRI, set this here

If CSP requires SRI (require-sri-for), then store that information in the loadInfo so we can enforce SRI before loading the subresource.

@@ +156,5 @@
> +      if (aLoadingPrincipal) {
> +        aLoadingPrincipal->GetCsp(getter_AddRefs(csp));
> +        // csp could be null if loading principal is system principal
> +        if (csp) {
> +          uint32_t loadType = nsContentUtils::InternalContentPolicyTypeToExternal(aContentPolicyType);

Nit: 80 char line limit.
Attachment #8758327 - Flags: review?(ckerschb) → review+
(Assignee)

Comment 21

a year ago
Created attachment 8758386 [details] [diff] [review]
0001-Bug-1265318-add-require-sri-for-CSP-directive-r-cker.patch

carrying over r+, addressing nits.
Attachment #8758327 - Attachment is obsolete: true
Attachment #8758386 - Flags: review+
(Assignee)

Comment 22

a year ago
Created attachment 8758387 [details] [diff] [review]
webidl changs

Moving Webidl change into its own patch
(Assignee)

Updated

a year ago
Attachment #8758387 - Flags: review?(mrbkap)
Attachment #8758387 - Flags: review?(mrbkap) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed
(Assignee)

Comment 23

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a0aac8db2df
(Assignee)

Comment 24

a year ago
The try push was successful, as expected.
I just went through all the intermittent oranges and pinned them accordingly.
Please someone check in :-)

Comment 25

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3dce332e6d22
tests for require-sri-for CSP directive. r=ckerschb
https://hg.mozilla.org/integration/mozilla-inbound/rev/b38b4072fd41
add require-sri-for CSP directive. r=ckerschb
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec9c63bd09fc
webidl changes only, r=mrbkap
Keywords: checkin-needed
(Assignee)

Updated

a year ago
Depends on: 1277248
(Assignee)

Updated

a year ago
Depends on: 1277495

Comment 26

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3dce332e6d22
https://hg.mozilla.org/mozilla-central/rev/b38b4072fd41
https://hg.mozilla.org/mozilla-central/rev/ec9c63bd09fc
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Assignee)

Updated

a year ago
Depends on: 1277557
Depends on: 1279139
Depends on: 1279420

Updated

a year ago
Depends on: 1280179
(Assignee)

Updated

8 months ago
Depends on: 1312680
Keywords: dev-doc-needed
https://developer.mozilla.org/en-US/Firefox/Releases/49#HTTP
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/require-sri-for
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.