Frederik Braun [:freddy] Assignee
	Description • 8 years ago

This should be relatively simple, using the `enforceSRI` plumbing introduced in bug 1235572.

	Frederik Braun [:freddy] Assignee
	Comment 1 • 8 years ago

This has moved from the CSP3 spec to SRI, changing the title to reflect that.

	Christoph Kerschbaumer [:ckerschb, back on May 21st]
	Comment 5 • 8 years ago

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.

	Christoph Kerschbaumer [:ckerschb, back on May 21st]
	Comment 6 • 8 years ago

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.

	Christoph Kerschbaumer [:ckerschb, back on May 21st]
	Comment 7 • 8 years ago

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.

	Frederik Braun [:freddy] Assignee
	Comment 8 • 8 years ago

New iteration.
This is Part 1, for all the CSP changes and the tests

	Frederik Braun [:freddy] Assignee
	Comment 9 • 8 years ago

This is part 2, which makes LoadInfo look into the current CSP.

	Frederik Braun [:freddy] Assignee
	Comment 10 • 8 years ago

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

	Christoph Kerschbaumer [:ckerschb, back on May 21st]
	Comment 11 • 8 years ago

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

	Christoph Kerschbaumer [:ckerschb, back on May 21st]
	Comment 12 • 8 years ago

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?

	Christoph Kerschbaumer [:ckerschb, back on May 21st]
	Comment 15 • 8 years ago

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

	Christoph Kerschbaumer [:ckerschb, back on May 21st]
	Comment 16 • 8 years ago

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?

	Frederik Braun [:freddy] Assignee
	Comment 17 • 8 years ago

addressed test nits. carrying over r+

	Frederik Braun [:freddy] Assignee
	Comment 18 • 8 years ago

Thank you for the speedy reviews.
and another round :)

	Frederik Braun [:freddy] Assignee
	Comment 19 • 8 years ago

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

	Christoph Kerschbaumer [:ckerschb, back on May 21st]
	Comment 20 • 8 years ago

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.

	Frederik Braun [:freddy] Assignee
	Comment 21 • 8 years ago

carrying over r+, addressing nits.

	Frederik Braun [:freddy] Assignee
	Comment 22 • 8 years ago

Moving Webidl change into its own patch

	Frederik Braun [:freddy] Assignee
	Comment 23 • 8 years ago

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

	Frederik Braun [:freddy] Assignee
	Comment 24 • 8 years ago

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

	Pulsebot
	Comment 25 • 8 years 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

	Carsten Book [:Tomcat]
	Comment 26 • 8 years 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

	Florian Scholz (Open Web Docs)
	Comment 27 • 8 years ago

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