Closed Bug 1393439 Opened 7 years ago Closed 7 years ago

SRI issue when using a service worker

Categories

(Core :: DOM: Security, defect, P2)

Product:
Core
Component:
DOM: Security
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: julienw, Assigned: tt)

References

(Blocks 1 open bug)

Details

(Whiteboard: [domsecurity-active])

Attachments

(4 files)

Bug 1393439 - P1: Don't return TypeError for no-cors mode and don't check SRI for the hidden opaque body.
59 bytes, text/x-review-board-request
bkelly
: review+
Details
Bug 1393439 - P2: Correct the variable naming.
59 bytes, text/x-review-board-request
bkelly
: review+
Details
Bug 1393439 - P3: Remove the wpt test for assuming to throw no-cors request with integrity attribute.
59 bytes, text/x-review-board-request
bkelly
: review+
Details
Bug 1393439 - P4: Modify wpt test to verify only the opaque response with the empty string will pass the SRI check.
59 bytes, text/x-review-board-request
bkelly
: review+
Details 
STR:
1. Go to https://everlong.org/mozilla/testcase-sri/
2. Reload the page with F5

Expected:
* The script runs and we have 2 lines of text.

Actual:
* The script doesn't run and we have 1 line of text.


What happens is that the script uses an "integrity" attribute, and the page registers a Service Worker that does nothing more than fetch the requested files.

The error we get is:

 None of the “sha384” hashes in the integrity attribute match the content of the subresource. [testcase-sri]
 Failed to load ‘https://everlong.org/mozilla/testcase-sri/script.js’. A ServiceWorker passed a promise to FetchEvent.respondWith() that rejected with ‘TypeError: Request integrity metadata should be an empty string when in no-cors mode.’. [sw.js]


I believe this is a bug in Firefox. I understand the error but I think it shouldn't happen if the fetch call is triggered from a Service Worker in the right scope for this request.
Here are the 3 files for this STR:

index.html:

<!doctype html> 
<html> 
  <head> 
    <meta charset='utf-8'> 
    <meta name='viewport' content='initial-scale=1'> 
  </head> 
  <body> 
    <p>If the script successfully loaded, you'll have something written 
    below.</p> 
    <div></div> 
    <script> 
navigator.serviceWorker.register('./sw.js').then( 
  registration => console.log('Service worker registration succeeded:', registra 
tion), 
  error => console.log('Service worker registration failed:', error) 
); 
    </script> 
    <script src='script.js' integrity='sha384-U9DHTQYuliLxLRa5M4e7JeWekjnxc9KV0g 
9TeoNbQYL3sMT36zF/FDILesrhDOLL'></script> 
  </body> 
</html> 


script.js:

const div = document.querySelector('div');
div.textContent = 'Hello World';


sw.js:

self.addEventListener('fetch', function(event) {
  event.respondWith(
    fetch(event.request)
  );
});
Your <script> element makes a no-cors request.  The `fetch()` API is designed to reject when integrity is specified on no-cors.  See step 32.3 here:

https://fetch.spec.whatwg.org/#dom-request

Try adding a crossorigin attribute to your <script> element.

Anne, does it make sense for fetch() to reject when integrity is specified on a no-cors request?  I believe integrity is often used on <script> which defaults to no-cors.  This makes it hard to do pass-through like this.  What do you think?
Flags: needinfo?(annevk)
If you need other pages to reproduct/test, here they are: 

https://van11y.net/accessible-modal-tooltip/ (go to "How it is working" and display the same page on Chrome/Opera/Edge, or look at the snapshot of the working page here: https://twitter.com/Nico3333fr/status/900422381264568321 )

https://rocssti.net/en/code-css-source-rocssti (below "Contents", the CSS source code should be enhanced by Prism.js, have a look on Chrome).

All these bugs have been seen and reproduced on Fx55.0.2 on PC and Mac.

If you make a hard refresh (Ctrl+F5 on PC), the bug disappears. If you make a "simple" refresh (only F5), the bug remains.

Hope this could help.
Ben, it's probably reasonable to not do anything instead.
Flags: needinfo?(annevk)
(In reply to Nicolas Hoffmann from comment #3)
> If you need other pages to reproduct/test, here they are: 
> 
> https://van11y.net/accessible-modal-tooltip/ (go to "How it is working" and
> display the same page on Chrome/Opera/Edge, or look at the snapshot of the
> working page here: https://twitter.com/Nico3333fr/status/900422381264568321 )

I believe this has the same problem.  Your outer <script> tag is generating a no-cors Request which is not allowed to have an integrity value when it gets passed through to fetch().

You are not seeing the message about this because the service worker script catches the TypeError and converts it to offlineAsset().  Of course, offlineAsset() does not match the outer integrity check.

> https://rocssti.net/en/code-css-source-rocssti (below "Contents", the CSS
> source code should be enhanced by Prism.js, have a look on Chrome).

