1235572 - Enforce SRI on remote about:newtab

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Franziskus Kiefer [:franziskus]]

Ensure that SRI is enforced on remote about:newtab. Verbatim from Bug 1226928 Comment 20:

> I think what we can do though is add yet another flag to the loadInfo: 'mEnforceSRI'. So here is what I think might work best:
> 1) Within the LoadInfo constructor you can do something like
> mEnforceSRI = aLoadingContext->OwnerDoc()->GetChannel()->GetLoadInfo()->GetVerifySignedContent()
> then we would know on what channels we have to enforce SRI.
> Potentially we only want something like 'if (aContentType == TYPE_SCRIPT) { then enforce SRI if needed } or something similar.
> 2) Within SRICheck::VerifyIntegrity() we query the loadInfo again and set that we actually enforced SRI
> 3) Within either ::OnStreamComplete() or potentially also ::OnStopRequest() we can check that SRI was actually enforced on that channel.
> 4) Open question: What happens if one of the SRI checks on a subresource fails? In such a case it's hard to bubble up the error back to the docShell and load a completely different page.

Comment 1

[Olivier Yiptong [:oyiptong]]

also tracked on github: https://github.com/mozilla/remote-newtab/issues/133

Comment 2

[Jonathan Hao (inactive) [:jhao]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #0)
> Ensure that SRI is enforced on remote about:newtab. Verbatim from Bug
> 1226928 Comment 20:
> 
> > I think what we can do though is add yet another flag to the loadInfo: 'mEnforceSRI'. So here is what I think might work best:
> > 1) Within the LoadInfo constructor you can do something like
> > mEnforceSRI = aLoadingContext->OwnerDoc()->GetChannel()->GetLoadInfo()->GetVerifySignedContent()
> > then we would know on what channels we have to enforce SRI.
> > Potentially we only want something like 'if (aContentType == TYPE_SCRIPT) { then enforce SRI if needed } or something similar.
> > 2) Within SRICheck::VerifyIntegrity() we query the loadInfo again and set that we actually enforced SRI
> > 3) Within either ::OnStreamComplete() or potentially also ::OnStopRequest() we can check that SRI was actually enforced on that channel.
> > 4) Open question: What happens if one of the SRI checks on a subresource fails? In such a case it's hard to bubble up the error back to the docShell and load a completely different page.

About 4): We need to fall back to a different page if content signature didn't check out, but do we need to fall back too if SRI check fails? Perhaps we could just preserve the original SRI behavior.

Comment 3

[Franziskus Kiefer [:franziskus]]

> About 4): We need to fall back to a different page if content signature didn't check out, but do we need to fall back too if SRI check fails? Perhaps we could just preserve the original SRI behavior.

agree, we should preserve the normal SRI behaviour.
The idea here is really only to make sure that we have SRI on critical things. So we probably only need to make sure that SRICheck::VerifyIntegrity is called even if there's no SRI tag (e.g. in [1]), or even better, fail if sriMetadata.IsEmpty() && weEnforceSRI (we EnforceSRI has to come from some load flag).

[1] https://dxr.mozilla.org/mozilla-central/rev/e1cf617a1f2813b6cd66f460313a61c223406c9b/layout/style/Loader.cpp#953

Comment 4

[Jonathan Hao (inactive) [:jhao]]

Attachment: Enforce SRI on remote about:newtab

MozReview-Commit-ID: 3b48DQWlDgG

Hi Franziskus and Christoph,

Could you take a look at this WIP patch and let me know if there's any problem with my approach?

In both OnStreamComplete's of scripts and css, I check the owner document's channel's loadInfo. If it has a content signature, then we enforce SRI. If SRI is enforced but integrity is empty then we reject the loading.

I'm not sure why Christoph said we should have an mEnforceSRI in the loadInfo.

Thank you.

Comment 5

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8722398 [details] [diff] [review]
Enforce SRI on remote about:newtab

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

Thanks for taking this on Jonathan. I think that's the right direction to go (though I'm not super familiar with this code).

Are these all places where SRI is verified? Some test cases would be nice.

::: dom/base/nsScriptLoader.cpp
@@ +1451,5 @@
>      if (NS_FAILED(aSRIDataVerifier->Verify(request->mIntegrity, channel,
>                                             request->mCORSMode, mDocument))) {
>        rv = NS_ERROR_SRI_CORRUPT;
>      }
> +  } else {

looks like this } doesn't belong there

