Devdatta Akhawe [:devd] Reporter
	Description • 10 years ago

The SRI specification "defines a mechanism by which user agents may verify that a fetched resource has been delivered without unexpected manipulation." We should implement this too.
http://w3c.github.io/webappsec/specs/subresourceintegrity/

	Frederik Braun [:freddy]
	Comment 1 • 10 years ago

This is slightly related to previous work on link fingerprints, adding a connection through "See Also" for convenience

	Doug Turner (:dougt)
	Comment 2 • 10 years ago

doesn't do anything but some basic plumbing for the script element.

	Doug Turner (:dougt)
	Comment 4 • 10 years ago

Hm. This approach doesn't handle preload very well.  While do a preload, the script element may not be available. In this siutation, we can not determine if there is an integrity attribute.  So, we need to do something a bit different.

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 10 years ago

Preload has access to attributes (obviously).  It should just be passing the attribute value in, along with the other ones it already passes in.

	Doug Turner (:dougt)
	Comment 6 • 10 years ago

bz, right -- just need to teach the parser about it.

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

This is looking quite good.
Doug, did you see the tests I provided in bug 1074136?
I can trim them to be as concise as yours, but I think we should make sure to keep a test for a correct content-type to avoid spoofing, shouldn't we?

	Devdatta Akhawe [:devd] Reporter
	Comment 11 • 10 years ago

@freddyb: More generally, maybe that should be a separate bug: check for correct content-type if CSP is enabled. There are some trivial CSP bypasses without this https://bugzilla.mozilla.org/show_bug.cgi?id=722547 and so this is a broader problem, imho.

	Doug Turner (:dougt)
	Comment 12 • 10 years ago

Yeah, we need moar tests.  this was just a flight hack and I am handing it off now.

	François Marier [:francois] Assignee
	Comment 13 • 10 years ago

Here's a rebased version of Doug's patch that updates the URI format and makes all of the tests pass.

	Frederik Braun [:freddy]
	Comment 14 • 10 years ago

(In reply to Devdatta Akhawe [:devd] from comment #11)
> @freddyb: More generally, maybe that should be a separate bug: check for
> correct content-type if CSP is enabled. There are some trivial CSP bypasses
> without this https://bugzilla.mozilla.org/show_bug.cgi?id=722547 and so this
> is a broader problem, imho.

Can you please elaborate? Bug 722547 talks about E4X, which we have dropped support for.

	Devdatta Akhawe [:devd] Reporter
	Comment 15 • 10 years ago

The concern is more broad than E4X: if attacker can upload some content and then include <script src="foobar.png"> and it works as a script, that is enough to bypass CSP controls, right? It works for any file type as long as attacker can control it and browser is willing to execute it as a script.

	François Marier [:francois] Assignee
	Comment 16 • 10 years ago

Comment on attachment 8499848 [details] [diff] [review]
Teach the parser about in integrity (java)

The parser changes have been moved to bug 1100206.

	François Marier [:francois] Assignee
	Comment 17 • 10 years ago

Steve, the part of this patch I'd like feedback on is this chunk of nsScriptLoader.cpp:

+  nsCOMPtr<nsIHttpChannel> httpChannel;
+  { // TODO: is there an easier way to get to the channel?
+    nsCOMPtr<nsIRequest> req;
+    nsresult rv = aLoader->GetRequest(getter_AddRefs(req));
+    NS_ASSERTION(req, "StreamLoader's request went away prematurely");
+    NS_ENSURE_SUCCESS(rv, rv);
+    httpChannel = do_QueryInterface(req);
+  }

The only reason I want to get to the channel here is to get a hold of the response headers.

	François Marier [:francois] Assignee
	Comment 18 • 10 years ago

Steve, in nsScriptLoader.cpp:IsEligible(), I've got to determine whether or not the script we're loading is publicly cacheable:

+  // TODO: check whether or not the resource is publicly cacheable

Is there an easier way than manually parsing the headers and applying all of the usual caching rules?

Here's the part of the SRI spec which requires this check: https://w3c.github.io/webappsec/specs/subresourceintegrity/#is-varresourcevar-eligible-for-integrity-validation

It refers to this definition of what a publicly cacheable resource is: http://tools.ietf.org/html/rfc7234#section-3

	Steve Workman [:sworkman] (INACTIVE)
	Comment 19 • 10 years ago

Didn't have time to get into this in detail, sorry, and I don't see a nice simple api in nsIHttpChannel to do the work here. But I have a couple of thoughts:

-- It looks like you need to iterated over the headers for WWW-Authenticate and Refresh anyway, no?
-- I think you can use nsIHttpChannel.isNoStoreResponse and .isNoCacheResponse - both should be false.

I'm leaving the needinfo as a reminder. In the meantime (i.e. during the Thanksgiving break), I'm also flagging Honza (yay Europe!) because I think he can answer this much more authoritatively than I can :)

	François Marier [:francois] Assignee
	Comment 20 • 10 years ago

Steve, the feedback request is only for the 8 lines of code contained in comment 17.

	François Marier [:francois] Assignee
	Comment 22 • 10 years ago

Here's the test page I use.

	Honza Bambas (:mayhemer)
	Comment 24 • 10 years ago

Comment on attachment 8531407 [details] [diff] [review]
WIP implementation

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

::: dom/base/nsScriptLoader.cpp
@@ +1725,5 @@
> +{
> +  if (aChannel) {
> +    nsAutoCString refresh, wwwAuth;
> +    aChannel->GetResponseHeader(NS_LITERAL_CSTRING("Refresh"), refresh);
> +    aChannel->GetResponseHeader(NS_LITERAL_CSTRING("WWW-Authenticate"), wwwAuth);

better (correct, actually) way is to:

nsresult rv = aChannel->GetResponseHeader(..);
if (NS_SUCCEEDED(rv)) {
  // the header *is* on the response
}

Checking for WWW-Authenticate is a non-sense.  It will never pop-up here.  Only when credentials are not provided and the channel just propagates the 401 response in which case the load fails anyway.

You should rather check for the "Authorization" request header.

@@ +1760,5 @@
> +  if (aRequest->mCORSMode != CORS_NONE) {
> +    return true;
> +  }
> +
> +  // TODO: check whether or not the resource is publicly cacheable

isNoCacheResponse is not the correct way here..  isNoStoreResponse *is* tho.

The spec says "If resource is cachable by a shared cache."  I'm not that deep expert to the referred RFC, but I'd say after some time spent reading it, you want to avoid responses with Cache-control: private (or no-store) response header or Authorization request header.

You will have to parse the Cache-control response header yourself or we could add isPrivateRespnse to nsIHttpChannel beside isNoStore, I would not be against.

	François Marier [:francois] Assignee
	Comment 25 • 10 years ago

This patch takes into account Honza's comments in comment 24.

I've opened bug 1113004 to add a new nsIHttpChannel.IsPrivateResponse().

	François Marier [:francois] Assignee
	Comment 26 • 10 years ago

Comment on attachment 8531674 [details] [diff] [review]
Parser changes (generated)

The parser changes have been merged onto mozilla-central: https://hg.mozilla.org/mozilla-central/rev/7b7cbab2c5e3

	François Marier [:francois] Assignee
	Comment 27 • 10 years ago

Chris, I've moved all of the code into dom/security/ as you suggested. I'm not sure exactly what to do about your other suggestion though (creating an IDL for it), but I thought I'd ask for your feedback on the C++ class I created and how I'm using it in nsScriptLoader.cpp.

Other than that, the implementation is complete and fully working with e10s disabled. I still need to find a hashing function that will work in content with e10s enabled.

	Christoph Kerschbaumer [:ckerschb]
	Comment 28 • 10 years ago

Comment on attachment 8539207 [details] [diff] [review]
WIP implementation

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

Francois, overall looks really good. Haven't looked at any of the tests yet. Since there are so many files in that patch, maybe you can separate all the test files and provide a separate patch for the testcase.

The other thing I am not really sure (but maybe it's just me) is that just using 'integrity' as variable/function/method name seems confusing. It's clear what the intention is, but maybe we can rename. So, e.g. instead of ParseIntegrityValue we could use ParseSRIValue.

It's quite some comments, but mostly nits, I am happy to review/provide feedback again whenever you have an updated version.

(In reply to François Marier [:francois] from comment #27)
> Chris, I've moved all of the code into dom/security/ as you suggested. I'm
> not sure exactly what to do about your other suggestion though (creating an
> IDL for it), but I thought I'd ask for your feedback on the C++ class I
> created and how I'm using it in nsScriptLoader.cpp.

So, the reason I suggested having an xpcom object is that it can be cycle-collected. What I've seen in the code, you allocate the SRI object on the stack. If there is no use case where we have to keep the SRI object alive, then obviously not having to create a fully blown xpcom object is the better strategy.

::: dom/base/Element.h
@@ +945,5 @@
>     * but if not should have been parsed via ParseCORSValue).
>     */
>    static CORSMode AttrValueToCORSMode(const nsAttrValue* aValue);
>  
> +  static void ParseIntegrityValue(const nsAString& aValue,

Nit: Add a comment on top what this function is doing.

::: dom/base/nsIScriptElement.h
@@ +240,5 @@
>     */
>    virtual nsresult FireErrorEvent() = 0;
>  
> +  /**
> +   *  Integrity

Nit: Expand Comment.

::: dom/base/nsScriptLoader.cpp
@@ +62,5 @@
> +GetSriLog()
> +{
> +  static PRLogModuleInfo *gSriPRLog;
> +  if (!gSriPRLog)
> +    gSriPRLog = PR_NewLogModule("SRI");

Nit: surround if-clause with curly braces.

::: dom/locales/en-US/chrome/security/security.properties
@@ +19,5 @@
>  LoadingMixedDisplayContent=Loading mixed (insecure) display content on a secure page "%1$S"
>  # LOCALIZATION NOTE: Do not translate "allow-scripts", "allow-same-origin", "sandbox" or "iframe"
>  BothAllowScriptsAndSameOriginPresent=An iframe which has both allow-scripts and allow-same-origin for its sandbox attribute can remove its sandboxing.
>  
> +# Sub-Resource Integrity

At the moment we are refactoring console messages for CORS and also CSP. Probably it makes sense to let folks from devtools have a look at those messages. For me they seem ok. Maybe ping sworkman on IRC and ask him as well. As I said, messages seem descriptive enough for me, but since SRI is a new feature they might have some input for us.

::: dom/security/nsSubResourceIntegrity.cpp
@@ +38,5 @@
> +SRICheck::NormalizeHashAlgorithm(nsAString& aMetadata)
> +{
> +  nsContentUtils::ASCIIToLower(aMetadata);
> +  NS_NAMED_LITERAL_STRING(shaPrefix, "sha-");
> +  if (StringBeginsWith(aMetadata, shaPrefix)) {

No need to declare a named literal on top, you can simplify to:
if (StringBeginsWith(aMetadata, NS_LITERAL_STRING("sha-")) {

Also, since this method is so short, probably worth adding a comment and just inline into the code.

@@ +45,5 @@
> +}
> +
> +bool
> +SRICheck::IsWhitelistedScheme(nsIURI* aRequestURI)
> +{

Are you expecting more schemes that need to be whitelisted in the future? At the moment I only see one call to that function, maybe it's worth inlining. You could add a comment and simplify to:

bool isAboutScheme =
      (NS_SUCCEEDED(aRequestURI->SchemeIs("about", &isAboutScheme)) && isAboutScheme);

@@ +77,5 @@
> +    }
> +  }
> +
> +  if (rankB == -1) {
> +    const char16_t* params[] = { PromiseFlatString(b).get() };

I see here an error check for rankB, which means 'b' contains some bogus value, can 'a' also contain a bogus value? What if 'a' ends up being -1 at the end of the loop?

@@ +96,5 @@
> +                            nsAString& aMetadataAlg)
> +{
> +  aMetadata = EmptyString();
> +  aMetadataAlg = EmptyString();
> +

Please add more documentation to this function to make it as readable as possible.

@@ +99,5 @@
> +  aMetadataAlg = EmptyString();
> +
> +  nsWhitespaceTokenizer tokenizer(aMetadataList);
> +  while (tokenizer.hasMoreTokens()) {
> +    nsAutoString token(tokenizer.nextToken());

Can we move nsAutoString token above the loop and just overwrite it's contents inside the while loop?

@@ +102,5 @@
> +  while (tokenizer.hasMoreTokens()) {
> +    nsAutoString token(tokenizer.nextToken());
> +
> +    // make sure the token can be parsed as a valid URI
> +    nsCOMPtr<nsIURI> uri;

Please also move the decl of uri outside the loop. Or even better, you could create some helper functions, e.g.

if (!generateURIFromToken(token, uri)) {
  continue;
}

@@ +131,5 @@
> +    }
> +
> +    // remove the prefix from the metadata string
> +    NS_NAMED_LITERAL_STRING(integrityPrefix, "ni:///");
> +    token.Cut(0, integrityPrefix.Length());

Do you really need to create a literal string to call Length() on it, probably worth adding a comment and just use a hardcoded integer value.

@@ +179,5 @@
> +  while (tokenizer.hasMoreTokens()) {
> +    nsAutoString token(tokenizer.nextToken());
> +    if (StringHead(token, 3).LowerCaseEqualsLiteral("ct=")) {
> +      token.Cut(0, 3);
> +      nsAutoCString contentType;

Please try to define string variables outside the loop and just overwrite their contents within the loop.

@@ +226,5 @@
> +  if (aChannel) {
> +    nsAutoCString refValue, authValue;
> +    NS_NAMED_LITERAL_CSTRING(refHeader, "Refresh");
> +    NS_NAMED_LITERAL_CSTRING(authHeader, "Authorization");
> +    nsresult hasRefresh = aChannel->GetResponseHeader(refHeader, refValue);

As before, no need to declare a NAMED_LITERAL, just use aChannel->GetResponseHeader(refValue, NS_LITERAL_STRING("Refresh))

@@ +277,5 @@
> +  }
> +
> +  if (aChannel) {
> +    nsAutoCString corsValue;
> +    NS_NAMED_LITERAL_CSTRING(corsHeader, "Access-Control-Allow-Origin");

Is that String not already defined somewhere and we could re-use?

@@ +310,5 @@
> +
> +    // Cache-Control: no-store
> +    bool isNoStoreResponse = false;
> +    aChannel->IsNoStoreResponse(&isNoStoreResponse);
> +    if (isNoStoreResponse) {

You can simplify to:
bool isNoStore =
  (NS_SUCCEEDED(aChannel->IsNoStoreResponse(&isNoStore) && isNoStore));

@@ +443,5 @@
> +  if (IsWhitelistedScheme(aRequestURI)) {
> +    return true; // whitelisted schemes always pass
> +  }
> +
> +  if (!Preferences::GetBool("security.subResourceIntegrity.enable", true)) {

Move that check to the beginning of the method. Also no need to query that bool all the time, you can cache that result in the constructor of SRI, see e.g.
http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPService.cpp#35

@@ +471,5 @@
> +  {
> +    nsCOMPtr<nsIRequest> req;
> +    aLoader->GetRequest(getter_AddRefs(req));
> +    httpChannel = do_QueryInterface(req);
> +  }

Am I missing something or is that code that you forgot to take out - Doesn't 'reg' run out of scope at the end of the closing curly brace?

::: dom/security/nsSubResourceIntegrity.h
@@ +25,5 @@
> +                       uint32_t aStringLen,
> +                       const uint8_t* aString);
> +
> +private:
> +  nsIDocument* mDocument;

I think the convention is that member variables are declared at the end of the class declaration. It's a nit though. More important, are you probably going to run in danger since mDocument is not an nsCOMPtr? Probably not, but maybe worth double checking.

@@ +42,5 @@
> +  /**
> +   * Returns whether or now "a" is a weaker hash algorithm then "b".
> +   */
> +  bool IsWeakerAlg(const nsAString& a, const nsAString& b);
> +

Nits: typo 'now' vs 'not' in comment. IsWearkAlg doesn't sound very descriptive to me, maybe we can come up with something better. Also, please don't use just 'a' and 'b' as argument names, maybe aHashAlgo1 and aHashAlgo2. Would it make sense to not let this function return bool, but rather the stronger algo?

@@ +49,5 @@
> +   * return the strongest supported hash.
> +   */
> +  nsresult IntegrityMetadata(const nsAString& aMetadataList,
> +                             nsAString& aMetadata,
> +                             nsAString& aMetadataAlg);

Is there a reason this returns nsresult? Mabye we can also use the strongest supported hash as the return value here.

@@ +57,5 @@
> +   * does not match the content type of the sub-resource to be
> +   * loaded. If the content type is not specified in the query string,
> +   * return true.
> +   */
> +  bool IsContentType(nsIHttpChannel* aChannel, const nsAString& aQueryString);

Nit: Can we also rename this method as well?

@@ +63,5 @@
> +    /**
> +   * Returns whether or not the sub-resource about to be loaded is eligible
> +   * for integrity checks. If it's not, the checks will be skipped and the
> +   * sub-resource will be loaded.
> +   */

Nit: Indentation of comment start /**

@@ +71,5 @@
> +  /**
> +   * Compute the hash of a sub-resource and compare it with the expected
> +   * value.
> +   */
> +  bool VerifyHash(const nsACString& hash, const nsAString& alg,

Please be consistent with leading 'a' for arguments.

	François Marier [:francois] Assignee
	Comment 30 • 9 years ago

Chris, thanks so much for your thorough comments! I've refactored this patch
quite and have hopefully addressed most of the issues you raised.

Here are the points I did _not_ address in this new patch:

> @@ +277,5 @@
> > +  }
> > +
> > +  if (aChannel) {
> > +    nsAutoCString corsValue;
> > +    NS_NAMED_LITERAL_CSTRING(corsHeader, "Access-Control-Allow-Origin");
> 
> Is that String not already defined somewhere and we could re-use?

I couldn't find one anywhere, everybody seems to be hard-coding the string:

  http://dxr.mozilla.org/mozilla-central/search?q=Access-Control-Allow-Origin

> @@ +443,5 @@
> > +  if (IsWhitelistedScheme(aRequestURI)) {
> > +    return true; // whitelisted schemes always pass
> > +  }
> > +
> > +  if (!Preferences::GetBool("security.subResourceIntegrity.enable", true)) {
> 
> Move that check to the beginning of the method. Also no need to query that
> bool all the time, you can cache that result in the constructor of SRI, see
> e.g. http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPService.cpp#35

I tried that but it turns out the same approach doesn't work since SRICheck
is currently not a singleton and its constructor is called multiple times.

> @@ +471,5 @@
> > +  {
> > +    nsCOMPtr<nsIRequest> req;
> > +    aLoader->GetRequest(getter_AddRefs(req));
> > +    httpChannel = do_QueryInterface(req);
> > +  }
> 
> Am I missing something or is that code that you forgot to take out - Doesn't
> 'reg' run out of scope at the end of the closing curly brace?

What you're missing is the line just before the start of the scope where I
declare httpChannel. I put req in a short-lived scope because I don't use it
anywhere, I only use it to get to the channel.

> @@ +57,5 @@
> > +   * does not match the content type of the sub-resource to be
> > +   * loaded. If the content type is not specified in the query string,
> > +   * return true.
> > +   */
> > +  bool IsContentType(nsIHttpChannel* aChannel, const nsAString& aQueryString);
> 
> Nit: Can we also rename this method as well?

Any suggestions? :)

	Christoph Kerschbaumer [:ckerschb]
	Comment 31 • 9 years ago

Comment on attachment 8542520 [details] [diff] [review]
Code changes

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

Hey Francois, thanks for addressing all my nits and also answering my questions. Looks good to me, since you changed so much in the overall structure I am also fine with if you keep 'IsContentType()' as a method name.

The one thing I am not sure about: What NS_LossyConvertUTF16toASCII() does exactly, I am more familiar with NS_ConvertUTF16toUTF8(), would that be an alternative? We just have to many string manipulation helpers, I can't keep track anymore.

Anyway, find some nits underneath, but overall this looks good to me! Ready for review by a peer!

::: dom/base/nsScriptLoader.cpp
@@ +658,5 @@
>  
>      if (!request) {
>        // no usable preload
> +      request = new nsScriptLoadRequest(aElement, version, ourCORSMode,
> +                                        integrity);

Nit: could all fit on one line, right?

::: dom/security/nsSRICheck.cpp
@@ +249,5 @@
> +    SRILOG(("nsSRICheck::IsEligible, same-origin"));
> +#endif
> +    return true;
> +  } else  {
> +#ifdef PR_LOGGING

Since you only use the else branch for logging - you could also move the 'else { ... }' inside the #ifdef.

@@ +279,5 @@
> +      return true;
> +    }
> +  } else {
> +#ifdef PR_LOGGING
> +    SRILOG(("nsSRICheck::IsEligible, no channel, can't check for CORS or caching headers"));

Same here, put the whole 'else' inside the ifdef'ed part.

@@ +314,5 @@
> +    return false;
> +  }
> +
> +  unsigned hashLength;
> +  short hashAlg;

uint32_t for hashLength and int16_t for the hashAlg, or is there a particular reason you use unisnged and short?

@@ +329,5 @@
> +  nsresult rv;
> +  nsCOMPtr<nsICryptoHash> cryptoHash;
> +  cryptoHash = do_CreateInstance("@mozilla.org/security/hash;1", &rv);
> +  MOZ_ASSERT(NS_SUCCEEDED(rv)); // TODO: make this work with e10s (bug 1114882)
> +  rv = cryptoHash->Init(hashAlg);

You could slightly clean that up:

nsCOMPtr<nsICryptoHash> cryptoHash =
  do_CreateInstance("@mozilla.org/security/hash;1", &rv);
NS_ENSURE_SUCCESS(rv, false); // TODO: make this work with e10s (bug 1114882)
rv = cryptoHash->Init(hashAlg);
NS_ENSURE_SUCCESS(rv, false);

unless you want to log to the console or do PR_LOGGING.

@@ +397,5 @@
> +#ifdef PR_LOGGING
> +    SRILOG(("nsSRICheck::VerifyIntegrity, sri is disabled (pref)"));
> +#endif
> +    return true;
> +  }

Mhm, I still don't like that code here :-(

What if you do in the constructor:

A::A() {
  static bool isPrefCached = false;
  if (!isPrefChached) {
    Preferences::AddBoolVarCache(&sSRIEnabled, "security.subResourceIntegrity.enable");
    isPrefCached = true;
  }
}
or something similar - every change in about:config needs a restart anyway, so something like that should work.
Please also move that check at the method entry, no need to do any operations if SRI is preffed off.

::: dom/security/nsSRICheck.h
@@ +11,5 @@
> +
> +class SRICheck
> +{
> +public:
> +  explicit SRICheck(nsCOMPtr<nsIDocument> aDocument);

Since mDocument is declared as comPtr, you can just pass a nsIDocument* (the raw pointer) instead of an nsCOMPtr. Your code works correctly as well, but your code performs an increment and decrement on the refcount of the document, which causes additonal overhead :-)

@@ +39,5 @@
> +   * loaded. If the content type is not specified in the query string,
> +   * return true.
> +   */
> +  bool IsContentType(nsIHttpChannel* aChannel,
> +                     const nsAString& aExpectedType) const;

super nit: since you all the *Is*Methods pass the channel as the second argument, maybe you wanna make it consistent and also pass the channel as the second argument here.

::: dom/security/nsSRIMetadata.cpp
@@ +50,5 @@
> +  int32_t questionMark = aToken.FindChar('?');
> +
> +  // split the token into its components
> +  MOZ_ASSERT(semicolon >= 6); // "ni:///" prefix enforced elsewhere
> +  mAlgorithm = Substring(aToken, 6, semicolon - 6);

The MOZ_ASSERT disappears in release mode - what if the semicolon appears at index 4? Is that possible? Wouldn't that case undefined behavior? Probably worth adding a return statement or any other kind of error handling!

@@ +52,5 @@
> +  // split the token into its components
> +  MOZ_ASSERT(semicolon >= 6); // "ni:///" prefix enforced elsewhere
> +  mAlgorithm = Substring(aToken, 6, semicolon - 6);
> +  uint32_t hashStart = semicolon + 1;
> +  if (questionMark == -1) {

I am not sure if the following suggestion works, but probably worth a try. The way I see this is, you have something like:
ni:///sha-256;C6CB9UYIS9UJeqinPHWTHVqh_E1uhG5Twh-Y5qFQmYg?ct=application/javascript, right?

So, if the token turns into a valid URI, you can also make use of the operations already defined on nsIURI, right?
The functions defined on nsIURI are already well tested and maybe less error prone: see
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIURI.idl#16

@@ +72,5 @@
> +  if (StringBeginsWith(mAlgorithm, NS_LITERAL_STRING("sha-"))) {
> +    mAlgorithm.Cut(3, 1); // remove the dash
> +  }
> +
> +  SetAlgorithmStrength();

Nit: small comment on top what SetAlrogithmStrength() is doing.

@@ +88,5 @@
> +{
> +  const char16_t* supportedAlgorithms[] = { MOZ_UTF16("sha256"),
> +                                            MOZ_UTF16("sha384"),
> +                                            MOZ_UTF16("sha512") };
> +

If it's not any of the three algos, then mAlgorithm remains -1, right? Maybe worth noting here.

::: dom/security/nsSRIMetadata.h
@@ +33,5 @@
> +  bool IsAlgorithmSupported() const { return (mAlgorithmStrength != -1); }
> +
> +  void GetHash(nsACString& aHash) const;
> +  void GetAlgorithm(nsAString& aAlgorithm) const;
> +  void GetHashType(short& aAlgorithm, unsigned& aLength) const;

Please use int16_t and uint32_t, or is there a particular reason?

@@ +40,5 @@
> +private:
> +  void SetAlgorithmStrength();
> +
> +  nsAutoString mHash;
> +  nsAutoString mAlgorithm;

Maybe its worth defining enums for sha256, and all the other algos, then you don't have to perform the expensive string comparison in SRIMetadata::GetHashType(), or even better, you could reuse nsICryptoHash::SHA256 and such, then you would only have to perform a int-comparison.

@@ +42,5 @@
> +
> +  nsAutoString mHash;
> +  nsAutoString mAlgorithm;
> +  int8_t mAlgorithmStrength; // higher is stronger, -1 if not supported
> +  nsAutoString mQueryString;

Mega nit: align all member variables.

	François Marier [:francois] Assignee
	Comment 32 • 9 years ago

Thanks again for your comments Chris.

> ::: dom/base/nsScriptLoader.cpp
> @@ +658,5 @@
> >  
> >      if (!request) {
> >        // no usable preload
> > +      request = new nsScriptLoadRequest(aElement, version, ourCORSMode,
> > +                                        integrity);
> 
> Nit: could all fit on one line, right?

Yes, but it would exceed the 80-char limit I saw on
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Line_Length

> @@ +314,5 @@
> > +    return false;
> > +  }
> > +
> > +  unsigned hashLength;
> > +  short hashAlg;
> 
> uint32_t for hashLength and int16_t for the hashAlg, or is there a
> particular reason you use unisnged and short?

I wanted to match the types of the nsICryptoHash constants I'm using:

http://dxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsICryptoHash.idl?from=nsICryptoHash.idl#25

but I've changed the length one to a uint32_t because it's just an untyped #define:

http://dxr.mozilla.org/mozilla-central/source/security/nss/lib/util/hasht.h#37

> every change in about:config needs a restart anyway, so something like that
> should work.

What exactly did you mean by that? Because that's certainly not true in the
general case. For example, you can toggle `network.http.referer.spoofSource`
without restarting anything and it takes effect immediately on the next page
load.

> Please also move that check at the method entry, no need to do
> any operations if SRI is preffed off.

I've moved the isAboutScheme check after the prefs, however, I want to keep
the check for empty integrity attributes before then so that we exit early
in the most common case of all.

> The MOZ_ASSERT disappears in release mode - what if the semicolon appears at
> index 4? Is that possible? Wouldn't that case undefined behavior? Probably
> worth adding a return statement or any other kind of error handling!

That comment is misleading, because the prefix is not enforced elsewhere,
it's enforced just a few lines above :) So it's not possible unless there's
a bug in the existing error handling in that constructor.

> So, if the token turns into a valid URI, you can also make use of the
> operations already defined on nsIURI, right?  The functions defined on
> nsIURI are already well tested and maybe less error prone: see
> http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIURI.idl#16

That's what I tried initially but then everything of interest was ending up
in the single component (I think it was "hostname") so I had to parse
everything myself. The only thing I was able to use this parser for was to
remove the scheme, so it didn't seem worth it.

	François Marier [:francois] Assignee
	Comment 33 • 9 years ago

Sid, I'd appreciate if you could also take a look at the way I've put the SRI patch together and give me any comments you have, nits and all :)

	Christoph Kerschbaumer [:ckerschb]
	Comment 34 • 9 years ago

(In reply to François Marier [:francois] from comment #32)
> > every change in about:config needs a restart anyway, so something like that
> > should work.
> 
> What exactly did you mean by that? Because that's certainly not true in the
> general case. For example, you can toggle `network.http.referer.spoofSource`
> without restarting anything and it takes effect immediately on the next page
> load.

Oh, is that the case? Is that also true for SRI if you change the pref from false to true? If so, then my suggestion does not work. What I want to avoid is, that we have to query the boolPref for SRI every single time we load a script. There must be a way to cache that.

> > Please also move that check at the method entry, no need to do
> > any operations if SRI is preffed off.
> 
> I've moved the isAboutScheme check after the prefs, however, I want to keep
> the check for empty integrity attributes before then so that we exit early
> in the most common case of all.

I get your point. You are optimizing for the most common case. I have a different point of view - no need to perform any task at all if SRI is prefed off. Since it's prefed on most of the time, I think you can leave what you have.

> > The MOZ_ASSERT disappears in release mode - what if the semicolon appears at
> > index 4? Is that possible? Wouldn't that case undefined behavior? Probably
> > worth adding a return statement or any other kind of error handling!
> 
> That comment is misleading, because the prefix is not enforced elsewhere,
> it's enforced just a few lines above :) So it's not possible unless there's
> a bug in the existing error handling in that constructor.

So why having the MOZ_ASSERT() at all then?

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 35 • 9 years ago

Comment on attachment 8542805 [details] [diff] [review]
Code changes

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

::: dom/locales/en-US/chrome/security/security.properties
@@ +21,5 @@
>  BothAllowScriptsAndSameOriginPresent=An iframe which has both allow-scripts and allow-same-origin for its sandbox attribute can remove its sandboxing.
>  
> +# Sub-Resource Integrity
> +# LOCALIZATION NOTE: Do not translate "script" or "integrity"
> +InvalidIntegrityURI=The script tag has an invalid URI in its integrity attribute: "%1$S"

s/tag/element/.

::: dom/security/moz.build
@@ +9,5 @@
>      'nsCSPService.h',
>      'nsCSPUtils.h',
>      'nsMixedContentBlocker.h',
> +    'nsSRICheck.h',
> +    'nsSRIMetadata.h',

Don't follow the poor example set here; new code should be namespaced and *not* have the ns* prefix.

::: dom/security/nsSRICheck.cpp
@@ +295,5 @@
> +}
> +
> +bool
> +SRICheck::VerifyHash(const SRIMetadata& aMetadata,
> +                     uint32_t aStringLen, const uint8_t* aString) const

Why not const nsACString&?

@@ +330,5 @@
> +  nsCOMPtr<nsICryptoHash> cryptoHash =
> +    do_CreateInstance("@mozilla.org/security/hash;1", &rv);
> +  if (NS_FAILED(rv)) {
> +    MOZ_ASSERT(false); // TODO: remove once it works with e10s (bug 1114882)
> +    NS_ASSERTION(false, "Could not initialize nsICryptoHash.");

Don't introduce new NS_ASSERTIONs.

@@ +376,5 @@
> +#endif
> +
> +  if (aIntegrity.IsEmpty()) {
> +#ifdef PR_LOGGING
> +      SRILOG(("nsSRICheck::VerifyIntegrity, no integrity attribute"));

Indentation is off

::: dom/security/nsSRICheck.h
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef nsSRICheck_h___

mozilla_dom_SRICheck_h

@@ +8,5 @@
> +
> +#include "nsSRIMetadata.h"
> +#include "mozilla/CORSMode.h"
> +
> +class SRICheck

Never ever expose random names in the global namespace.

::: dom/security/nsSRIMetadata.cpp
@@ +101,5 @@
> +  aAlgorithm = mAlgorithm;
> +}
> +
> +void
> +SRIMetadata::GetHashType(short& aType, uint32_t& aLength) const

`short` is not used in Mozilla code. Use stdint types. Also use pointers, not references, for outparams; references make the callers unreadable.

::: dom/security/nsSRIMetadata.h
@@ +28,5 @@
> +   */
> +  inline bool operator<(const SRIMetadata& aOther) const
> +  {
> +    MOZ_ASSERT(nsICryptoHash::SHA256 < nsICryptoHash::SHA384);
> +    MOZ_ASSERT(nsICryptoHash::SHA384 < nsICryptoHash::SHA512);

Should be able to static_assert those.

@@ +33,5 @@
> +    return (mAlgorithmType < aOther.mAlgorithmType);
> +  }
> +
> +  bool IsValid() const { return (!mHash.IsEmpty() && !mAlgorithm.IsEmpty()); }
> +  bool IsAlgorithmSupported() const { return (mAlgorithmType != -1); }

Don't overparenthesise.

@@ +43,5 @@
> +
> +private:
> +  nsAutoString mHash;
> +  nsAutoString mAlgorithm;
> +  short        mAlgorithmType;

No short, no alignment, probably no nsAutoString in fields, and sort the fields by size to avoid extraneous padding.

::: dom/svg/SVGScriptElement.cpp
@@ +232,5 @@
>  
> +void
> +SVGScriptElement::GetIntegrity(nsAString& aIntegrity)
> +{
> +  // Sub-resource integrity is not supported in SVG scripts

Then you'd better not expose the attribute.

	François Marier [:francois] Assignee
	Comment 36 • 9 years ago

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #34)
> (In reply to François Marier [:francois] from comment #32)
> > What exactly did you mean by that? Because that's certainly not true in the
> > general case. For example, you can toggle `network.http.referer.spoofSource`
> > without restarting anything and it takes effect immediately on the next page
> > load.
> 
> Oh, is that the case? Is that also true for SRI if you change the pref from
> false to true? If so, then my suggestion does not work. What I want to avoid
> is, that we have to query the boolPref for SRI every single time we load a
> script. There must be a way to cache that.

Yes, that works fine for SRI. You can toggle on and off between page loads.

Ideally the pref should be checked once per page and that SRI should be either ON or OFF for all of the scripts included in that page. I'm not sure where or how to do that though.

> > That comment is misleading, because the prefix is not enforced elsewhere,
> > it's enforced just a few lines above :) So it's not possible unless there's
> > a bug in the existing error handling in that constructor.
> 
> So why having the MOZ_ASSERT() at all then?

I was being perhaps a little too cautious :) I'll remove it.

	François Marier [:francois] Assignee
	Comment 37 • 9 years ago

Thanks so much for your comments Ms2ger! It's not always easy to distinguish the legacy style that should be ignored and the conventions that are still current :)

I've addressed almost all of your comments in this revised patch and I've also re-read the coding style guidelines and fixed a few other violations.

(In reply to :Ms2ger from comment #35)
> ::: dom/security/nsSRICheck.cpp
> @@ +295,5 @@
> > +}
> > +
> > +bool
> > +SRICheck::VerifyHash(const SRIMetadata& aMetadata,
> > +                     uint32_t aStringLen, const uint8_t* aString) const
> 
> Why not const nsACString&?

The uint8_t and uint32_t come from nsScriptLoader::OnStreamComplete() and
then get consumed by nsICryptoHash::Update(). I thought it would be wasteful
to convert this to an ns*String and then back.

> @@ +43,5 @@
> > +
> > +private:
> > +  nsAutoString mHash;
> > +  nsAutoString mAlgorithm;
> > +  short        mAlgorithmType;
> 
> No short, no alignment, probably no nsAutoString in fields, and sort the
> fields by size to avoid extraneous padding.

Would you recommend an nsString instead?

Can you expand on what you mean by "extraneous padding" and how I should
sort the fields?

	Sid Stamm [:geekboy or :sstamm]
	Comment 38 • 9 years ago

Comment on attachment 8543138 [details] [diff] [review]
Code changes

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

::: dom/base/nsScriptLoader.cpp
@@ +1448,5 @@
> +  if (sri.VerifyIntegrity(request->mIntegrity, request->mURI,
> +                          request->mCORSMode, aLoader, aStringLen, aString)) {
> +    rv = PrepareLoadedRequest(request, aLoader, aStatus, aStringLen, aString);
> +  }
> +

This doesn't seem to fully implement the script tag logic mentioned in the draft:
http://w3c.github.io/webappsec/specs/subresourceintegrity/#the-script-element-1

Maybe I'm missing something, but it's not checking for the document's integrity policy (block vs report).  Is this intentional?

The draft spec also says to augment step 14 of the HTML5 spec (That's done in ProcessScriptElement: http://mxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#588).  Is there areason you're doing this here and not in step 14?

I also don't see where NS_ERROR_CORRUPTED_CONTENT causes an error to fire if the blocked content is same-origin with the document.  I think it happens, but it's not clear where the same-origin check is happening.  The link above states: "If resource is same origin with the link element’s Document’s origin, then queue a task to fire a simple event..." (Yes, "link element" is mentioned in the "script element" section, probably an editor typo, please have the SRI draft fixed here.)

::: dom/security/SRICheck.cpp
@@ +444,5 @@
> +  }
> +
> +  nsAutoString expectedType;
> +  metadata.GetContentType(&expectedType);
> +  if (httpChannel && !IsContentType(expectedType, httpChannel)) {

I'm pretty sure this is the only place you use SRICheck::IsContentType.  You could just inline the check here instead of making a subroutine.  If you have a good reason to break it out into a separate subroutine, please call it something like "ChannelMatchesContentType" since it's specific to an HTTP channel.

Please do something similar for IsCorsEnabled and IsPubliclyCacheable, which are also only used once each.

::: dom/security/SRICheck.h
@@ +41,5 @@
> +   * Parse the multiple hashes specified in the integrity attribute and
> +   * return the strongest supported hash.
> +   */
> +  void IntegrityMetadata(const nsAString& aMetadataList,
> +                         SRIMetadata* aMetadata) const;

Why is this here and not in a SRIMetadata.h as a constructor or factory method (e.g., SRIMetadata::CreateSRIMetadataWithStrongestSupportedHash() or SRIMetadata::FromIntegrityAttribute())?

If aMetadata is a return argument, please make it obvious (either mention it in the doc comment, or call it outMetadata).

::: modules/libpref/init/all.js
@@ +1806,5 @@
>  pref("security.mixed_content.block_active_content", false);
>  pref("security.mixed_content.block_display_content", false);
>  
> +pref("security.subResourceIntegrity.enable", true);
> +pref("security.subResourceIntegrity.requireSecure", false);

Please put a comment here that explains what requireSecure means.

	François Marier [:francois] Assignee
	Comment 39 • 9 years ago

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #38)
> This doesn't seem to fully implement the script tag logic mentioned in the
> draft:
> http://w3c.github.io/webappsec/specs/subresourceintegrity/#the-script-
> element-1
> 
> Maybe I'm missing something, but it's not checking for the document's
> integrity policy (block vs report).  Is this intentional?

Yes, I've intentionally ignored the reporting stuff. I will file a follow-up bug once the basic support lands.

> The draft spec also says to augment step 14 of the HTML5 spec (That's done
> in ProcessScriptElement:
> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.
> cpp#588).  Is there areason you're doing this here and not in step 14?

So the spec says:

  "When executing step 5 of step 14 of HTML5’s “prepare a script” algorithm:

     1. Set the integrity metadata of the request to the value of the element’s integrity attribute."

which is done here: https://bitbucket.org/fmarier/mozilla-central-mq-992096/src/bf7a69b2f1ee1111e41307f36cef65a4591a9011/bug992096.patch?at=default#cl-206

Then it says:

  "Insert the following steps after step 5 of step 14 of HTML5’s “prepare a script” algorithm:

     6. Once the fetching algorithm has completed:

       1. If the response’s integrity state is corrupt:
         1. If the document’s integrity policy is block:
           1. If resource is same origin with the link element’s Document’s origin, then queue
              a task to fire a simple event named error at the element, and abort these steps.
         2. Report a violation."

which is done in the OnStreamComplete callback: https://bitbucket.org/fmarier/mozilla-central-mq-992096/src/bf7a69b2f1ee1111e41307f36cef65a4591a9011/bug992096.patch?at=default#cl-272

> I also don't see where NS_ERROR_CORRUPTED_CONTENT causes an error to fire if
> the blocked content is same-origin with the document.  I think it happens,
> but it's not clear where the same-origin check is happening.  The link above
> states: "If resource is same origin with the link element’s Document’s
> origin, then queue a task to fire a simple event..."

You're right, the error always fire I think, which is a bug I need to fix!

> ::: dom/security/SRICheck.h
> @@ +41,5 @@
> > +   * Parse the multiple hashes specified in the integrity attribute and
> > +   * return the strongest supported hash.
> > +   */
> > +  void IntegrityMetadata(const nsAString& aMetadataList,
> > +                         SRIMetadata* aMetadata) const;
> 
> Why is this here and not in a SRIMetadata.h as a constructor or factory
> method (e.g., SRIMetadata::CreateSRIMetadataWithStrongestSupportedHash() or
> SRIMetadata::FromIntegrityAttribute())?

I was thinking of SRIMetadata as a lightweight class that does minimal string parsing and is used to compare metadata items together (to pick the strongest one). IntegrityMetadata() does some parsing but also issues a number of devtools warnings and errors (and therefore needs to have a pointer to the document) while doing so.

I'm totally open to an alternative design however.

	François Marier [:francois] Assignee
	Comment 47 • 9 years ago

Sid, this should fix the bug you found. We now only trigger onerror for same-origin scripts.

	François Marier [:francois] Assignee
	Comment 48 • 9 years ago

Updated for the new hash formats and now supports stylesheets.

	François Marier [:francois] Assignee
	Comment 49 • 9 years ago

Updated tests for the new hash formats. Stylesheet tests are missing.

	François Marier [:francois] Assignee
	Comment 51 • 9 years ago

Updated patch that fully complies with the current spec.

	François Marier [:francois] Assignee
	Comment 52 • 9 years ago

Updated tests which include stylesheet support.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 53 • 9 years ago

(In reply to François Marier [:francois] from comment #37)
> > @@ +43,5 @@
> > > +
> > > +private:
> > > +  nsAutoString mHash;
> > > +  nsAutoString mAlgorithm;
> > > +  short        mAlgorithmType;
> > 
> > No short, no alignment, probably no nsAutoString in fields, and sort the
> > fields by size to avoid extraneous padding.
> 
> Would you recommend an nsString instead?

Yeah.

> Can you expand on what you mean by "extraneous padding" and how I should
> sort the fields?

Sort them from large to small; if you have fields

x: u8,
y: u8,
z: usize,

your struct will be laid out on 32-bit as

 0 -   8  x
 8 -  16  y
16 -  32  padding
32 -  64  z

and on 64-bit as

 0 -   8  x
 8 -  16  y
16 -  64  padding
64 - 128  z

while ordering them z - x - y would not need this padding.

	François Marier [:francois] Assignee
	Comment 54 • 9 years ago

Bug 992096 - Implement Sub Resource Integrity [2/2]

Mochitests

	François Marier [:francois] Assignee
	Comment 55 • 9 years ago

Bug 992096 - Implement Sub Resource Integrity [1/2]

Code changes

	François Marier [:francois] Assignee
	Comment 56 • 9 years ago

Ehsan, if you're happy to review this, the spec that it implements is here:

  https://w3c.github.io/webappsec/specs/subresourceintegrity/

The spec is still changing, but I'm one of the editors and I will do follow-up commits to address any changes as they come up.

	(no longer active)
	Comment 57 • 9 years ago

How soon do you need this?  This is a pretty big review and I'm very busy these days, it may take me a few weeks to be able to get to it....  Is that OK?

	François Marier [:francois] Assignee
	Comment 58 • 9 years ago

If it takes a couple of weeks, that's totally fine, it's not urgent and I've got some work to do on the spec in the meantime.

Also it there's anything you'd like me to do (e.g. breaking up the patch) ahead of time, don't hesitate.

	François Marier [:francois] Assignee
	Comment 59 • 9 years ago

Comment on attachment 8612204 [details]
MozReview Request: Bug 992096 - Implement Sub Resource Integrity [2/2]. r=ckerschb

Bug 992096 - Implement Sub Resource Integrity [2/2]

Mochitests

	François Marier [:francois] Assignee
	Comment 60 • 9 years ago

Comment on attachment 8612205 [details]
MozReview Request: Bug 992096 - Implement Sub Resource Integrity [1/2]. r=baku,r=ckerschb

Bug 992096 - Implement Sub Resource Integrity [1/2]

Code changes

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 61 • 9 years ago

WPT has subresource-integrity tests, so we should use and extend those rather than writing new mochitests.

	François Marier [:francois] Assignee
	Comment 62 • 9 years ago

> WPT has subresource-integrity tests, so we should use and extend those rather than writing new mochitests.

Yes, I'm working on fixing those: https://github.com/w3c/web-platform-tests/pull/1853

Once they're in a good shape (they don't currently match the spec) I'll replace the mochitests with them.

	(no longer active)
	Comment 63 • 9 years ago

Andrea, do you feel comfortable taking these reviews from me?  I'm way too overloaded these days, and hate to keep Francois waiting...  :-)

	(no longer active)
	Comment 64 • 9 years ago

Comment on attachment 8612204 [details]
MozReview Request: Bug 992096 - Implement Sub Resource Integrity [2/2]. r=ckerschb

Andrea said yes offline...

	François Marier [:francois] Assignee
	Comment 65 • 9 years ago

Comment on attachment 8612204 [details]
MozReview Request: Bug 992096 - Implement Sub Resource Integrity [2/2]. r=ckerschb

Bug 992096 - Implement Sub Resource Integrity [2/2]

Mochitests

	François Marier [:francois] Assignee
	Comment 66 • 9 years ago

Comment on attachment 8612205 [details]
MozReview Request: Bug 992096 - Implement Sub Resource Integrity [1/2]. r=baku,r=ckerschb

Bug 992096 - Implement Sub Resource Integrity [1/2]

Code changes

	Andrea Marchesini [:baku]
	Comment 67 • 9 years ago

Comment on attachment 8612205 [details]
MozReview Request: Bug 992096 - Implement Sub Resource Integrity [1/2]. r=baku,r=ckerschb

https://reviewboard.mozilla.org/r/9535/#review11127

It looks very good. But I cannot review all these changes. In case they didn't do it yet, I would like to have somebody from the security team to review this patch.

::: dom/base/nsContentSink.cpp:762
(Diff revision 3)
> +  aElement->GetAttr(kNameSpaceID_None, nsGkAtoms::integrity, integrity);

This can fail. Check the return value.

::: dom/base/nsIDocument.h:2018
(Diff revision 3)
> +                            const nsAString& aIntegrity) = 0;

change NS_IDOCUMENT_IID

::: dom/base/nsScriptLoader.cpp:625
(Diff revision 3)
> +        scriptContent->GetAttr(kNameSpaceID_None, nsGkAtoms::integrity,

return value.

::: dom/base/nsStyleLinkElement.cpp:426
(Diff revision 3)
> +    thisContent->GetAttr(kNameSpaceID_None, nsGkAtoms::integrity, integrity);

here too?

::: dom/locales/en-US/chrome/security/security.properties:42
(Diff revision 3)
> +IntegrityMismatch=None of the '%1$S' hashes in the integrity attribute match the content of the sub-resource.

LOCALIZATION NOTE for each line where you use 'integrity'

::: dom/security/SRICheck.h:23
(Diff revision 3)
> +class SRICheck

final?

::: dom/security/SRICheck.cpp:44
(Diff revision 3)
> +{

MOZ_ASSERT(aRequestURI);
MOZ_ASSERT(aDocument);

::: dom/security/SRICheck.cpp:46
(Diff revision 3)
> +  aRequestURI->GetSpec(requestSpec);

This can fail.

::: dom/security/SRICheck.cpp:61
(Diff revision 3)
> +  } else if (MOZ_LOG_TEST(GetSriLog(), mozilla::LogLevel::Debug)) {

no 'else' after a 'return'.

::: dom/security/SRICheck.cpp:63
(Diff revision 3)
> +    aDocument->GetDocumentURI()->GetAsciiSpec(documentURI);

This can fail.

::: dom/security/SRICheck.cpp:85
(Diff revision 3)
> +{

MOZ_ASSERT(aString);
MOZ_ASSERT(aDocument);

::: dom/security/SRICheck.cpp:91
(Diff revision 3)
> +  if (NS_FAILED(Base64Decode(base64Hash, binaryHash))) {

NS_WARN_IF

::: dom/security/SRICheck.cpp:115
(Diff revision 3)
> +  if (NS_FAILED(rv)) {

NS_WARN_IF

::: dom/security/SRICheck.cpp:120
(Diff revision 3)
> +  if (NS_FAILED(cryptoHash->Update(aString, aStringLen))) {

NS_WARN_IF

::: dom/security/SRICheck.cpp:119
(Diff revision 3)
> +  NS_ENSURE_SUCCESS(cryptoHash->Init(hashType), false);

rv = cryptoHash->Init(hashType)
if (NS_WARN_IF(NS_FAILED(rv)) {
  return false;
}

::: dom/security/SRICheck.cpp:126
(Diff revision 3)
> +  cryptoHash->Finish(false, computedHash);

can this fail?

::: dom/security/SRICheck.cpp:140
(Diff revision 3)
> +{

MOZ_ASSERT(outMetadata);
MOZ_ASSERT(aDocument);

::: dom/security/SRICheck.cpp:238
(Diff revision 3)
> +    aRequestURI->GetAsciiSpec(requestURL);

This can fail.

::: dom/security/SRICheck.h:41
(Diff revision 3)
> +  static bool VerifyIntegrity(const SRIMetadata& aMetadata,

I prefer to have a nsresult instead a boolean. This will allow us to propagate the error correctly.

::: dom/security/SRIMetadata.h:16
(Diff revision 3)
> +class SRIMetadata

final

	Andrea Marchesini [:baku]
	Comment 68 • 9 years ago

Comment on attachment 8612204 [details]
MozReview Request: Bug 992096 - Implement Sub Resource Integrity [2/2]. r=ckerschb

https://reviewboard.mozilla.org/r/9533/#review11131

Ship It!

	Boris Zbarsky [:bzbarsky]
	Comment 69 • 9 years ago

> This can fail. Check the return value.

Is there a behavior difference between no "integrity" attribute and integrity=""?  If not, then no need to check GetAttr.  But if there is, then yes, you need to check it.

	François Marier [:francois] Assignee
	Comment 70 • 9 years ago

Comment on attachment 8612204 [details]
MozReview Request: Bug 992096 - Implement Sub Resource Integrity [2/2]. r=ckerschb

Bug 992096 - Implement Sub Resource Integrity [2/2]. r?ckerschb

Mochitests

	François Marier [:francois] Assignee
	Comment 71 • 9 years ago

Comment on attachment 8612205 [details]
MozReview Request: Bug 992096 - Implement Sub Resource Integrity [1/2]. r=baku,r=ckerschb

Bug 992096 - Implement Sub Resource Integrity [1/2]. r=baku,r?ckerschb

Code changes

	François Marier [:francois] Assignee
	Comment 72 • 9 years ago

Thanks Andrea for the thorough review and Boris for your comment. I think I've addressed everything in the latest revision of my patch.

Christoph, would you be willing to do a final review of it from a SecEng point of view? I know you've seen that patch a few times already, but it has also changed a lot over the last few months :)

	Andrea Marchesini [:baku]
	Comment 73 • 9 years ago

Comment on attachment 8612205 [details]
MozReview Request: Bug 992096 - Implement Sub Resource Integrity [1/2]. r=baku,r=ckerschb

https://reviewboard.mozilla.org/r/9535/#review11729

Ship It!

	Christoph Kerschbaumer [:ckerschb]
	Comment 74 • 9 years ago

Comment on attachment 8612205 [details]
MozReview Request: Bug 992096 - Implement Sub Resource Integrity [1/2]. r=baku,r=ckerschb

https://reviewboard.mozilla.org/r/9535/#review11757

Hi Francois, looks pretty good. Would be great if you could incorporate my suggestions and answer my questions. Once you are ready - re-flag me. I really want to see that landed. Great stuff!!

::: dom/security/SRICheck.h:35
(Diff revision 4)
> +                                nsIDocument* aDocument);

Nit: Could you you re-arrange the arguments so that the last argument is the outgoing argument?

::: dom/security/SRICheck.h:45
(Diff revision 4)
> +                                  nsIDocument* aDocument);

Any reason why nsIURI and nsIDocument are not const here?

::: dom/security/SRICheck.h:56
(Diff revision 4)
> +                                  nsIDocument* aDocument);

same here

::: dom/security/SRICheck.cpp:98
(Diff revision 4)
> +  MOZ_ASSERT(aDocument);

same here, please use NS_ENSURE_ARG(aString) otherwise you might run in danger of dereferencing a null ptr.

::: dom/security/SRICheck.cpp:162
(Diff revision 4)
> +  MOZ_ASSERT(outMetadata->IsEmpty()); // caller must pass empty metadata

why can't you use a reference for outMetaData instead of a pointer? If you really need a pointer, please use
NS_ENSURE_ARG(outMetaData);
same for aDocument

::: dom/base/nsScriptLoader.cpp:636
(Diff revision 4)
> +                                        sriMetadata);

Do you expect SRIMetadata to be extended? You are handing off a variable from the stack to nsScriptLoadRequest, where C++'s internal copy constructor copies the values into mIntegrity. It's fine at the moment because you use primitives and nsCString performs the right task.

But potentially we run into problems in the future. Just wanna make sure we are not missing anything - but looks good as is.

::: dom/security/SRIMetadata.h:24
(Diff revision 4)
> +  SRIMetadata() : mAlgorithmType(-1), mEmpty(true) { }

nit: could you define a variable
ALGORITHM_NOT_INITIALISED or something and use that instead of -1 here and elsewhere?

::: dom/security/SRIMetadata.h:55
(Diff revision 4)
> +  void GetAlgorithm(nsCString* outAlg) const { *outAlg = mAlgorithm; }

Does the nsCstring need to be a pointer? Can we use a reference instead?

::: dom/security/SRIMetadata.h:54
(Diff revision 4)
> +  void GetHash(uint32_t aIndex, nsCString* outHash) const;

Does the nsCstring need to be a pointer? Can we use a reference instead?

::: dom/security/SRIMetadata.cpp:30
(Diff revision 4)
> +SRIMetadata::SRIMetadata(const nsACString& aToken)

please use the initalization list
SRIMetadata:SRIMetadata(const nsACString& aToken)
: mAlgorithmType(-1)
, mEmpty(false)

::: dom/security/SRIMetadata.cpp:33
(Diff revision 4)
> +  mEmpty = false;

So, random idea: what if you rename mEmpty to mIntialized and set it at the very end of the constructor to indicate whehter initalization was successful or not. At the moment it seems whenver you encounter an error within the constructor of SRIMetaData you are in a somehow inconsistent state which is hard to track. Would that make sense?

Probably makes also sense to then rename .Empty() to .IsInitalized().

::: dom/security/SRIMetadata.cpp:72
(Diff revision 4)
> +  } else {

no need to reset that value - it will remain -1 if you not overwrite it, because you set it to -1 in the initialization list.

::: dom/security/SRIMetadata.cpp:139
(Diff revision 4)
> +  MOZ_ASSERT(aIndex < mHashes.Length());

danger - please do not use MOZ_ASSERT but actually return in case you are trying to index out of bounds.

if (aIndex >= mHashes.Length()) {
  *outHash = nullptr;
  return;
}

::: dom/security/SRIMetadata.cpp:145
(Diff revision 4)
> +{

Nit: you could use NS_ENSURE_ARG for the two incoming arguments.

::: layout/style/CSSStyleSheet.h:130
(Diff revision 4)
> +                dom::SRIMetadata aIntegrity);

const dom::SRIMetadata& aIntegrity

::: layout/style/CSSStyleSheet.cpp:819
(Diff revision 4)
> +                                       SRIMetadata aIntegrity)

For nsScriptLoadRequest you use the following signature:
const mozilla::dom::SRIMetadata &aIntegrity
Can we use the same signature here as well and safe one copy by value?

::: layout/style/CSSStyleSheet.cpp:1102
(Diff revision 4)
> +    mRuleProcessors(nullptr)

Nit: CSSStyleSheetInner::CSSStyleSheetInner uses commas in the front of the initialization list, I prefer that too, but up to you.

::: dom/security/SRICheck.cpp:47
(Diff revision 4)
> +  MOZ_ASSERT(aRequestURI);

MOZ_ASSERT is probably not that great here, I think what you want is
NS_ENSURE_ARG(aRequestURI);
NS_ENSURE_ARG(aDocument);

::: dom/security/SRIMetadata.h:36
(Diff revision 4)
> +  bool operator<(const SRIMetadata& aOther) const;

Please please also overload the greater than operator. People see that the less than operator is used, so they start using the greater than operator which then turns into undefined behavior - thanks!

	François Marier [:francois] Assignee
	Comment 75 • 9 years ago

Comment on attachment 8612204 [details]
MozReview Request: Bug 992096 - Implement Sub Resource Integrity [2/2]. r=ckerschb

Bug 992096 - Implement Sub Resource Integrity [2/2]. r?ckerschb

Mochitests

	François Marier [:francois] Assignee
	Comment 76 • 9 years ago

Comment on attachment 8612205 [details]
MozReview Request: Bug 992096 - Implement Sub Resource Integrity [1/2]. r=baku,r=ckerschb

Bug 992096 - Implement Sub Resource Integrity [1/2]. r=baku,r?ckerschb

Code changes

	François Marier [:francois] Assignee
	Comment 77 • 9 years ago

Sorry Andrea, despite my marking the commit as r=baku, MozReview is resetting your review flag whenever I push a new revision.

Note: I've disabled SRI by default in this revision because there is an unresolved issue in the spec.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #74)
> ::: dom/security/SRICheck.h:45
> (Diff revision 4)
> > +                                  nsIDocument* aDocument);
> 
> Any reason why nsIURI and nsIDocument are not const here?

I've changed nsIDocument. For nsIURI, we need it because GetSpec is not
const.

> ::: dom/security/SRICheck.cpp:162
> (Diff revision 4)
> > +  MOZ_ASSERT(outMetadata->IsEmpty()); // caller must pass empty metadata
> 
> why can't you use a reference for outMetaData instead of a pointer? If you
> really need a pointer, please use
> NS_ENSURE_ARG(outMetaData);
> same for aDocument

Maybe I misunderstood this part of the coding guidelines, but the last item
here:

  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#COM_and_pointers

"Use pointers instead of references for function out parameters, even for
primitive types."

> ::: dom/security/SRIMetadata.h:55
> (Diff revision 4)
> > +  void GetAlgorithm(nsCString* outAlg) const { *outAlg = mAlgorithm; }
> 
> Does the nsCstring need to be a pointer? Can we use a reference instead?
>
> ::: dom/security/SRIMetadata.h:54
> (Diff revision 4)
> > +  void GetHash(uint32_t aIndex, nsCString* outHash) const;
> 
> Does the nsCstring need to be a pointer? Can we use a reference instead?

Same as the previous comment.

> ::: dom/security/SRIMetadata.cpp:33
> (Diff revision 4)
> > +  mEmpty = false;
> 
> So, random idea: what if you rename mEmpty to mIntialized and set it at the
> very end of the constructor to indicate whehter initalization was successful
> or not. At the moment it seems whenver you encounter an error within the
> constructor of SRIMetaData you are in a somehow inconsistent state which is
> hard to track. Would that make sense?
> 
> Probably makes also sense to then rename .Empty() to .IsInitalized().

Actually in this case, it does mean empty as in 'integrity=""'. We treat
invalid metadata differently from empty ones.

> ::: dom/security/SRICheck.cpp:47
> (Diff revision 4)
> > +  MOZ_ASSERT(aRequestURI);
> 
> MOZ_ASSERT is probably not that great here, I think what you want is
> NS_ENSURE_ARG(aRequestURI);
> NS_ENSURE_ARG(aDocument);

It seems like something is seriously wrong if the request object in script
loader doesn't have a valid mURI (or mDocument) member.

	Andrea Marchesini [:baku]
	Comment 78 • 9 years ago

Comment on attachment 8612205 [details]
MozReview Request: Bug 992096 - Implement Sub Resource Integrity [1/2]. r=baku,r=ckerschb

https://reviewboard.mozilla.org/r/9535/#review11851

Ship It!

::: dom/security/SRICheck.cpp:130
(Diff revisions 4 - 5)
> -  if (NS_WARN_IF(NS_FAILED(rv))) {
> +  NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);

NS_ENSURE_SUCCESS(rv, rv);
and above.

	Christoph Kerschbaumer [:ckerschb]
	Comment 79 • 9 years ago

(In reply to François Marier [:francois] from comment #77)
> > ::: dom/security/SRICheck.cpp:162
> > (Diff revision 4)
> > > +  MOZ_ASSERT(outMetadata->IsEmpty()); // caller must pass empty metadata
> > 
> > why can't you use a reference for outMetaData instead of a pointer? If you
> > really need a pointer, please use
> > NS_ENSURE_ARG(outMetaData);
> > same for aDocument
> 
> Maybe I misunderstood this part of the coding guidelines, but the last item
> here:
> 
>  
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Coding_Style#COM_and_pointers
> 
> "Use pointers instead of references for function out parameters, even for
> primitive types."

Oh ok, fair enough. I don't completely agree, but that sounds reasonable. Thanks for the clarification.

> > ::: dom/security/SRICheck.cpp:47
> > (Diff revision 4)
> > > +  MOZ_ASSERT(aRequestURI);
> > 
> > MOZ_ASSERT is probably not that great here, I think what you want is
> > NS_ENSURE_ARG(aRequestURI);
> > NS_ENSURE_ARG(aDocument);
> 
> It seems like something is seriously wrong if the request object in script
> loader doesn't have a valid mURI (or mDocument) member.

Sure, but what happens in release builds once you hit aRequestURI->GetSpec()? Wouldn't you deref a null-pointer? Please use NS_ENSURE_ARG() which would cause an early return rather than a browser crash in case aRequestURI is a nullptr.

	Christoph Kerschbaumer [:ckerschb]
	Comment 80 • 9 years ago

Comment on attachment 8612205 [details]
MozReview Request: Bug 992096 - Implement Sub Resource Integrity [1/2]. r=baku,r=ckerschb

https://reviewboard.mozilla.org/r/9535/#review12019

r+, but please make sure to update MOZ_ASSERT to NS_ENSURE_ARG for all the pointer arguments within your function bodies. Otherwise you run in danger of derefing a nullptr. Nice work :-)

::: dom/locales/en-US/chrome/security/security.properties:43
(Diff revision 5)
> +IntegrityMismatch=None of the '%1$S' hashes in the integrity attribute match the content of the sub-resource.

Nit: everywhere else you use double quotes instead of single quotes - please be consistent throughout security.properties.

::: dom/security/SRICheck.cpp:48
(Diff revision 5)
> +  MOZ_ASSERT(aDocument);

As already mentioend, please use NS_ENSURE_ARG. Otherwise you might deref a nullptr in release builds.

::: dom/security/SRICheck.cpp:130
(Diff revision 5)
> +  NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);

Here and underneath you can use
NS_ENSURE_SUCCESS(rv, rv);

::: dom/security/SRICheck.cpp:154
(Diff revision 5)
> +  MOZ_ASSERT(aDocument);

again MOZ_ASSERT only terminates a debug build. Please use NS_ENSURE_ARG which returns early if handed a nullptr as argument instead of crashing on first access in case of a nullptr.

::: dom/security/SRIMetadata.cpp:107
(Diff revision 5)
> +  return false;

Why don't you wanna implement it? But I am just nitpicky.

	Christoph Kerschbaumer [:ckerschb]
	Comment 81 • 9 years ago

Comment on attachment 8612204 [details]
MozReview Request: Bug 992096 - Implement Sub Resource Integrity [2/2]. r=ckerschb

https://reviewboard.mozilla.org/r/9533/#review12031

So, before I am going to mark this as an r+, I have one last question: What about redirects? In the past we had quite so many issues with redirects for mixed content as well as for CSP. Any chance you wanna write a 302 redirect case at least? (Using a *.sjs file makes it very easy to write one). Other than that I think tests look great.

::: dom/security/test/sri/script_401.js:1
(Diff revision 5)
> +var load=true;

You have quite soo many files, e.g. script_401.js including the ^header^ files. You don't have to, but woulnd't it be nicer to use *one* *.sjs file instead?

You could send different request to that *.sjs file and depending on the queryString you could branch and return different headers and content. But as I said, it's up to you. I would prefer the *.sjs solution though :-)

	Frederik Braun [:freddy]
	Comment 82 • 9 years ago

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #80)
> ::: dom/locales/en-US/chrome/security/security.properties:43
> (Diff revision 5)
> > +IntegrityMismatch=None of the '%1$S' hashes in the integrity attribute match the content of the sub-resource.
> 
> Nit: everywhere else you use double quotes instead of single quotes - please
> be consistent throughout security.properties.
> 

I have another nit :-P, spec is called "subresource" integrity, message says "sub-resource".

	François Marier [:francois] Assignee
	Comment 83 • 9 years ago

Comment on attachment 8612204 [details]
MozReview Request: Bug 992096 - Implement Sub Resource Integrity [2/2]. r=ckerschb

Bug 992096 - Implement Sub Resource Integrity [2/2]. r?ckerschb

Mochitests

	François Marier [:francois] Assignee
	Comment 84 • 9 years ago

Comment on attachment 8612205 [details]
MozReview Request: Bug 992096 - Implement Sub Resource Integrity [1/2]. r=baku,r=ckerschb

Bug 992096 - Implement Sub Resource Integrity [1/2]. r=baku,r=ckerschb

Code changes

	François Marier [:francois] Assignee
	Comment 85 • 9 years ago

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #81)
> ::: dom/security/test/sri/script_401.js:1
> (Diff revision 5)
> > +var load=true;
> 
> You have quite soo many files, e.g. script_401.js including the ^header^
> files. You don't have to, but woulnd't it be nicer to use *one* *.sjs file
> instead?
> 
> You could send different request to that *.sjs file and depending on the
> queryString you could branch and return different headers and content. But
> as I said, it's up to you. I would prefer the *.sjs solution though :-)

That's a good idea. I might wait until the next SRI patch though (we're changing a small part of the spec) because that will involve writing more tests. Also, I might be able to get rid of many of the tests once we merge the w3c web platform ones too.

Other than that, I think I've addressed all of the review feedback in my last revision, including adding a redirect test case.

	François Marier [:francois] Assignee
	Comment 86 • 9 years ago

Comment on attachment 8612204 [details]
MozReview Request: Bug 992096 - Implement Sub Resource Integrity [2/2]. r=ckerschb

Bug 992096 - Implement Sub Resource Integrity [2/2]. r=ckerschb

Mochitests

	François Marier [:francois] Assignee
	Comment 87 • 9 years ago

Comment on attachment 8612205 [details]
MozReview Request: Bug 992096 - Implement Sub Resource Integrity [1/2]. r=baku,r=ckerschb

Bug 992096 - Implement Sub Resource Integrity [1/2]. r=baku,r=ckerschb

Code changes

	François Marier [:francois] Assignee
	Comment 88 • 9 years ago

Comment on attachment 8612205 [details]
MozReview Request: Bug 992096 - Implement Sub Resource Integrity [1/2]. r=baku,r=ckerschb

Carrying previous r+ from Christoph

	François Marier [:francois] Assignee
	Comment 89 • 9 years ago

Comment on attachment 8612204 [details]
MozReview Request: Bug 992096 - Implement Sub Resource Integrity [2/2]. r=ckerschb

Carrying previous r+ from Andrea and Christoph.

	Pulsebot
	Comment 90 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/bab5913ea6cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/948b2e9d1fa2

	Ryan VanderMeulen [:RyanVM]
	Comment 91 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/bab5913ea6cb
https://hg.mozilla.org/mozilla-central/rev/948b2e9d1fa2

	Florian Bender
	Comment 92 • 9 years ago

Release Note Request (optional, but appreciated)
[Why is this notable]: ultimately more security & stability for end users on websites
[Suggested wording]: Implemented method for websites to enforce integrity of external content
[Links (documentation, blog post, etc)]: (waiting on MDN / dev-doc-needed)

	Stefan Plewako [:stef]
	Comment 93 • 9 years ago

> https://hg.mozilla.org/mozilla-central/diff/bab5913ea6cb/dom/locales/en-US/chrome/security/security.properties

What will replace all those %1$S?

	François Marier [:francois] Assignee
	Comment 94 • 9 years ago

(In reply to Stefan Plewako [:stef] from comment #93)
> > https://hg.mozilla.org/mozilla-central/diff/bab5913ea6cb/dom/locales/en-US/chrome/security/security.properties
> 
> What will replace all those %1$S?

It's different for each string.

> MalformedIntegrityURI=The script element has a malformed URI in its
> integrity attribute: "%1$S". The correct format is "<hash algorithm>-<hash
> value>".

Here %1$S is the invalid token found in the integrity attribute. For
example:

  integrity="garbage sha256-..."

will output:

  The script element has a malformed URI in its integrity attribute: "garbage". The correct format is "<hash algorithm>-<hash value>".

> IntegrityMismatch=None of the "%1$S" hashes in the integrity attribute
> match the content of the subresource.

Here %1$S is the type of hash algorigthm in use. For example,

  integrity="sha256-... sha256-..."

will output:

  None of the "sha256" hashes in the integrity attribute match the content of the subresource.

> IneligibleResource="%1$S" is not eligible for integrity checks since it's
> neither CORS-enabled nor same-origin.

Here %1$S is the full URL of the sub-resource that cannot be protected using
SRI.

> UnsupportedHashAlg=Unsupported hash algorithm in the integrity attribute:
> "%1$S"

Here %1$S is the invalid hash algorithm found in the integrity attribute. For
example:

  integrity="rot13-... sha256-..."

will output:

  Unsupported hash algorithm in the integrity attribute: "rot13"

Is there a good way to encode these explanations into the locale file so that translators will see them?

	Boris Zbarsky [:bzbarsky]
	Comment 95 • 9 years ago

> Is there a good way to encode these explanations into the locale file so that translators
> will see them?

Yes, the LOCALIZATION NOTE comments.  See the one for LoadingMixedActiveContent2 in the same file, for example.

	Jean-Yves Perrier [:teoli]
	Comment 96 • 9 years ago

Sideshowbarker made a very nice documentation for this feature! (Many thanks here!)
https://developer.mozilla.org/en-US/docs/Web/Security/Subresource_Integrity
https://developer.mozilla.org/en/docs/Web/HTML/Element/link#attr-integrity
https://developer.mozilla.org/en/docs/Web/HTML/Element/script#attr-integrity

I've added a note in:
https://developer.mozilla.org/en-US/Firefox/Releases/43#Security

	François Marier [:francois] Assignee
	Comment 97 • 9 years ago

(In reply to Jean-Yves Perrier [:teoli] from comment #96)
> I've added a note in:
> https://developer.mozilla.org/en-US/Firefox/Releases/43#Security

That note is confusing to me because "Subresource integrity has been implemented for <script> and <link> that doesn't link to stylesheets" seems to imply that <link rel="stylesheet"> is not supported whereas we _only_ support <link> elements that refer to stylesheets.

	(not reading bugmail) Nick Desaulniers [:\n]
	Comment 98 • 9 years ago

Removing devrel-needed due to https://hacks.mozilla.org/2015/09/subresource-integrity-in-firefox-43/