(In reply to Anne (:annevk) from bug 1735923 comment 7)

1. Jake's fundamental fix is essentially not allowing mixing of non-opaque and opaque responses in media elements.

Bug 1762068 covers a current exposure there.

Arguably one problem here is that the cross-origin request is still made. It seems like it should be possible to network error earlier when we detect a cross-origin request after we've started using same-origin responses.

That's this bug.

2. There's another solution to this problem outside of media elements, which is that we fix Resource Timing along the lines suggested in https://github.com/w3c/resource-timing/issues/165,

Firefox already adds ResourceTiming entries for both 206 and 416 responses.

but it's not clear to me that it could not be exploited in other ways. E.g., it seems timing the cross-origin response might also give the attacker some information about whether it's a 206 or 416.

This bug.

(There's also https://github.com/whatwg/fetch/pull/1311 about exposing network errors to Resource Timing as well [...].)

Anne, if bug 1762068 were addressed, then would the remaining known/expected issue be only the timing of responses leaking information about the resource, such as its length?

This issue exists to some extent to without ServiceWorker and just the no-cors media element request, where timing might provide an estimate of length or existence.

Do you have any more details of a solution in mind that you could fill in here?

Once a service worker synthesizes a response to cross-origin request, were you thinking along the lines of one of these?

1. The media element would then no longer send cross origin requests.
   The ServiceWorker would have no way to complete the response.
   (The ServiceWorker could still cache cross-origin responses, with support from a fix to bug 1465074, and respond with those, if it does not synthesize any response.)
2. fetch() would then network error before sending requests in response to the cross-origin FetchEvent, and HTTP fetch error on a null response from FetchEvent.
   The first part at least might be tricky given responses are async, but perhaps the Request could be neutered so that it (or its clone) could never be used to generate a Response.
   The ServiceWorker has the opportunity to synthesize another response.
   Presumably (possibly cached) cross-origin responses would continue to be rejected by media element (bug 1762068).

Our defense model for passing Range requests to ServiceWorker seems largely based on assuming media element is not going to make requests that might leak too much information.
Allowing the ServiceWorker to synthesize a response makes this easier to defeat.

A ServiceWorker synthesized response to a cross-origin request feels like an unauthorized redirect. Is this usually OK because the ServiceWorker is always same-origin with the entity making the request?

For option 1 (and bug 1762068 comment 2), is there a good way for media element to identify a synthesized response?

I think if bug 1762068 were addressed, not making the request at all would mainly be defense-in-depth. Attackers can create such requests although until bug 1733981 is addressed this might be harder in Firefox.

The model I have in mind for media elements is that they make an initial request and then multiple subsequent requests. The final redirected URL of the initial request is what is used by subsequent requests. (This matches Chrome as far as I can tell.) If subsequent requests redirect, terminate. (This used to match Chrome, but they changed things. Reportedly they're flexible on this though.)

That's how you'd do requests. Then for responses you'd keep track of their origin and the moment you get a response from a different origin you'd terminate.

Upon reflection though it seems hard to prevent a "malicious" request being made. If the attacker always supplies synthetic responses for a cross-origin URL, the request would go out to the service worker which could then forward it. As you mention in comment 1 that would get quite messy to avoid and it's not clear it would even stick given that we are making the Range header available to attackers.

The only other thing I can think of is that synthetic responses would not be allowed for "no-cors" media elements that make requests across the origin boundary. You could still use cached responses however assuming the URLs match up. It's not clear how web compatible that would be.

What do you think?

(In reply to Anne (:annevk) from comment #3)

The model I have in mind for media elements is that they make an initial request and then multiple subsequent requests. The final redirected URL of the initial request is what is used by subsequent requests. (This matches Chrome as far as I can tell.)

This is somewhat consistent with Gecko behavior, but the final redirected path of each subsequent request is used for its next request.

If subsequent requests redirect, terminate. (This used to match Chrome, but they changed things. Reportedly they're flexible on this though.)

That's how you'd do requests. Then for responses you'd keep track of their origin and the moment you get a response from a different origin you'd terminate.

In Gecko, subsequent responses will terminate only when origins differ.

If the cross-origin server (rather than the service worker) chooses to redirect to a different path but same (cross-)origin, then I'm not aware of a problem with that.

I'm uncomfortable with allowing service workers to coerce a mixture of same-cross-origin but different-path opaque responses by emulating a path redirect through returning an opaque response from a fetch with different path.
A cached response from a ServiceWorker with a different path would currently be treated by media element like a redirect.
AFAIK that is not possible to abuse until ServiceWorker can pass on the Range headers (bug 1465074), but I haven't investigated whether a 200 OK response to a different-path request would overwrite the media resource from a 206 Partial Content initial response or whether it might lead to problematic mixing of different opaque resources.

(In reply to Anne (:annevk) from comment #3)

The only other thing I can think of is that synthetic responses would not be allowed for "no-cors" media elements that make requests across the origin boundary. You could still use cached responses however assuming the URLs match up. It's not clear how web compatible that would be.

Media elements have their own fallback mechanisms for network failures (Source child elements and/or MediaSource), so I don't know of a good additional use case for synthetic responses for cross-origin "no-cors" requests.

The key question is web-compatibility, and I don't know the answer to that.

As for the benefits of disallowing such responses, the area of risk addressed would be in preventing use of synthesized responses to make media elements generate specific no-cors Range requests, which might not otherwise be available but could then be passed on to fetch() when we add support for passing on Range headers (bug 1465074).

It would not address either of these risks:

• Unsanctioned cross-path redirects mixing same-cross-origin opaque resources.
• Sending media element requests to ServiceWorker is exposing a lower bound on the length of the opaque cross-origin resource. Addressing this would require that no-cors cross-origin requests (or at least parts of them) are not readable by ServiceWorker.

(In reply to Karl Tomlinson (:karlt) from comment #5)

As for the benefits of disallowing such responses, the area of risk addressed would be in preventing use of synthesized responses to make media elements generate specific no-cors Range requests, which might not otherwise be available but could then be passed on to fetch() when we add support for passing on Range headers (bug 1465074).

I considered verifying Partial Content response byte ranges against the requested ranges as an alternative solution with the same benefits. To be effective, however, the body provided would also need to match up, but erroring on an incomplete stream could be problematic, as we intentionally don't do that currently and restarts on incomplete bodies were explicitly added despite questionable validity of such responses.

IIUC a synthetic response from the Response constructor is not a filtered response.
The main fetch algorithm says that when 'request’s mode is "no-cors"' and noCorsResponse is not a filtered response, the algorithm should "Return a new response whose status is noCorsResponse’s status."
Such a response would have no body or headers, and so such synthetic responses are already effectively blocked by the spec (unless I've missed something that would turn the response into a filtered response).
That's not how Firefox behaves though, nor Chrome.

I am not sure if my understanding of the above discussion is correct.

It seems that the media element could send a series range of requests that request same-cross-origin resources. And these requests could be intercepted by a ServiceWorker.
When the cross-origin resource is requested, we should return a NetworkError response for such cross-origin resources, since the opaque and non-opaque mixed response is not allowed according to the first point of https://bugzilla.mozilla.org/show_bug.cgi?id=1735923#c7.

And we have a patch https://phabricator.services.mozilla.com/D143865 which is handled in the media element.

However, it could return NetworkError much earlier in ServiceWorker code. Two options are proposed by Karl in comment 1.

But the statement in comment 3 "That's how you'd do requests. Then for responses you'd keep track of their origin and the moment you get a response from a different origin you'd terminate." confuses me a little bit where should this be done? in ServiceWorker code or Media element code?
We probably can decide if should we return NetworkError when by checking the response sent to FetchEvent.respondWith, but is it the correct point to handle this? Because it would become a special case response handling in ServiceWorker.

Hi Eden, any progress here? Thanks!

The discussion here has not yet converged on a clear plan. There are risks (comment 3) but actual likely exploits are not identified. The risks should at least be reconsidered before addressing bug 1465074 and bug 1733981, but applying mitigations now might be easier than after more sites might depend on the current behavior.

The most effective reduction in risks would be to skip passing no-cors cross-origin range requests to ServiceWorker at all.
Perhaps trying that on nightly might be a first step here?

(In reply to Karl Tomlinson (back July 25 :karlt) from comment #10)

The most effective reduction in risks would be to skip passing no-cors cross-origin range requests to ServiceWorker at all.
Perhaps trying that on nightly might be a first step here?

Going to implement this.

Hi Andrew, any news here? Thank you!

Landed: https://hg.mozilla.org/integration/autoland/rev/822f257fe8707522d26a810d194d13a99aed362e

Backed out for causing wpt failures related to service workers:
https://hg.mozilla.org/integration/autoland/rev/dcd1d755604f367a20c90958be67328a1a70ed2b

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=BKKvwENFSiKQty2eQZcDXQ.0&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&revision=822f257fe8707522d26a810d194d13a99aed362e
Failure log: https://treeherder.mozilla.org/logviewer?job_id=392813804&repo=autoland

TEST-UNEXPECTED-TIMEOUT | /fetch/range/sw.https.window.html | Defer range header filter tests to service worker - Test timed out
followed by more failure lines

This passes for me:
$ ./mach wpt testing/web-platform/tests/content-security-policy/gen/top.meta/worker-src-self/sharedworker-classic.https.html

Unfortunately tests passing locally but not in CI is common. :aryx did the backout as a code sheriff per policy and it's on us to dig into figuring out why the failure happened (or unexpected successes! WPT tests can get in general if the expectations don't match with what happens.) Setting a needinfo on me to try and provide some context on this.

The test failed on all platforms except on Windows debug and Linux asan opt because it doesn't run for the latter.

Blocking SerivceWorker interception for no_cors cross-origin range request. r=dom-worker-reviewers,karlt
https://hg.mozilla.org/integration/autoland/rev/47a7f8cc08213c60a6fc0c294c18b803fa69d2aa
https://hg.mozilla.org/mozilla-central/rev/47a7f8cc0821

This is rated sec-high and AFAICT affects more than just Nightly. As such, it should have gone through the sec-approval process before landing.
https://firefox-source-docs.mozilla.org/bug-mgmt/processes/security-approval.html

Please fill out the form retroactively so we can assess what needs to happen here for the branches.

Comment on attachment 9276874 [details]
Bug 1762078 - Blocking SerivceWorker interception for no_cors cross-origin range request. r=#dom-worker-reviewers

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easily. The concern is that the timing of ResourceTiming entries (influenced by a nefarious service worker) could be used to determine the size of an opaque cross-origin resource. But this patch only prevents service workers from intercepting the request--timing isn't mentioned.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?:
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: This change should be easy to backport to any revision needing it.
• How likely is this patch to cause regressions; how much testing does it need?:
• Is Android affected?: Yes

Comment on attachment 9276874 [details]
Bug 1762078 - Blocking SerivceWorker interception for no_cors cross-origin range request. r=#dom-worker-reviewers

So we've landed the patch and some test annotations and e want to land some test updates that show the interception doesn't work but doesn't explain why that's important. We can land those tests now, and uplift everything when ready.

Please nominate this for Beta and ESR102 approval when you get a chance.

Comment on attachment 9276874 [details]
Bug 1762078 - Blocking SerivceWorker interception for no_cors cross-origin range request. r=#dom-worker-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: Possible security hole involving resource timing, allowing an attacker to determine the length of opaque cross-origin media elements. (Test is https://phabricator.services.mozilla.com/D159315, landing shortly.)
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change just prevents service workers from intercepting cross-origin no-cors media requests. I believe the risk is low.
• String changes made/needed:
• Is Android affected?: Yes

ESR Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]:
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]:
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]:

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment on attachment 9301605 [details] [diff] [review]
Patch for esr102

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Possible security hole involving resource timing, allowing an attacker to determine the length of opaque cross-origin media elements. (Test is https://phabricator.services.mozilla.com/D159315, landing shortly.)
• Fix Landed on Version: 77cf4980fc9f
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change just prevents service workers from intercepting cross-origin no-cors media requests. I believe the risk is low.

Comment on attachment 9276874 [details]
Bug 1762078 - Blocking SerivceWorker interception for no_cors cross-origin range request. r=#dom-worker-reviewers

Approved for 107.0b9

https://hg.mozilla.org/releases/mozilla-beta/rev/de41ff951322

https://bugzilla.mozilla.org/attachment.cgi?id=9298501 is still pending landing in central. Required before uplifting to beta.

Comment on attachment 9301605 [details] [diff] [review]
Patch for esr102

Approved for 102.5esr.

https://hg.mozilla.org/releases/mozilla-esr102/rev/3589b2870511

Landed: https://hg.mozilla.org/integration/autoland/rev/1840f6c848d9db0d801094b3809d9d77c6dbf469

Backed out from autoland for linting failures: https://hg.mozilla.org/integration/autoland/rev/aace7a9cb4c0c2e34b17ddb9efaa7f50dfa7f27f

Push with failures (black and flake8): https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=WrdbyU81QRaMSc0jr7G7jw.0&resultStatus=retry%2Ctestfailed%2Cbusted%2Cexception%2Cusercancel&revision=1840f6c848d9db0d801094b3809d9d77c6dbf469

Depends on D159315

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

(In reply to Release mgmt bot [:suhaib / :marco/ :calixte] from comment #35)

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

Should we exclude security bugs from this nag? It is a common and wanted pattern to fix it first and make/land a test later for security bugs.

I checked the numbers and it seems that the tool commented on 5 security bugs since it was added (i.e., April 2022).

Previously, :dholbert suggested softening the comment language (which we did in #1538), but if you think this is not enough, we code go further and exclude cases where the patch is only touching test files (Marco's suggestion in #1538). If this is still not enough, we could consider disabling these comments for security bugs.

:jstutte, please let me know what you think.

Depends on D159315

Ensure service workers don't intercept crossorigin media requests r=edenchuang
https://hg.mozilla.org/integration/autoland/rev/510df507ca0e92605a0682e94e52f85e5e40f186
https://hg.mozilla.org/mozilla-central/rev/510df507ca0e

Comment on attachment 9298501 [details]
Bug 1762078 - Ensure service workers don't intercept crossorigin media requests r=edenchuang!

Switching beta uplift request to release, 107 is now in RC.
Keeping on the radar for potential uplift later for a ride-along.

(In reply to Suhaib Mujahid [:suhaib] from comment #37)

I checked the numbers and it seems that the tool commented on 5 security bugs since it was added (i.e., April 2022).

Previously, :dholbert suggested softening the comment language (which we did in #1538), but if you think this is not enough, we code go further and exclude cases where the patch is only touching test files (Marco's suggestion in #1538). If this is still not enough, we could consider disabling these comments for security bugs.

:jstutte, please let me know what you think.

Thanks for looking into this! I think, Marco's suggestion would be fine if we could extend it also to uplifts (sometimes we need to rebase and create a new attachment). The sequence of events on closing security bugs can be slightly confusing for a bot, though. So maybe having just a hint in the comment (at least if it is subject to sec-approval) that we can ignore the message in this and that case would be fine, too.

(clearing the old ni?s as a drive by)

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

So maybe having just a hint in the comment (at least if it is subject to sec-approval) that we can ignore the message in this and that case would be fine, too.

We have the following hint in the bot's comment:

if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message

What would you suggest to improve the hint?

(In reply to Suhaib Mujahid [:suhaib] from comment #42)

We have the following hint in the bot's comment:

if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message

What would you suggest to improve the hint?

I'd propose a slightly different message for bugs with sec-approval (maybe with a link to that process to make clear that this does not override it), just to show to the reader that we know about that case. A nag should help to re-enforce an existing process, without that specificity it might just feel "wrong" to nag at all to the reader and one must spend some extra cycles to understand "do I need to take action here or not?". But it is just a nuance, of course. Thanks

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

I'd propose a slightly different message for bugs with sec-approval (maybe with a link to that process to make clear that this does not override it), just to show to the reader that we know about that case.

I filed an issue to follow up on this: https://github.com/mozilla/relman-auto-nag/issues/1748

A nag should help to re-enforce an existing process, without that specificity it might just feel "wrong" to nag at all to the reader and one must spend some extra cycles to understand "do I need to take action here or not?". But it is just a nuance, of course. Thanks

I totally agree with you.

It would be great if you could double check this description.

(In reply to Tom Ritter [:tjr] from comment #45)

It would be great if you could double check this description.

That's probably close enough. At least a lower bound on the size/length of cross-origin resources could sometimes be leaked, even without timing information. Whether timing information can make that information more precise hasn't been demonstrated.
Consider s/could/might/g.

Comment on attachment 9298501 [details]
Bug 1762078 - Ensure service workers don't intercept crossorigin media requests r=edenchuang!

Approved for 107.0.1

https://hg.mozilla.org/releases/mozilla-release/rev/b07dc4ddfbc7