@@ +1453,5 @@
>        rv = NS_ERROR_SRI_CORRUPT;
>      }
> +  } else {
> +    nsCOMPtr<nsILoadInfo> loadInfo;
> +    mDocument->GetChannel()->GetLoadInfo(getter_AddRefs(loadInfo));

if DropDocumentReference() is called, mDocument is null

@@ +1459,5 @@
> +    // Enforce SRI if content signature exists
> +    bool enforceSRI;
> +    loadInfo->GetVerifySignedContent(&enforceSRI);
> +    if (enforceSRI && request->mIntegrity.IsEmpty()) {
> +      rv = NS_ERROR_SRI_MISSING;

if we want to keep the default SRI behaviour we should probably set rv = NS_ERROR_SRI_CORRUPT here as well (not sure if it's necessary though)

::: layout/style/Loader.cpp
@@ +955,5 @@
>  
>    SRIMetadata sriMetadata = mSheet->GetIntegrity();
> +  if (mLoader->mDocument) {
> +    nsCOMPtr<nsILoadInfo> loadInfo;
> +    mLoader->mDocument->GetChannel()->GetLoadInfo(getter_AddRefs(loadInfo));

what if mLoader->mDocument is null?

@@ +964,5 @@
> +    if (enforceSRI && sriMetadata.IsEmpty()) {
> +      LOG(("  Load was blocked by SRI"));
> +      MOZ_LOG(gSriPRLog, mozilla::LogLevel::Debug,
> +              ("css::Loader::OnStreamComplete, empty metadata"));
> +      mLoader->SheetComplete(this, NS_ERROR_SRI_MISSING);

same here, not sure if this is the right error to return.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8722398 [details] [diff] [review]
Enforce SRI on remote about:newtab

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

Hey Jonathan, thanks for looking into this. Please answer my question and I have a closer look. As a starting point - looks already promising.

::: dom/base/nsScriptLoader.cpp
@@ +1457,5 @@
> +    mDocument->GetChannel()->GetLoadInfo(getter_AddRefs(loadInfo));
> +
> +    // Enforce SRI if content signature exists
> +    bool enforceSRI;
> +    loadInfo->GetVerifySignedContent(&enforceSRI);

I can't recall: Are we setting the 'enforceSignedContent' bit in every loadInfo, or just the toplevel-load-loadInfo?
In other words, I suppose we have to do something like
loadInfo->LoadingNode()->GetDoc()->GetChannel() and query the loadInfo from that channel.

Ideally we should abstract that away within the loadInfo as something like
LoadInfo::GetEnforceSRI().

Comment 7

[Jonathan Hao (inactive) [:jhao]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> Comment on attachment 8722398 [details] [diff] [review]
> Enforce SRI on remote about:newtab
> 
> Review of attachment 8722398 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hey Jonathan, thanks for looking into this. Please answer my question and I
> have a closer look. As a starting point - looks already promising.
> 
> ::: dom/base/nsScriptLoader.cpp
> @@ +1457,5 @@
> > +    mDocument->GetChannel()->GetLoadInfo(getter_AddRefs(loadInfo));
> > +
> > +    // Enforce SRI if content signature exists
> > +    bool enforceSRI;
> > +    loadInfo->GetVerifySignedContent(&enforceSRI);
> 
> I can't recall: Are we setting the 'enforceSignedContent' bit in every
> loadInfo, or just the toplevel-load-loadInfo?

Maybe just the top level loadInfo, but I thought mDocument is already the top-level document. I might be wrong because I'm not very familiar with nsScriptLoader.

Or are you worrying about iframes being involved?

> In other words, I suppose we have to do something like
> loadInfo->LoadingNode()->GetDoc()->GetChannel() and query the loadInfo from
> that channel.
> 
> Ideally we should abstract that away within the loadInfo as something like
> LoadInfo::GetEnforceSRI().

Comment 8

[Jonathan Hao (inactive) [:jhao]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #5)
> Comment on attachment 8722398 [details] [diff] [review]
> Enforce SRI on remote about:newtab
> 
> Review of attachment 8722398 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for taking this on Jonathan. I think that's the right direction to go
> (though I'm not super familiar with this code).
> 
> Are these all places where SRI is verified? Some test cases would be nice.
> 
> ::: dom/base/nsScriptLoader.cpp
> @@ +1451,5 @@
> >      if (NS_FAILED(aSRIDataVerifier->Verify(request->mIntegrity, channel,
> >                                             request->mCORSMode, mDocument))) {
> >        rv = NS_ERROR_SRI_CORRUPT;
> >      }
> > +  } else {
> 
> looks like this } doesn't belong there

Actually this "}" matches with another "if" beyond the 8 lines diff, but I think I should've made it clearer. I'll rewrite it as something like

if (request->mIntegrity->IsEmpty()) {
   // check if we enforce SRI
} else {
   // the original SRI logic
}
 
> @@ +1453,5 @@
> >        rv = NS_ERROR_SRI_CORRUPT;
> >      }
> > +  } else {
> > +    nsCOMPtr<nsILoadInfo> loadInfo;
> > +    mDocument->GetChannel()->GetLoadInfo(getter_AddRefs(loadInfo));
> 
> if DropDocumentReference() is called, mDocument is null

I saw mDocument used everywhere in nsScriptLoader without being checked.
To clear your worries, DropDocumentReference is called only when the owner document is being destructed[1], so I guess it's fine.
 
> @@ +1459,5 @@
> > +    // Enforce SRI if content signature exists
> > +    bool enforceSRI;
> > +    loadInfo->GetVerifySignedContent(&enforceSRI);
> > +    if (enforceSRI && request->mIntegrity.IsEmpty()) {
> > +      rv = NS_ERROR_SRI_MISSING;
> 
> if we want to keep the default SRI behaviour we should probably set rv =
> NS_ERROR_SRI_CORRUPT here as well (not sure if it's necessary though)

Agree.

> ::: layout/style/Loader.cpp
> @@ +955,5 @@
> >  
> >    SRIMetadata sriMetadata = mSheet->GetIntegrity();
> > +  if (mLoader->mDocument) {
> > +    nsCOMPtr<nsILoadInfo> loadInfo;
> > +    mLoader->mDocument->GetChannel()->GetLoadInfo(getter_AddRefs(loadInfo));
> 
> what if mLoader->mDocument is null?

According to [2], if it's null then it must be a non-document sheet, then do we still care SRI?

Another is after DropDocumentReference is called, but it also happens when the owner document is being destructed.

> @@ +964,5 @@
> > +    if (enforceSRI && sriMetadata.IsEmpty()) {
> > +      LOG(("  Load was blocked by SRI"));
> > +      MOZ_LOG(gSriPRLog, mozilla::LogLevel::Debug,
> > +              ("css::Loader::OnStreamComplete, empty metadata"));
> > +      mLoader->SheetComplete(this, NS_ERROR_SRI_MISSING);
> 
> same here, not sure if this is the right error to return.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#1619
[2] https://dxr.mozilla.org/mozilla-central/source/layout/style/Loader.cpp#807
[3] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#1624

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonathan Hao [:jhao] from comment #7)
> Maybe just the top level loadInfo, but I thought mDocument is already the
> top-level document. I might be wrong because I'm not very familiar with
> nsScriptLoader.

Ah, I think you are right, mDocument is already the toplevel document. Either way, I think the better strategy is to have a function within loadInfo (::GetEnforceSRI()) where we consult the toplevel load within that loadInfo. Then we can call enforceSRI on the loadInfo of the script-channel. I think that makes more sense semantically and is less error prone.

Comment 10

[Frederik Braun [:freddy]]

(In reply to Jonathan Hao [:jhao] from comment #4)
> In both OnStreamComplete's of scripts and css, I check the owner document's
> channel's loadInfo. If it has a content signature, then we enforce SRI. If
> SRI is enforced but integrity is empty then we reject the loading.


For future-proofing this code I recommend untangling the SRI enforcement and the existence of content signatures.
There will be a way to enforce SRI on the web and it will be independent of content signatures.

(For compatibility, this enforcement rule would also contain an explicit list of elements that require integrity. So if a standard website would enforce integrity and then a later version of SRI brings support for additional elements, we'd have to make the new enforcement opt-in or break old web pages. I think Franziskus told me that this would live in something that LoadInfo would still abstract away as a boolean, but I'd rather let him speak to that in greater detail)

Comment 11

[Jonathan Hao (inactive) [:jhao]]

Attachment: Enforce SRI on remote about:newtab

A working patch, but I haven't address to Comment 9 and Comment 10.

Comment 12

[Jonathan Hao (inactive) [:jhao]]

Attachment: Tests of enforcing SRI on remote about:newtab

Testcases.

Comment 13

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8726171 [details] [diff] [review]
Enforce SRI on remote about:newtab

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

LGTM
If you address Comment 9 and Comment 10 we get there.

Could you use mozreview for future patches?

Comment 14

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8726172 [details] [diff] [review]
Tests of enforcing SRI on remote about:newtab

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

::: dom/security/test/contentverifier/browser_verify_content_about_newtab.js
@@ +68,5 @@
> +  { "aboutURI" : URI_SRI, "testString" : [
> +    SCRIPT_WITH_SRI_ABOUT_STRING,
> +    SCRIPT_WITHOUT_SRI_ABOUT_STRING,
> +    LINK_WITH_SRI_ABOUT_STRING,
> +    LINK_WITHOUT_SRI_ABOUT_STRING] }

does this test only a single load or also a refresh? (if you look in the test logic, you see that successful loads get a second reload test it would be good to have that in this case as well)

@@ +127,5 @@
>        }
>        yield ContentTask.spawn(
>            browser, aExpectedString, function * (aExpectedString) {
> +            let expectedStrings;
> +            if (typeof aExpectedString == "string") {

I think it would be nicer to change the TESTS vector to [aExpectedString], then we don't this if

::: dom/security/test/contentverifier/file_about_newtab_sri.html
@@ +20,5 @@
> +        onload="loaded('Link without SRI')"
> +        onerror="blocked('Link without SRI')">
> +
> +  <link rel="stylesheet" href="style2.css"
> +        integrity="sha256-qs8lnkunWoVldk5d5E+652yth4VTSHohlBKQvvgGwa8="

FYI I think we're using sha384, but not important here.

::: dom/security/test/contentverifier/style2.css
@@ +1,3 @@
> +#red-text {
> +  color: red;
> +}

why do you need two different files? you could just include the same file twice (once with and once without SRI)

Comment 15

[Jonathan Hao (inactive) [:jhao]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #13)
> Comment on attachment 8726171 [details] [diff] [review]
> Enforce SRI on remote about:newtab
> 
> Review of attachment 8726171 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM
> If you address Comment 9 and Comment 10 we get there.
> 
> Could you use mozreview for future patches?

Sure.

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #14)
> Comment on attachment 8726172 [details] [diff] [review]
> Tests of enforcing SRI on remote about:newtab
> 
> Review of attachment 8726172 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/security/test/contentverifier/style2.css
> @@ +1,3 @@
> > +#red-text {
> > +  color: red;
> > +}
> 
> why do you need two different files? you could just include the same file
> twice (once with and once without SRI)

Actually I included the same file twice at first, but the tests kept failing. I thought there was something wrong with enforcing SRI on CSS, but later I discovered an interesting behavior.
* If I put the "with SRI" one before the "without SRI" one, both will be loaded.
* If I put the "without SRI" one before the "with SRI" one, both will be blocked.
So I guess if a CSS file is linked twice, the first load (or error) will somehow be cached. That's why I used two different files. Perhaps there's a better way to work around this behavior.

Comment 16

[Jonathan Hao (inactive) [:jhao]]

(In reply to Jonathan Hao [:jhao] from comment #15)
> (In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #13)
> > why do you need two different files? you could just include the same file
> > twice (once with and once without SRI)
> 
> Actually I included the same file twice at first, but the tests kept
> failing. I thought there was something wrong with enforcing SRI on CSS, but
> later I discovered an interesting behavior.
> * If I put the "with SRI" one before the "without SRI" one, both will be
> loaded.
> * If I put the "without SRI" one before the "with SRI" one, both will be
> blocked.
> So I guess if a CSS file is linked twice, the first load (or error) will
> somehow be cached. That's why I used two different files. Perhaps there's a
> better way to work around this behavior.

Using "style.css?1" and "style.css?2" works too. I suppose this is better than two different files.

Comment 17

[Jonathan Hao (inactive) [:jhao]]

Attachment: MozReview Request: Bug 1235572 - Enforce SRI if content signature is enforced r?francois

Review commit: https://reviewboard.mozilla.org/r/38429/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38429/

Comment 18

[Jonathan Hao (inactive) [:jhao]]

Attachment: MozReview Request: Bug 1235572 - Tests of enforcing SRI on remote about:newtab r?francois

Review commit: https://reviewboard.mozilla.org/r/38431/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38431/

Comment 19

[Jonathan Hao (inactive) [:jhao]]

Actually I wanted to send feedback requests, because I still haven't addressed Freddy's Comment 10, but it seems that I can only send review requests using MozReview.

In the first patch, I tried to do it the way that Christoph described in Comment 9. Does it match what you meant?
In the second patch, I modified the tests based on what Franziskus mentioned in Comment 14.

Comment 20

[Jonathan Hao (inactive) [:jhao]]

(In reply to Frederik Braun [:freddyb] from comment #10)
> For future-proofing this code I recommend untangling the SRI enforcement and
> the existence of content signatures.
> There will be a way to enforce SRI on the web and it will be independent of
> content signatures.

I'm still thinking how to make SRI enforcing independent to content signature. For now, GetEnforceSRI() directly calls the owner document's GetVerifySignedContent(). Maybe I should make a flag enforceSRI and set it in LoadInfo's constructor based on owner document, but also allows it to be set through a setter for future use. Does it sound independent enough for you, Freddy?

> (For compatibility, this enforcement rule would also contain an explicit
> list of elements that require integrity. So if a standard website would
> enforce integrity and then a later version of SRI brings support for
> additional elements, we'd have to make the new enforcement opt-in or break
> old web pages. I think Franziskus told me that this would live in something
> that LoadInfo would still abstract away as a boolean, but I'd rather let him
> speak to that in greater detail)

I'm not sure if I understand your "explicit list of elements". Do you want to implement SRI on new elements, but gradually do enforcing on them? Currently, both scripts and css do its own checking if integrity is empty.[1][2] If we want to easily turn on SRI enforcing for selected elements, perhaps we should move the check of integrity's existence to somewhere inside SRI's own logic, say SRICheck::VerifyIntegrity(), and pass some enum (or use polymorphism inheritance?) to indicate which element is the caller. Inside SRI's logic, we can check if the caller under SRI enforcement. This could be a separate bug, though.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.cpp?from=nsScriptLoader.cpp#1417
[2] https://dxr.mozilla.org/mozilla-central/source/layout/style/Loader.cpp?from=Loader.cpp#959

Comment 21

[Frederik Braun [:freddy]]

(In reply to Jonathan Hao [:jhao] from comment #20)
> I'm not sure if I understand your "explicit list of elements". Do you want
> to implement SRI on new elements, but gradually do enforcing on them?

I want to build this in a way where we can enforce it on scripts & styles only (for now) but make it easy enough to change this in a compatible way in the future without breaking older things.

> Currently, both scripts and css do its own checking if integrity is
> empty.[1][2] If we want to easily turn on SRI enforcing for selected
> elements, perhaps we should move the check of integrity's existence to
> somewhere inside SRI's own logic, say SRICheck::VerifyIntegrity(), and pass
> some enum (or use polymorphism inheritance?) to indicate which element is
> the caller. Inside SRI's logic, we can check if the caller under SRI
> enforcement. This could be a separate bug, though.

I don't understand the code well enough to speak to that. I hope Franziskus can shed some light on that (and on the question whether this is easy enough to do here or in a follow-up bug)

Comment 22

[Franziskus Kiefer [:franziskus]]

> I'm still thinking how to make SRI enforcing independent to content signature. For now, GetEnforceSRI() directly calls the owner document's GetVerifySignedContent(). Maybe I should make a flag enforceSRI and set it in LoadInfo's constructor based on owner document, but also allows it to be set through a setter for future use. Does it sound independent enough for you, Freddy?

This should be fine for now (haven't looked at the code yet). The idea is to have GetEnforceSRI() to be able to set it in a different way than depending on verifySignedContent. But for now this is the only way it is set.

(In reply to Frederik Braun [:freddyb] from comment #21)
> (In reply to Jonathan Hao [:jhao] from comment #20)
> > Currently, both scripts and css do its own checking if integrity is
> > empty.[1][2] If we want to easily turn on SRI enforcing for selected
> > elements, perhaps we should move the check of integrity's existence to
> > somewhere inside SRI's own logic, say SRICheck::VerifyIntegrity(), and pass
> > some enum (or use polymorphism inheritance?) to indicate which element is
> > the caller. Inside SRI's logic, we can check if the caller under SRI
> > enforcement. This could be a separate bug, though.
> 
> I don't understand the code well enough to speak to that. I hope Franziskus
> can shed some light on that (and on the question whether this is easy enough
> to do here or in a follow-up bug)

I think for what we want to do at the moment (enforce SRI on the two loads we might have it) we can do this explicitly. If we actually do SRI on more than those two, we probably want to rewrite SRI as a StreamListener (or the like) to intercept loads and check for SRI. Then there is a single place to check and enforce SRI. This will be it's own bug and probably bigger than this. But as long as there are only those two SRICheck calls, that's not necessary IMHO.

Comment 23

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8727311 [details]
MozReview Request: Bug 1235572 - Tests of enforcing SRI on remote about:newtab r?francois

https://reviewboard.mozilla.org/r/38431/#review35021

looking good in general. But please check the comments below.

::: dom/security/test/contentverifier/browser_verify_content_about_newtab.js:144
(Diff revision 1)
> -            url: INVALIDATE_FILE,
> +            url: (aNewTabPref == URI_GOOD) ? INVALIDATE_GOOD_FILE : INVALIDATE_SRI_FILE

I'm not sure if I understand the logic in case of INVALIDATE_SRI_FILE.

* You make the SRI file invalid
* Reload the newtab
* The current logic expects the page to load the fallback now, right? (that's what should happen when the content-signature fails) But that's not what's supposed to happen when SRI fails. In this case we simply don't load the script, i.e. we still load the remote newtab page.

I'm not sure if that's what's happening here.

::: dom/security/test/contentverifier/browser_verify_content_about_newtab.js:194
(Diff revision 1)
> -      if (aExpectedString == GOOD_ABOUT_STRING) {
> +      if (testCase.reload || aExpectedStrings[0] == GOOD_ABOUT_STRING) {

If you use reload it's probably better to add the reload in TESTS to the few cases that have GOOD_ABOUT_STRING (simplifies the test logic)

Comment 24

[Jonathan Hao (inactive) [:jhao]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #23)
> Comment on attachment 8727311 [details]
> MozReview Request: Bug 1235572 - Tests of enforcing SRI on remote
> about:newtab f?franziskus
> 
> https://reviewboard.mozilla.org/r/38431/#review35021
> 
> looking good in general. But please check the comments below.
> 
> :::
> dom/security/test/contentverifier/browser_verify_content_about_newtab.js:144
> (Diff revision 1)
> > -            url: INVALIDATE_FILE,
> > +            url: (aNewTabPref == URI_GOOD) ? INVALIDATE_GOOD_FILE : INVALIDATE_SRI_FILE
> 
> I'm not sure if I understand the logic in case of INVALIDATE_SRI_FILE.
> 
> * You make the SRI file invalid
> * Reload the newtab
> * The current logic expects the page to load the fallback now, right?
> (that's what should happen when the content-signature fails) But that's not
> what's supposed to happen when SRI fails. In this case we simply don't load
> the script, i.e. we still load the remote newtab page.
> 
> I'm not sure if that's what's happening here.

Thanks for your feedback.

I realize that I misunderstood. What I did was invalidate the HTML and reload, so getting about:blank is the correct result, but that doesn't test anything new. What's needed here is to invalidate the script and css, and then reload. I'll rewrite that part.

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8727310 [details]
MozReview Request: Bug 1235572 - Enforce SRI if content signature is enforced r?francois

https://reviewboard.mozilla.org/r/38429/#review35143

Looks good so far, probably what we should do though is set a flag on the document that keeps track whether we should enforce SRI or not. I agree with FreddyB that the enforcement meachnaism should be as generic as possible. If we set a flag on the document then we could query that flag from witin LoadInfo::GetEnforceSRI. Most likely the working group will adopt a mechanism for CSP to enforce SRI. Whenever we set that CSP on the document, we could than also set the flag on the document, similar what we do for upgrade-insecure-reuqests [1]. That same mechanism also works (with some minor tweaks) for about:newtab.

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

::: dom/base/nsScriptLoader.cpp:1422
(Diff revision 1)
> +  if (request->mIntegrity.IsEmpty()) {

Are you sure you didn't change the behavior introcuded within [1] here? What happened to the aSRIStatus argument?

[1] http://hg.mozilla.org/mozilla-central/rev/2bb2a3f0fb90#l1.93

::: dom/base/nsScriptLoader.cpp:1424
(Diff revision 1)
> +    channel->GetLoadInfo(getter_AddRefs(loadInfo));

You can simplify to
nsCOMPtr<nsILoadInfo> loadInfo = channel->GetLoadInfo();

::: layout/style/Loader.cpp:963
(Diff revision 1)
> +  loadInfo->GetEnforceSRI(&enforceSRI);

so, I think we should bail out early here in case we get the enforceSRI flag in the loadInfo but can not query any SRI attribute, right?

::: netwerk/base/LoadInfo.cpp:418
(Diff revision 1)
> +}

Thanks for encapsulating the EnforceSRI, the next step now is to use early returns, e.g.

if (!node) {
  // nothing to enforce if there is no node
  return NS_OK;
}
...
if (!ownerDoc) {
  // nothing to enforce if there is no ownerDoc
}

...

return ownerLoadinfo->GetVerifySignedContent(aResult);

btw, OwnerDoc() already returns an nsIDocument*, so no need for the do_Queryinterface.

Comment 26

[Jonathan Hao (inactive) [:jhao]]

https://reviewboard.mozilla.org/r/38429/#review35143

> so, I think we should bail out early here in case we get the enforceSRI flag in the loadInfo but can not query any SRI attribute, right?

I don't quite get what you mean. Doesn't line 965 take care of the the case when enforceSRI is set and no SRI attribute is found?

Comment 27

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8727310 [details]
MozReview Request: Bug 1235572 - Enforce SRI if content signature is enforced r?francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38429/diff/1-2/

Comment 28

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8727311 [details]
MozReview Request: Bug 1235572 - Tests of enforcing SRI on remote about:newtab r?francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38431/diff/1-2/

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

Francois basically owns SRI. I chattet with him and he will review those patches!

Comment 30

[Franziskus Kiefer [:franziskus]]

https://reviewboard.mozilla.org/r/38429/#review36425

::: netwerk/base/nsILoadInfo.idl:32
(Diff revision 2)
>  typedef unsigned long nsSecurityFlags;
>  
>  /**
>   * An nsILoadOwner represents per-load information about who started the load.
>   */
> -[scriptable, builtinclass, uuid(ddc65bf9-2f60-41ab-b22a-4f1ae9efcd36)]
> +[scriptable, builtinclass, uuid(ee2a7028-e37e-4c69-9d2a-f9a8b90dad33)]

it's not necessary anymore to bump the uuid afaik

Comment 31

[François Marier [:francois]]

https://reviewboard.mozilla.org/r/38429/#review36447

r+ with the following changes to the PR_LOGs

::: dom/base/nsScriptLoader.cpp:1435
(Diff revision 2)
> +    nsCOMPtr<nsILoadInfo> loadInfo = channel->GetLoadInfo();
> +
> +    bool enforceSRI = false;
> +    loadInfo->GetEnforceSRI(&enforceSRI);
> +    if (enforceSRI) {
> +      rv = NS_ERROR_SRI_CORRUPT;

I would suggest a LOG message here (something like "A required SRI attribute was not found") to easily distinguish this case from a genuine bad SRI hash.

::: layout/style/Loader.cpp:970
(Diff revision 2)
> +  if ((sriMetadata.IsEmpty() && enforceSRI) || (!sriMetadata.IsEmpty() &&
>        NS_FAILED(SRICheck::VerifyIntegrity(sriMetadata, aLoader,
>                                            mSheet->GetCORSMode(), aBuffer,
> -                                          mLoader->mDocument))) {
> +                                          mLoader->mDocument)))) {
>      LOG(("  Load was blocked by SRI"));
>      MOZ_LOG(gSriPRLog, mozilla::LogLevel::Debug,

Similarly here, I think it would be good to have a different MOZ_LOG message when the reason for blocking this load is because of a missing SRI attribute.

::: netwerk/base/LoadInfo.cpp:108
(Diff revision 2)
>      // then we should enforce upgrade insecure requests only for preloads.
>      mUpgradeInsecureRequests =
>        aLoadingContext->OwnerDoc()->GetUpgradeInsecureRequests(false) ||
>        (nsContentUtils::IsPreloadType(mInternalContentPolicyType) &&
>         aLoadingContext->OwnerDoc()->GetUpgradeInsecureRequests(true));
> + 

Nit: stray whitespace

Comment 32

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8727310 [details]
MozReview Request: Bug 1235572 - Enforce SRI if content signature is enforced r?francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38429/diff/2-3/

Comment 33

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8727311 [details]
MozReview Request: Bug 1235572 - Tests of enforcing SRI on remote about:newtab r?francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38431/diff/2-3/

Comment 34

[Franziskus Kiefer [:franziskus]]

https://reviewboard.mozilla.org/r/38429/#review36579

looks good, just a few comments, but you can also wait for Francois' review.

::: layout/style/Loader.cpp:972
(Diff revision 3)
> +              ("css::Loader::OnStreamComplete, required SRI not found"));
> +      mLoader->SheetComplete(this, NS_ERROR_SRI_CORRUPT);
> +      return NS_OK;
> +    }
> +  } else if (NS_FAILED(SRICheck::VerifyIntegrity(sriMetadata, aLoader,
> -                                          mSheet->GetCORSMode(), aBuffer,
> +                                                 mSheet->GetCORSMode(), aBuffer,

these look like tabs oO

::: netwerk/base/LoadInfo.cpp:110
(Diff revision 3)
>        aLoadingContext->OwnerDoc()->GetUpgradeInsecureRequests(false) ||
>        (nsContentUtils::IsPreloadType(mInternalContentPolicyType) &&
>         aLoadingContext->OwnerDoc()->GetUpgradeInsecureRequests(true));
> +
> +    // if owner doc has content signature, we enforce SRI
> +    nsCOMPtr<nsILoadInfo> loadInfo = aLoadingContext->OwnerDoc()->GetChannel()->GetLoadInfo();

you might want to check if aLoadingContext->OwnerDoc()->GetChannel() != NULL

::: netwerk/base/LoadInfo.cpp:111
(Diff revision 3)
>        (nsContentUtils::IsPreloadType(mInternalContentPolicyType) &&
>         aLoadingContext->OwnerDoc()->GetUpgradeInsecureRequests(true));
> +
> +    // if owner doc has content signature, we enforce SRI
> +    nsCOMPtr<nsILoadInfo> loadInfo = aLoadingContext->OwnerDoc()->GetChannel()->GetLoadInfo();
> +    loadInfo->GetVerifySignedContent(&mEnforceSRI);

and here if we actually were able to get loadInfo

Comment 35

[François Marier [:francois]]

Comment on attachment 8727310 [details]
MozReview Request: Bug 1235572 - Enforce SRI if content signature is enforced r?francois

https://reviewboard.mozilla.org/r/38429/#review36739

Comment 36

[François Marier [:francois]]

Comment on attachment 8727311 [details]
MozReview Request: Bug 1235572 - Tests of enforcing SRI on remote about:newtab r?francois

https://reviewboard.mozilla.org/r/38431/#review36741

Comment 37

[Jonathan Hao (inactive) [:jhao]]

https://reviewboard.mozilla.org/r/38429/#review36579

> these look like tabs oO

Thanks for your review.
Actually those are all spaces, but this line is too long and has so many parentheses that it indeed reduces readability.
How about:

```cpp
  } else {
    nsresult rv = SRICheck::VerifyIntegrity(sriMetadata, aLoader,
                                            mSheet->GetCORSMode(), aBuffer,
                                            mLoader->mDocument);
    if (NS_FAILED(rv)) {
      LOG(("  Load was blocked by SRI"));
      MOZ_LOG(gSriPRLog, mozilla::LogLevel::Debug,
              ("css::Loader::OnStreamComplete, bad metadata"));
      mLoader->SheetComplete(this, NS_ERROR_SRI_CORRUPT);
      return NS_OK;
    }
  }
```

Comment 38

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8727310 [details]
MozReview Request: Bug 1235572 - Enforce SRI if content signature is enforced r?francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38429/diff/3-4/

Comment 39

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8727311 [details]
MozReview Request: Bug 1235572 - Tests of enforcing SRI on remote about:newtab r?francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38431/diff/3-4/

Comment 40

[Jonathan Hao (inactive) [:jhao]]

https://reviewboard.mozilla.org/r/38429/#review35143

> I don't quite get what you mean. Doesn't line 965 take care of the the case when enforceSRI is set and no SRI attribute is found?

Dropping this issue for now. Please re-open if you think there's still concern.

Comment 41

[Jonathan Hao (inactive) [:jhao]]

Attachment: Python code to generate content signatures for tests.

This is what I used to generate content signatures when writing those tests. It may be useful if someone else tries to modify those tests.

Comment 42

[Jonathan Hao (inactive) [:jhao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=caa1a95397d7&selectedJob=18141237

Try builds looks good except for some intermittent failures in unrelated tests. I think it's ready to land.

Thanks to Franziskus, Christoph, and François for your help and review.

Comment 43

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c69aaedbde9
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6e3806b9aff

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5c69aaedbde9
https://hg.mozilla.org/mozilla-central/rev/a6e3806b9aff