And same thing here.

The reason this does not happen in chrome is because they have not implemented Request.integrity yet.  Once they do chrome should fail in the same way.
(In reply to Anne (:annevk) from comment #4)
> Ben, it's probably reasonable to not do anything instead.

Does this mean just remove the TypeError on cors or also skip doing the integrity check?
Looking at the spec discussion, it seems we can remove the TypeError here:

http://searchfox.org/mozilla-central/source/dom/fetch/Request.cpp#536

But we also need to avoid comparing the integrity check against the hidden opaque body.  We don't do that today because we have the TypeError protecting us.  So we need to add a check for "not opaque" here:

http://searchfox.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#740

I think that would help fix this problem.

We also need to add some fetch integrity tests in no-cors mode.

Tom, do you have any time to look at this?
Flags: needinfo?(ttung)
Sure! Start to wrok on this!
Assignee: nobody → ttung
Status: NEW → ASSIGNED
Flags: needinfo?(ttung)
(In reply to Ben Kelly [:bkelly] from comment #8)
> Looking at the spec discussion, it seems we can remove the TypeError here:
> 
> http://searchfox.org/mozilla-central/source/dom/fetch/Request.cpp#536
> 
> But we also need to avoid comparing the integrity check against the hidden
> opaque body.  We don't do that today because we have the TypeError
> protecting us.  So we need to add a check for "not opaque" here:
> 
> http://searchfox.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#740
> 
> I think that would help fix this problem.
> 
> We also need to add some fetch integrity tests in no-cors mode.

Right now, I have two question for this change:

1. What should we do for the <script> element with wrong integrity attribute? 
That might be the problem if a web site cannot pass through its SRI check for a <script> element with wrong integrity attribute. However, the same request works fine after intercepting by a service worker.

2. Do we need to raise a warning when there is a no-cors mode request with integrity attribute since we don't check the integrity in this situation?
(In reply to Tom Tung [:tt] from comment #13)
> 1. What should we do for the <script> element with wrong integrity
> attribute? 
> That might be the problem if a web site cannot pass through its SRI check
> for a <script> element with wrong integrity attribute. However, the same
> request works fine after intercepting by a service worker.
typo: after intercepted by a SW

BTW: I think it's better to align the behavior for intercepted by the SW with no SW.
> However, the same request works fine after intercepting by a service worker.

Why would it work fine? fetch() should reject if there's an integrity mismatch.

> Do we need to raise a warning when there is a no-cors mode request with integrity attribute since we don't check the integrity in this situation?

I think you misread the specification. It should definitely still be checked. The only proposed change is to make the Request constructor no longer throw for "no-cors" in combination with a non-empty integrity value.
Yea, the intention is:

* Allow no-cors fetch() with integrity to work for same origin requests.  So the cases where fetch() resolves a non-opaque Response.
* If a no-cors fetch() with integrity results in an opaque response, then we compute the hash against an empty body which should fail the integrity check and result in a rejected promise.

Its unclear to me what <script integrity> does for a cross-origin no-cors script today.  It may succeed without SW and we would fail with the SW pass-through case.  This would still be a difference, but its a smaller one than today.
Comment on attachment 8901085 [details]
Bug 1393439 - P1: Don't return TypeError for no-cors mode and don't check SRI for the hidden opaque body.

https://reviewboard.mozilla.org/r/172580/#review178050

I think maybe you need two different helper methods here:

* `ShouldCheckSRI()` would return true if metadata is specified and its not an error response.
* `ShouldCopyDataToSRI()` would return true if `ShouldCheckSRI()` is true and the response is not opaque.

Then use `ShouldCopyDataToSRI()` in `OnDataAvailable()`.  Everywhere else you can use `ShouldCheckSRI()`.

What do you think?

::: dom/fetch/FetchDriver.cpp:49
(Diff revision 1)
>  namespace dom {
>  
> +namespace {
> +
> +bool
> +ShouldCheckSRI(const bool aSRIMetadataIsEmpty, const ResponseType& aType)

nit: It might cleaner to make this take the request and response as args.  Then you can get whatever data out of them you need.  This reduces all the getter calls outside of ShouldCheckSRI().

::: dom/fetch/FetchDriver.cpp:438
(Diff revision 1)
>      }
>    }
>  
>    MOZ_ASSERT(filteredResponse);
>    MOZ_ASSERT(mObserver);
> -  if (filteredResponse->Type() == ResponseType::Error ||
> +  if (!ShouldCheckSRI(mRequest->GetIntegrity().IsEmpty(),

This is not quite right.  We still want to treat the fetch() as being integrity validated.  So you should not skip this for opaque type.

::: dom/fetch/FetchDriver.cpp:612
(Diff revision 1)
>    // Resolves fetch() promise which may trigger code running in a worker.  Make
>    // sure the Response is fully initialized before calling this.
>    mResponse = BeginAndGetFilteredResponse(response, foundOpaqueRedirect);
>  
> -  // From "Main Fetch" step 17: SRI-part1.
> -  if (mResponse->Type() != ResponseType::Error &&
> +  // From "Main Fetch" step 19: SRI-part1.
> +  if (ShouldCheckSRI(mRequest->GetIntegrity().IsEmpty(), mResponse->Type()) &&

Again, we probably don't want to bypass this logic for opaque responses.

::: dom/fetch/FetchDriver.cpp:750
(Diff revision 1)
>    uint32_t aRead;
>    MOZ_ASSERT(mResponse);
>    MOZ_ASSERT(mPipeOutputStream);
>  
> -  // From "Main Fetch" step 17: SRI-part2.
> -  if (mResponse->Type() != ResponseType::Error &&
> +  // From "Main Fetch" step 19: SRI-part2.
> +  if (ShouldCheckSRI(mRequest->GetIntegrity().IsEmpty(), mResponse->Type())) {

This is the one place we want to bypass for opaque response.  Its simulating an empty body.

::: dom/fetch/FetchDriver.cpp:788
(Diff revision 1)
>    } else {
>      MOZ_ASSERT(mResponse);
>      MOZ_ASSERT(!mResponse->IsError());
>  
> -    // From "Main Fetch" step 17: SRI-part3.
> -    if (mResponse->Type() != ResponseType::Error &&
> +    // From "Main Fetch" step 19: SRI-part3.
> +    if (ShouldCheckSRI(mRequest->GetIntegrity().IsEmpty(), mResponse->Type())) {

And again, here we still want to do this logic for opaque responses.

::: dom/fetch/FetchDriver.cpp:820
(Diff revision 1)
>      }
>    }
>  
>    if (mObserver) {
> -    if (mResponse->Type() != ResponseType::Error &&
> -        !mRequest->GetIntegrity().IsEmpty()) {
> +    //From "Main Fetch" step 19.1, 19.2: Process response.
> +    if (ShouldCheckSRI(mRequest->GetIntegrity().IsEmpty(), mResponse->Type())) {

And here, we should do this for opaque responses.
(In reply to Anne (:annevk) from comment #15)
(In reply to Ben Kelly [:bkelly] from comment #16)
> Yea, the intention is:
> 
> * Allow no-cors fetch() with integrity to work for same origin requests.  So
> the cases where fetch() resolves a non-opaque Response.
> * If a no-cors fetch() with integrity results in an opaque response, then we
> compute the hash against an empty body which should fail the integrity check
> and result in a rejected promise.
> 
> Its unclear to me what <script integrity> does for a cross-origin no-cors
> script today.  It may succeed without SW and we would fail with the SW
> pass-through case.  This would still be a difference, but its a smaller one
> than today.

Thanks for the clarifications, and they solve my confusion!
(In reply to Ben Kelly [:bkelly] from comment #17)
> Comment on attachment 8901085 [details]
> Bug 1393439 - P1: Don't return TypeError for no-cors mode and don't check
> SRI for the hidden opaque body.
> 
> https://reviewboard.mozilla.org/r/172580/#review178050
> 
> I think maybe you need two different helper methods here:
> 
> * `ShouldCheckSRI()` would return true if metadata is specified and its not
> an error response.
> * `ShouldCopyDataToSRI()` would return true if `ShouldCheckSRI()` is true
> and the response is not opaque.
> 
> Then use `ShouldCopyDataToSRI()` in `OnDataAvailable()`.  Everywhere else
> you can use `ShouldCheckSRI()`.
> 
> What do you think?

It sounds great, but maybe just explicitly check ResponseType::Opaque for now since there is no another place need to use this function?

> ::: dom/fetch/FetchDriver.cpp:49
> (Diff revision 1)
> >  namespace dom {
> >  
> > +namespace {
> > +
> > +bool
> > +ShouldCheckSRI(const bool aSRIMetadataIsEmpty, const ResponseType& aType)
> 
> nit: It might cleaner to make this take the request and response as args. 
> Then you can get whatever data out of them you need.  This reduces all the
> getter calls outside of ShouldCheckSRI().

Sure.

> ::: dom/fetch/FetchDriver.cpp:438
> (Diff revision 1)
> >      }
> >    }
> >  
> >    MOZ_ASSERT(filteredResponse);
> >    MOZ_ASSERT(mObserver);
> > -  if (filteredResponse->Type() == ResponseType::Error ||
> > +  if (!ShouldCheckSRI(mRequest->GetIntegrity().IsEmpty(),
> 
> This is not quite right.  We still want to treat the fetch() as being
> integrity validated.  So you should not skip this for opaque type.

Yeah, I misunderstood about what should we do for this bug. Sorry for that :(

> ::: dom/fetch/FetchDriver.cpp:612
> (Diff revision 1)
> >    // Resolves fetch() promise which may trigger code running in a worker.  Make
> >    // sure the Response is fully initialized before calling this.
> >    mResponse = BeginAndGetFilteredResponse(response, foundOpaqueRedirect);
> >  
> > -  // From "Main Fetch" step 17: SRI-part1.
> > -  if (mResponse->Type() != ResponseType::Error &&
> > +  // From "Main Fetch" step 19: SRI-part1.
> > +  if (ShouldCheckSRI(mRequest->GetIntegrity().IsEmpty(), mResponse->Type()) &&
> 
> Again, we probably don't want to bypass this logic for opaque responses.
> 
> ::: dom/fetch/FetchDriver.cpp:750
> (Diff revision 1)
> >    uint32_t aRead;
> >    MOZ_ASSERT(mResponse);
> >    MOZ_ASSERT(mPipeOutputStream);
> >  
> > -  // From "Main Fetch" step 17: SRI-part2.
> > -  if (mResponse->Type() != ResponseType::Error &&
> > +  // From "Main Fetch" step 19: SRI-part2.
> > +  if (ShouldCheckSRI(mRequest->GetIntegrity().IsEmpty(), mResponse->Type())) {
> 
> This is the one place we want to bypass for opaque response.  Its simulating
> an empty body.

Like:
ShouldCheckSRI(mRequest, mResponse) && mResponse->Type() != Response::Opaque

> ::: dom/fetch/FetchDriver.cpp:788
> (Diff revision 1)
> >    } else {
> >      MOZ_ASSERT(mResponse);
> >      MOZ_ASSERT(!mResponse->IsError());
> >  
> > -    // From "Main Fetch" step 17: SRI-part3.
> > -    if (mResponse->Type() != ResponseType::Error &&
> > +    // From "Main Fetch" step 19: SRI-part3.
> > +    if (ShouldCheckSRI(mRequest->GetIntegrity().IsEmpty(), mResponse->Type())) {
> 
> And again, here we still want to do this logic for opaque responses.
> 
> ::: dom/fetch/FetchDriver.cpp:820
> (Diff revision 1)
> >      }
> >    }
> >  
> >    if (mObserver) {
> > -    if (mResponse->Type() != ResponseType::Error &&
> > -        !mRequest->GetIntegrity().IsEmpty()) {
> > +    //From "Main Fetch" step 19.1, 19.2: Process response.
> > +    if (ShouldCheckSRI(mRequest->GetIntegrity().IsEmpty(), mResponse->Type())) {
> 
> And here, we should do this for opaque responses.

Thanks for the explanations again!
(In reply to Ben Kelly [:bkelly] from comment #8)
> We also need to add some fetch integrity tests in no-cors mode.

I'll provide a patch for add some fetch integrity test in no-cors mode.
Comment on attachment 8901085 [details]
Bug 1393439 - P1: Don't return TypeError for no-cors mode and don't check SRI for the hidden opaque body.

https://reviewboard.mozilla.org/r/172580/#review179086
Attachment #8901085 - Flags: review?(bkelly) → review+
Comment on attachment 8901086 [details]
Bug 1393439 - P2: Correct the variable naming.

https://reviewboard.mozilla.org/r/172582/#review179088
Attachment #8901086 - Flags: review?(bkelly) → review+
Comment on attachment 8902209 [details]
Bug 1393439 - P3: Remove the wpt test for assuming to throw no-cors request with integrity attribute.

https://reviewboard.mozilla.org/r/173688/#review179090

Can you also add a WPT test case that attempts an integrity fetch() on a cross-origin no-cors request that results in an opaque result?
Attachment #8902209 - Flags: review?(bkelly) → review+
Priority: -- → P2
Whiteboard: [domsecurity-active]
Comment on attachment 8902209 [details]
Bug 1393439 - P3: Remove the wpt test for assuming to throw no-cors request with integrity attribute.

https://reviewboard.mozilla.org/r/173688/#review179090

Sure
Comment on attachment 8902651 [details]
Bug 1393439 - P4: Modify wpt test to verify only the opaque response with the empty string will pass the SRI check.

https://reviewboard.mozilla.org/r/174318/#review179522

::: testing/web-platform/tests/fetch/api/basic/integrity.js:6
(Diff revision 1)
>  if (this.document === undefined) {
>    importScripts("/resources/testharness.js");
>    importScripts("../resources/utils.js");
>  }
>  
> -function integrity(desc, url, integrity, shouldPass) {
> +function integrity(desc, url, integrity, isNoCorsMode, shouldPass) {

I think this would be clearer if you passed the mode as a string.

::: testing/web-platform/tests/fetch/api/basic/integrity.js:15
(Diff revision 1)
> -   if (shouldPass) {
> +  if (shouldPass) {
>      promise_test(function(test) {
> -      return fetch(url, {'integrity': integrity}).then(function(resp) {
> +      return fetchPromise.then(function(resp) {
> +        if (isNoCorsMode) {
> +          assert_equals(resp.status, 0, "Opaque response's status is 0");
> +          assert_equals(resp.type, "opaque");

Do we really need this case here?  The no-cors mode should reject.
Attachment #8902651 - Flags: review?(bkelly) → review+
Comment on attachment 8902651 [details]
Bug 1393439 - P4: Modify wpt test to verify only the opaque response with the empty string will pass the SRI check.

https://reviewboard.mozilla.org/r/174318/#review179522

> I think this would be clearer if you passed the mode as a string.

Sure.

> Do we really need this case here?  The no-cors mode should reject.

The no-cors mode with the empty integrity will pass the SRI check and be resolved, so that it will run into this check. Do I misunderstad anything or we need to avoid this from happening?
Addressed the first issue.
> The no-cors mode with the empty integrity will pass the SRI check and be resolved, so that it will run into this check. Do I misunderstad anything or we need to avoid this from happening?

Indeed, if integrity is the empty string, then no integrity check takes place. That's per step 19 of https://fetch.spec.whatwg.org/#concept-main-fetch. Hope that helps.
(In reply to Anne (:annevk) from comment #33)
> > The no-cors mode with the empty integrity will pass the SRI check and be resolved, so that it will run into this check. Do I misunderstad anything or we need to avoid this from happening?
> 
> Indeed, if integrity is the empty string, then no integrity check takes
> place. That's per step 19 of
> https://fetch.spec.whatwg.org/#concept-main-fetch. Hope that helps.

Thanks for the feedback! That does help. So, it's reasonable to have this check for the request wtih empty integrity string and no-cors mode.
(In reply to Tom Tung [:tt] from comment #34)
> Comment on attachment 8902651 [details]
> Bug 1393439 - P4: Modify wpt test to verify only the opaque response with
> the empty string will pass the SRI check.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/174318/diff/2-3/

Update patch for fixing a nit (checking non-empty string via |!== ""| rather than |!= 0|).
Pushed by ttung@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/76d554bc5e2e
P1: Don't return TypeError for no-cors mode and don't check SRI for the hidden opaque body. r=bkelly
https://hg.mozilla.org/integration/autoland/rev/d579a12e0521
P2: Correct the variable naming. r=bkelly
https://hg.mozilla.org/integration/autoland/rev/d36e0b2393c6
P3: Remove the wpt test for assuming to throw no-cors request with integrity attribute. r=bkelly
https://hg.mozilla.org/integration/autoland/rev/17d94b20daeb
P4: Modify wpt test to verify only the opaque response with the empty string will pass the SRI check. r=bkelly
Hi, I confirm that the problem is fixed on my two websites that are impacted (tested on Firefox Nightly 57.0a1).

Great job, and thanks a lot to all ! :)
(In reply to Nicolas Hoffmann from comment #39)
> Hi, I confirm that the problem is fixed on my two websites that are impacted
> (tested on Firefox Nightly 57.0a1).
> 
> Great job, and thanks a lot to all ! :)

Thanks for verifying!  Just FYI, this should hit release channel around Nov 14.
No longer blocks: 1381841
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: