Implement Subresource Integrity

RESOLVED FIXED in Firefox 43

Status

()

Core
DOM: Security
--
enhancement
RESOLVED FIXED
3 years ago
22 hours ago

People

(Reporter: devd, Assigned: francois)

Tracking

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

unspecified
mozilla43
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox43 fixed, relnote-firefox 43+)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 29 obsolete attachments)

39 bytes, text/x-review-board-request
francois
: review+
francois
: review+
Details | Review
39 bytes, text/x-review-board-request
francois
: review+
Details | Review
(Reporter)

Description

3 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/
This is slightly related to previous work on link fingerprints, adding a connection through "See Also" for convenience
See Also: → bug 292481

Updated

3 years ago
Assignee: nobody → dougt

Comment 2

3 years ago
Created attachment 8495600 [details] [diff] [review]
wip

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

Comment 3

3 years ago
Created attachment 8496320 [details] [diff] [review]
wip
Attachment #8495600 - Attachment is obsolete: true

Updated

3 years ago
Attachment #8496320 - Flags: feedback?(bzbarsky)

Comment 4

3 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.

Updated

3 years ago
Attachment #8496320 - Flags: feedback?(bzbarsky)
Preload has access to attributes (obviously).  It should just be passing the attribute value in, along with the other ones it already passes in.
Depends on: 1074136

Comment 6

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

Comment 7

3 years ago
Created attachment 8499848 [details] [diff] [review]
Teach the parser about in integrity (java)
Attachment #8496320 - Attachment is obsolete: true

Comment 8

3 years ago
Created attachment 8499849 [details] [diff] [review]
Teach the parser about in integrity (generated)

Comment 9

3 years ago
Created attachment 8499850 [details] [diff] [review]
impl. sri for script
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?
(Reporter)

Comment 11

3 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.
Yeah, we need moar tests.  this was just a flight hack and I am handing it off now.
Assignee: dougt → sworkman
Assignee: sworkman → francois
(Assignee)

Comment 13

3 years ago
Created attachment 8519711 [details] [diff] [review]
WIP impl for script tag

Here's a rebased version of Doug's patch that updates the URI format and makes all of the tests pass.
Attachment #8499850 - Attachment is obsolete: true
(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.
(Reporter)

Comment 15

3 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.
(Assignee)

Updated

3 years ago
Depends on: 1100206
(Assignee)

Comment 16

3 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.
Attachment #8499848 - Attachment is obsolete: true
(Assignee)

Comment 17

3 years ago
Created attachment 8528939 [details] [diff] [review]
WIP implementation

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.
Attachment #8519711 - Attachment is obsolete: true
Attachment #8528939 - Flags: feedback?(sworkman)
(Assignee)

Comment 18

3 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
Status: NEW → ASSIGNED
Flags: needinfo?(sworkman)
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 :)
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 20

3 years ago
Created attachment 8531407 [details] [diff] [review]
WIP implementation

Steve, the feedback request is only for the 8 lines of code contained in comment 17.
Attachment #8528939 - Attachment is obsolete: true
Attachment #8528939 - Flags: feedback?(sworkman)
Attachment #8531407 - Flags: feedback?(sworkman)
(Assignee)

Comment 21

3 years ago
Created attachment 8531674 [details] [diff] [review]
Parser changes (generated)
Attachment #8499849 - Attachment is obsolete: true
(Assignee)

Comment 22

3 years ago
Created attachment 8531679 [details]
Test page (html)

Here's the test page I use.
(Assignee)

Comment 23

3 years ago
Created attachment 8531680 [details]
Test page (js)
(Assignee)

Updated

3 years ago
Attachment #8531407 - Flags: feedback?(sworkman)
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.
Flags: needinfo?(honzab.moz)
Flags: needinfo?(sworkman)
(Assignee)

Updated

3 years ago
Depends on: 1113004
(Assignee)

Comment 25

3 years ago
Created attachment 8538287 [details] [diff] [review]
WIP implementation

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

I've opened bug 1113004 to add a new nsIHttpChannel.IsPrivateResponse().
Attachment #8531407 - Attachment is obsolete: true
(Assignee)

Comment 26

3 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
Attachment #8531674 - Attachment is obsolete: true
(Assignee)

Comment 27

3 years ago
Created attachment 8539207 [details] [diff] [review]
WIP implementation

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.
Attachment #8538287 - Attachment is obsolete: true
Attachment #8539207 - Flags: feedback?(mozilla)
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.
Attachment #8539207 - Flags: feedback?(mozilla)
(Assignee)

Updated

2 years ago
Depends on: 1114882
(Assignee)

Comment 29

2 years ago
Created attachment 8542512 [details] [diff] [review]
Mochitests
(Assignee)

Comment 30

2 years ago
Created attachment 8542520 [details] [diff] [review]
Code changes

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? :)
Attachment #8539207 - Attachment is obsolete: true
Attachment #8542520 - Flags: feedback?(mozilla)
(Assignee)

Updated

2 years ago
Keywords: dev-doc-needed
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.
Attachment #8542520 - Flags: feedback?(mozilla) → feedback+
(Assignee)

Comment 32

2 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.
(Assignee)

Comment 33

2 years ago
Created attachment 8542805 [details] [diff] [review]
Code changes

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 :)
Attachment #8542520 - Attachment is obsolete: true
Attachment #8542805 - Flags: feedback?(sstamm)
(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?
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.
(Assignee)

Comment 36

2 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.
(Assignee)

Comment 37

2 years ago
Created attachment 8543138 [details] [diff] [review]
Code changes

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?
Attachment #8542805 - Attachment is obsolete: true
Attachment #8542805 - Flags: feedback?(sstamm)
Flags: needinfo?(Ms2ger)
Attachment #8543138 - Flags: feedback?(sstamm)
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.
Attachment #8543138 - Flags: feedback?(sstamm)
(Assignee)

Comment 39

2 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.
(Assignee)

Comment 40

2 years ago
Created attachment 8543781 [details] [diff] [review]
Code changes
Attachment #8543138 - Attachment is obsolete: true
(Assignee)

Comment 41

2 years ago
Created attachment 8543864 [details] [diff] [review]
Mochitests
Attachment #8542512 - Attachment is obsolete: true
(Assignee)

Comment 42

2 years ago
Created attachment 8543865 [details] [diff] [review]
Code changes
Attachment #8543781 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Depends on: 1118185
(Assignee)

Comment 43

2 years ago
Created attachment 8545110 [details] [diff] [review]
Mochitests
Attachment #8543864 - Attachment is obsolete: true
(Assignee)

Comment 44

2 years ago
Created attachment 8545114 [details] [diff] [review]
Code changes
Attachment #8543865 - Attachment is obsolete: true
(Assignee)

Comment 45

2 years ago
Created attachment 8545115 [details] [diff] [review]
Mochitests
Attachment #8545110 - Attachment is obsolete: true
(Assignee)

Comment 46

2 years ago
Created attachment 8546418 [details] [diff] [review]
Mochitests
Attachment #8545115 - Attachment is obsolete: true
(Assignee)

Comment 47

2 years ago
Created attachment 8546419 [details] [diff] [review]
Code changes

Sid, this should fix the bug you found. We now only trigger onerror for same-origin scripts.
Attachment #8545114 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
No longer depends on: 1118185
(Assignee)

Comment 48

2 years ago
Created attachment 8568368 [details] [diff] [review]
Code changes

Updated for the new hash formats and now supports stylesheets.
Attachment #8546419 - Attachment is obsolete: true
(Assignee)

Comment 49

2 years ago
Created attachment 8568369 [details] [diff] [review]
Mochitests

Updated tests for the new hash formats. Stylesheet tests are missing.
Attachment #8546418 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8531679 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8531680 - Attachment is obsolete: true

Updated

2 years ago
Duplicate of this bug: 522651

Updated

2 years ago
Summary: Implement Sub Resource Integrity → Implement Subresource Integrity
(Assignee)

Comment 51

2 years ago
Created attachment 8603231 [details] [diff] [review]
Code changes

Updated patch that fully complies with the current spec.
Attachment #8568368 - Attachment is obsolete: true
(Assignee)

Comment 52

2 years ago
Created attachment 8603232 [details] [diff] [review]
Mochitests

Updated tests which include stylesheet support.
Attachment #8568369 - Attachment is obsolete: true
(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.
Flags: needinfo?(Ms2ger)
(Assignee)

Comment 54

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

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

Mochitests
Attachment #8612204 - Flags: review?(ehsan)
(Assignee)

Comment 55

2 years ago
Created 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
Attachment #8612205 - Flags: review?(ehsan)
(Assignee)

Updated

2 years ago
Attachment #8603231 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8603232 - Attachment is obsolete: true
(Assignee)

Comment 56

2 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.
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?
Flags: needinfo?(francois)
(Assignee)

Comment 58

2 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.
Flags: needinfo?(francois)
(Assignee)

Comment 59

2 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
(Assignee)

Comment 60

2 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
WPT has subresource-integrity tests, so we should use and extend those rather than writing new mochitests.
(Assignee)

Comment 62

2 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.
Andrea, do you feel comfortable taking these reviews from me?  I'm way too overloaded these days, and hate to keep Francois waiting...  :-)
Flags: needinfo?(amarchesini)
Comment on attachment 8612204 [details]
MozReview Request: Bug 992096 - Implement Sub Resource Integrity [2/2]. r=ckerschb

Andrea said yes offline...
Flags: needinfo?(amarchesini)
Attachment #8612204 - Flags: review?(ehsan) → review?(amarchesini)
Attachment #8612205 - Flags: review?(ehsan) → review?(amarchesini)
(Assignee)

Updated

2 years ago
Component: Networking → DOM: Security
(Assignee)

Comment 65

2 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
(Assignee)

Comment 66

2 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
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
Attachment #8612205 - Flags: review?(amarchesini) → review+
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!
Attachment #8612204 - Flags: review?(amarchesini) → review+
> 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.
(Assignee)

Comment 70

2 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
Attachment #8612204 - Attachment description: MozReview Request: Bug 992096 - Implement Sub Resource Integrity [2/2] → MozReview Request: Bug 992096 - Implement Sub Resource Integrity [2/2]. r?ckerschb
Attachment #8612204 - Flags: review+ → review?(mozilla)
(Assignee)

Comment 71

2 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
Attachment #8612205 - Attachment description: MozReview Request: Bug 992096 - Implement Sub Resource Integrity [1/2] → MozReview Request: Bug 992096 - Implement Sub Resource Integrity [1/2]. r=baku,r?ckerschb
Attachment #8612205 - Flags: review?(mozilla)
Attachment #8612205 - Flags: review?(amarchesini)
Attachment #8612205 - Flags: review+
(Assignee)

Comment 72

2 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 :)
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!
Attachment #8612205 - Flags: review?(amarchesini) → review+
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!
Attachment #8612205 - Flags: review?(mozilla)
(Assignee)

Comment 75

2 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
(Assignee)

Updated

2 years ago
Attachment #8612205 - Flags: review?(mozilla)
Attachment #8612205 - Flags: review?(amarchesini)
Attachment #8612205 - Flags: review+
(Assignee)

Comment 76

2 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
(Assignee)

Comment 77

2 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.
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.
Attachment #8612205 - Flags: review?(amarchesini) → review+
(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.
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.
Attachment #8612205 - Flags: review?(mozilla) → review+
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 :-)
Attachment #8612204 - Flags: review?(mozilla)

(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".
(Assignee)

Updated

2 years ago
Attachment #8612204 - Flags: review?(mozilla)
(Assignee)

Comment 83

2 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
(Assignee)

Comment 84

2 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
Attachment #8612205 - Attachment description: MozReview Request: Bug 992096 - Implement Sub Resource Integrity [1/2]. r=baku,r?ckerschb → MozReview Request: Bug 992096 - Implement Sub Resource Integrity [1/2]. r=baku,r=ckerschb
Attachment #8612205 - Flags: review?(mozilla)
Attachment #8612205 - Flags: review?(amarchesini)
Attachment #8612205 - Flags: review+
(Assignee)

Comment 85

2 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.
Blocks: 1187335
(Assignee)

Updated

2 years ago
Attachment #8612204 - Attachment description: MozReview Request: Bug 992096 - Implement Sub Resource Integrity [2/2]. r?ckerschb → MozReview Request: Bug 992096 - Implement Sub Resource Integrity [2/2]. r=ckerschb
Attachment #8612204 - Flags: review?(amarchesini)
(Assignee)

Comment 86

2 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
(Assignee)

Comment 87

2 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
(Assignee)

Comment 88

2 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
Attachment #8612205 - Flags: review?(mozilla)
Attachment #8612205 - Flags: review?(amarchesini)
Attachment #8612205 - Flags: review+
(Assignee)

Comment 89

2 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.
Attachment #8612204 - Flags: review?(mozilla)
Attachment #8612204 - Flags: review?(amarchesini)
Attachment #8612204 - Flags: review+

Comment 90

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bab5913ea6cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/948b2e9d1fa2
https://hg.mozilla.org/mozilla-central/rev/bab5913ea6cb
https://hg.mozilla.org/mozilla-central/rev/948b2e9d1fa2
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox43: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Keywords: devrel-needed

Comment 92

2 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)
relnote-firefox: --- → ?
> https://hg.mozilla.org/mozilla-central/diff/bab5913ea6cb/dom/locales/en-US/chrome/security/security.properties

What will replace all those %1$S?

Updated

2 years ago
Depends on: 1196740
Flags: needinfo?(francois)
(Assignee)

Comment 94

2 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?
Flags: needinfo?(francois)
> 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.
(Assignee)

Updated

2 years ago
Depends on: 1202015
(Assignee)

Updated

2 years ago
Depends on: 1202027
(Assignee)

Updated

2 years ago
Blocks: 1205448
(Assignee)

Updated

2 years ago
Depends on: 1208629
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
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 97

2 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.
Flags: needinfo?(jypenator)
relnote-firefox: ? → 43+
Removing devrel-needed due to https://hacks.mozilla.org/2015/09/subresource-integrity-in-firefox-43/
Keywords: devrel-needed
Blocks: 1278803
Flags: needinfo?(jypenator)
Depends on: 1364262
You need to log in before you can comment on or make changes to this bug.