Closed Bug 1669355 (CVE-2020-26958) Opened 4 years ago Closed 4 years ago

ServiceWorker intercepted channels don't run main fetch step 12 MIME type checks; nsHttpChannel logic should be moved into HttpBaseChannel

Categories

(Core :: Networking: HTTP, defect, P1)

Product:
Core
Component:
Networking: HTTP
Version:
Firefox 81
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
84 Branch
Tracking Flags:
Tracking Status
firefox-esr78 83+ fixed
firefox82 --- wontfix
firefox83 + fixed
firefox84 + fixed

People

(Reporter: moti, Assigned: asuth)

Details

(Keywords: sec-moderate, Whiteboard: [necko-triaged][adv-main83+][adv-esr78.5+])

Attachments

(4 files, 1 obsolete file)

Bug 1669355 - Refactor MIME type warnings into base class. r=valentin
47 bytes, text/x-phabricator-request
pascalc
: approval-mozilla-beta+
Details | Review
Bug 1669355 - Enhance MIME type validation tests with ServiceWorkers. r=ckerschb!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1669355 - Refactor MIME type warnings into base class. r=valentin
47 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-esr78+
Details | Review
advisory.txt
332 bytes, text/plain
Details

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/85.0.4183.121 Safari/537.36

Steps to reproduce:

  • Set up a simple web server that responds to /aaa.csv with a csv file (content-type: text/csv)
  • Create an HTML page containing <script src='/aaa.csv'></script>
  • FF will pop an error (in console) saying "The resource from '/aaa.csv' was blocked due to MIME type (“text/csv”) mismatch". (that's the desired behavior)

In order to bypass this restriction, set up a service worker "proxy" layer. It will serve the content from a local cache. FF will execute the fetched content (despite the content-type which isn't application/javascript) This effectively opens up a class of vulnerabilities, the most notable that comes to my mind is XSSI (Cross Site Script Inclusion).

HTML Code:
<html><body>
<script>navigator.serviceWorker.register('sw.js');</script>
<script src='/intercept'></script>
</body></html>

SW Code:
self.addEventListener('fetch', (event) => {
const url = 'http://localhost:8081/aaaafffa.csv?11';
if(event.request.url.endsWith('/intercept'))
event.respondWith(
caches.match(event.request)
.then((response) => response ||
fetch(url, {
method : 'GET',
mode : 'no-cors',
credentials: 'include',
headers : {'Content-type':'application/x-www-form-urlencoded'}
})
)
.catch(() => caches.match('offline')),
);
});

Actual results:

JavaScript execution is possible - bypassing script inclusion security restrictions. (This allows a set of XSSI attacks)

Expected results:

JavaScript execution should be blocked due to mismatching MIME type

The tl;dr is that nsHttpChannel the security checks that should happen happen in nsHttpChannel and are contingent on the "destination" of the fetch being script-like. Because ServiceWorker interception is its own channel type that inherits from HttpBaseChannel, not nsHttpChannel, the intercepted subresource request is not subject to the check. However, if the intercepted FetchEvent's Request instance is passed to fetch() to actually go to the network, the destination will be propagated and the checks will be applied. So in the case of a naive pass-through ServiceWorker or naive caching ServiceWorker, the checks remain in effect.

:valentin (and cc :dragana), do you have thoughts on whether the check logic should be pushed into the HttpBaseChannel common class?

Detailed Analysis

Based on my understanding of what's going on:

  • In the normal window ScriptLoader situation described in the bug:
    • An HTTP channel is created with an internal load type of nsIContentPolicy::TYPE_INTERNAL_SCRIPT which corresponds to an external content policy of nsIContentPolicy::TYPE_SCRIPT and also corresponds to a fetch destination of "script" which is RequestDestination::Script for our purposes.
    • nsHttpChannel is responsible for doing any appropriate MIME type checks. In both cases, the check is dependent on the destination of the script.
      1. The "nosniff" case, fetch 3.4.1 seems to be what's being reported here based on the error message matching the nsHttpChannel nosniff check logic. This depends on the server returning a header of X-Content-Type-Options: nosniff
      2. The other check is fetch 2.9 which is implemented by EnsureMIMEOfScript
  • In the Firefox ServiceWorker case:
    • The fetch being issued from the ServiceWorker has no explicit destination so it gets a destination of "" which doesn't induce the check.
    • A naive pass-through fetch(event.request) would have the destination and the check would be maintained.
  • From a spec perspective:

This behavior shouldn't have changed with the introduction of parent intercept.

Flags: needinfo?(valentin.gosu)

(Moving this to necko for now, but this is very much at the intersection of ServiceWorkers and necko.)

Group: firefox-core-security → core-security
Component: Untriaged → Networking: HTTP
Product: Firefox → Core
Group: core-security → network-core-security

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #1)

:valentin (and cc :dragana), do you have thoughts on whether the check logic should be pushed into the HttpBaseChannel common class?

I think that makes sense., and it's the thing we usually do when we discover some properties of nsHttpChannel also apply to other types of channels 🙂

Flags: needinfo?(valentin.gosu)
Summary: Script execution MIME type security restrictions can be bypassed → ServiceWorker intercepted channels don't run main fetch step 12 MIME type checks; nsHttpChannel logic should be moved into HttpBaseChannel

Also, thanks very much for filing this, Moti!

Glad to help :),
Thanks for your quick response!

Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P1

Jens, since this is a P1 bug, could you find someone to work on this?
Thanks.

Severity: -- → S2
Flags: needinfo?(jstutte)
Whiteboard: [necko-triaged]

Christoph: what other DOM: Security type checks might get bypassed in this path?

Flags: needinfo?(ckerschb)

(In reply to Daniel Veditz [:dveditz] from comment #7)

Christoph: what other DOM: Security type checks might get bypassed in this path?

I did an anlysis of nsHTTPChannel and ultimately we should move the following security mechanisms from nsHttpChannel::CallOnStartRequest in the base channel implementation:

  • EnsureMIMEOfScript including the WarnWrongMIMEOfScript, and
  • ProcessXCTO which enfoces `x-content-type-options nosniff.

I am not entirely sure about the actual mime type sniffing code surrounded by if (mLoadFlags & LOAD_CALL_CONTENT_SNIFFERS) {, that probably needs to be evaluated again, but i could imagine that we want to move that as well.

Flags: needinfo?(ckerschb)
Assignee: nobody → bugmail
Status: NEW → ASSIGNED
Flags: needinfo?(jstutte)

Per https://firefox-source-docs.mozilla.org/bug-mgmt/processes/security-approval.html#on-requesting-sec-approval and because this is sec-moderate, I am planning to land the fix (not the tests) after the soft freeze ends and to request beta uplift after letting it bake for a few days on nightly. If there's something else I should do, please let me know.

This had landed: https://hg.mozilla.org/integration/autoland/rev/989a139969993f4f00ada9356a88b84b2e4d4682

Backed out for causing mochitest failures in test_fetch_event.html:

https://hg.mozilla.org/integration/autoland/rev/538e243f07cab5bd2f7db5efdda83d1b7ef6ae85

Push range with failure selected: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&tochange=538e243f07cab5bd2f7db5efdda83d1b7ef6ae85&searchStr=mochitest%2Cplain&fromchange=3bbdaba33c6e1c04cbfd9170ad317e71f88c8535&selectedTaskRun=c_cCbSQVRKKD5kRFAEe3WQ.0
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=319138341&repo=autoland

[task 2020-10-20T13:35:20.244Z] 13:35:20 INFO - TEST-PASS | dom/serviceworkers/test/test_fetch_event.html | stylesheet load should be intercepted
[task 2020-10-20T13:35:20.267Z] 13:35:20 INFO - TEST-INFO | started process screentopng
[task 2020-10-20T13:35:20.384Z] 13:35:20 INFO - *** error running SJS at /builds/worker/workspace/build/tests/mochitest/tests/dom/serviceworkers/test/fetch/interrupt.sjs: 2152398850 on line NaN
[task 2020-10-20T13:35:20.710Z] 13:35:20 INFO - TEST-INFO | screentopng: exit 0
[task 2020-10-20T13:35:20.710Z] 13:35:20 INFO - TEST-UNEXPECTED-FAIL | dom/serviceworkers/test/test_fetch_event.html | worker load should be intercepted
[task 2020-10-20T13:35:20.710Z] 13:35:20 INFO - SimpleTest.ok@SimpleTest/SimpleTest.js:417:16
[task 2020-10-20T13:35:20.710Z] 13:35:20 INFO - testController/p</window.onmessage@dom/serviceworkers/test/test_fetch_event.html:41:13

Flags: needinfo?(bugmail)
Flags: needinfo?(bugmail) → needinfo?(ytausky)

The test that failed was actually incorrect: it had a service worker returning scripts without the Content-Type header, so with the patch it failed. Once I added the correct MIME type the test passes. Fixing the test in the same patch as the actual fix might tip off observers about this issue, so I'm not sure what the best course of action here would be. Maybe fix the test in a separate patch?

Flags: needinfo?(ytausky)

Thanks for the quick investigation! Yes, that sounds good! If you want to file a bug for that and put the patch up, I can review that quickly. Setting needinfo because I find it more helpful with secbugs.

Flags: needinfo?(ytausky)

I opened bug 1673748 for the test fix.

Flags: needinfo?(ytausky)

Thanks! I've approved it and hit the landing button for expediency. Aside: I somehow confused myself into thinking a WPT test had failed, not sure why. Glad it wasn't!

Would it be worth to add a test for the correct failure handling, too? Like the old test but with inverse expectation?

Flags: needinfo?(ytausky)

(In reply to Jens Stutte [:jstutte] from comment #17)

Would it be worth to add a test for the correct failure handling, too? Like the old test but with inverse expectation?

You mean like we should add a test that scripts lacking the correct MIME type are rejected? That's covered by the enhanced test on this bug (that we can't land yet).

Clearing ni since Andrew answered the question.

Flags: needinfo?(ytausky)

https://hg.mozilla.org/integration/autoland/rev/6873589e0ede3c11bc48243be67c3d51e214873f
https://hg.mozilla.org/mozilla-central/rev/6873589e0ede

Group: network-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox84: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 84 Branch

Looks like this grafts cleanly to Beta as-landed. Does this also affect ESR78? If we wanted to uplift there, it'd need a bit of rebasing, however.

status-firefox83: --- → affected
Flags: needinfo?(bugmail)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #21)

Looks like this grafts cleanly to Beta as-landed. Does this also affect ESR78? If we wanted to uplift there, it'd need a bit of rebasing, however.

This affects ESR78, yes. (If only we'd disabled ServiceWorkers on ESR again![1] ;) I think it would make sense to let it bake for a little more to make sure there's no obvious real-world fallout from this, but it does seem desirable to have the spec-correct behavior on ESR78. :ytausky, do you have cycles to do the rebase and check that the (un-landed) test from this bug passes with the rebased fix applied?

Any uplift will also need to take the fix from https://phabricator.services.mozilla.com/D94902 for https://bugzilla.mozilla.org/show_bug.cgi?id=1673748 which is where :ytausky fixed the test that was unhappy and caused this to get backed out the first time. (We landed that separately because otherwise it rather does draw a lot of attention to what's changing, despite this just being sec-med.)

1: This is a joke whose context is that we disabled ServiceWorkers on ESR repeatedly because

Flags: needinfo?(bugmail) → needinfo?(ytausky)

I rebased the patch. How do we handle the test fix from bug 1673748? Should I request an uplift on that bug?

Flags: needinfo?(ytausky)

The patch landed in nightly and beta is affected.
:asuth, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(bugmail)

We can take the test-only change as a ride-along. Note that we're building our final beta tomorrow (Thursday), so we need approval requests ASAP here.

Comment on attachment 9185504 [details]
Bug 1669355 - Refactor MIME type warnings into base class. r=valentin

Beta/Release Uplift Approval Request

  • User impact if declined: This is a sec-moderate bug -- I'm not sure about exploitability.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: Bug 1673748
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This change adds MIME checks to intercepted channels, it seems like a low risk thing.
  • String changes made/needed:
Attachment #9185504 - Flags: approval-mozilla-beta?
Attachment #9185504 - Flags: approval-mozilla-beta?

Comment on attachment 9182268 [details]
Bug 1669355 - Refactor MIME type warnings into base class. r=valentin

Beta/Release Uplift Approval Request

  • User impact if declined: This is a sec-moderate bug -- I'm not sure about exploitability.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: Bug 1673748
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This change adds MIME checks to intercepted channels, it seems like a low risk thing.
  • String changes made/needed:
Attachment #9182268 - Flags: approval-mozilla-beta?

Comment on attachment 9182268 [details]
Bug 1669355 - Refactor MIME type warnings into base class. r=valentin

Approved for our last beta, are you going to request an uplift to ESR as well?

Attachment #9182268 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment on attachment 9185504 [details]
Bug 1669355 - Refactor MIME type warnings into base class. r=valentin

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a relatively simple fix.
  • User impact if declined: I'm not sure about its exploitability.
  • Fix Landed on Version: 84
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This change adds MIME checks to intercepted channels, it seems like a low risk thing.
  • String or UUID changes made by this patch:
Attachment #9185504 - Flags: approval-mozilla-esr78?

Comment on attachment 9185504 [details]
Bug 1669355 - Refactor MIME type warnings into base class. r=valentin

Approved for 78.5esr.

Attachment #9185504 - Flags: approval-mozilla-esr78? → approval-mozilla-esr78+
Flags: needinfo?(bugmail)
Whiteboard: [necko-triaged] → [necko-triaged][adv-main83+]
Attached file advisory.txt (obsolete) —
Attached file advisory.txt —
Attachment #9187092 - Attachment is obsolete: true
Whiteboard: [necko-triaged][adv-main83+] → [necko-triaged][adv-main83+][adv-esr78.5+]
Alias: CVE-2020-26958
Group: core-security-release
Attachment #9182269 - Attachment description: Bug 1669355 - Enhance MIME type validation tests with ServiceWorkers. r=ckerschb → Bug 1669355 - Enhance MIME type validation tests with ServiceWorkers. r=ckerschb!
Pushed by bugmail@asutherland.org: https://hg.mozilla.org/integration/autoland/rev/d151edecc032 Enhance MIME type validation tests with ServiceWorkers. r=ckerschb
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: