Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Specify range fetching for media #7655

Merged
merged 10 commits into from
Mar 31, 2022
Merged

Specify range fetching for media #7655

merged 10 commits into from
Mar 31, 2022

Conversation

noamr
Copy link
Contributor

@noamr noamr commented Feb 23, 2022

(See WHATWG Working Mode: Changes for more details.)


/infrastructure.html ( diff )
/media.html ( diff )

@noamr
Copy link
Contributor Author

noamr commented Feb 23, 2022

This PR attempts to resolve #2814 (comment).

It has several differences from the original PR. The key difference is that multiple opaque range responses are OK as long as they're from the same origin - they don't have to be the same URL. The key verification point is that opaque range responses are only OK if the media resource had contributions from only one origin (programmatic SW responses are considered a null origin).

It's ready for initial feedback to see if stakeholders are aligned before I spend the time required on WPTs.

@noamr noamr changed the title Rigorous fetching of media requests Specify range fetching for media Feb 23, 2022
@noamr
Copy link
Contributor Author

noamr commented Feb 23, 2022

/FYI @annevk @jakearchibald @yoavweiss @domenic (please CC whoever else is interested in this)

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a promising start!

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@domenic
Copy link
Member

domenic commented Feb 27, 2022

Note that this does no Resource Timing for blob-derived (i.e. "local") media resource fetches. Is that what we want / do we have tests for that?

@noamr
Copy link
Contributor Author

noamr commented Feb 28, 2022

Note that this does no Resource Timing for blob-derived (i.e. "local") media resource fetches. Is that what we want / do we have tests for that?

Regardless of media, blobs don't get resource timing entries in general - only HTTP(s). I can add a test for that but it's not a specific/novel thing.

@noamr
Copy link
Contributor Author

noamr commented Mar 14, 2022

@domenic additional review pass ? :)

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@noamr
Copy link
Contributor Author

noamr commented Mar 22, 2022

@domenic additional review pass ? :)

^^^

@domenic
Copy link
Member

domenic commented Mar 22, 2022

There are still three outstanding review comments.

@noamr
Copy link
Contributor Author

noamr commented Mar 22, 2022

There are still three outstanding review comments.

Oh I see now, missed them for some reason. Sorry for the noise.

@noamr
Copy link
Contributor Author

noamr commented Mar 22, 2022

There are still three outstanding review comments.

I found two and fixed them. Did I miss anything?

@domenic domenic added the needs tests Moving the issue forward requires someone to write tests label Mar 22, 2022
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Let us know when the template is filled out, including tests.

noamr added a commit to web-platform-tests/wpt that referenced this pull request Mar 24, 2022
See whatwg/html#7655

When loading video from multiple opaque origins (by a middleman service-worker),
video loading should fail rather than be alllowed and taint the canvas.

That's because some of the video responses may contain metadata such as duration that
would leak to the subsequent requests.

See whatwg/html#2814 (comment) for further details.

This change makes the test case pass in all browsers.
noamr added a commit to web-platform-tests/wpt that referenced this pull request Mar 24, 2022
See whatwg/html#7655

When loading video from multiple opaque origins (by a middleman service-worker),
video loading should fail rather than be alllowed and taint the canvas.

That's because some of the video responses may contain metadata such as duration that
would leak to the subsequent requests.

See whatwg/html#2814 (comment) for further details.

This change makes the test case pass in all browsers.
@karlt
Copy link

karlt commented Mar 28, 2022

The key difference is that multiple opaque range responses are OK as long as they're from the same origin - they don't have to be the same URL.

Can you elaborate on the reasons for this change in approach please?

If an attacker knows the content of some of the files from the origin being attacked, then it would seem plausible that they might be able to piece together leading bytes of their choice and so still succeed in the attacks that the URL check was intended to prevent.

@noamr
Copy link
Contributor Author

noamr commented Mar 28, 2022

The key difference is that multiple opaque range responses are OK as long as they're from the same origin - they don't have to be the same URL.

Can you elaborate on the reasons for this change in approach please?

If an attacker knows the content of some of the files from the origin being attacked, then it would seem plausible that they might be able to piece together leading bytes of their choice and so still succeed in the attacks that the URL check was intended to prevent.

This use-case remains not allowed in this PR.

What's allowed is piecing together pieces from different URLs of the same origin.
For example, the first media request redirects you to https://origin-a.com/media1.mp4 and the second media request redirects you to https://origin-a.com/media2.mp4.

Since the security issue here is a cross-origin, the checks of whether two media responses can be pieced together are same-origin checks rather than URL equality checks.

noamr added a commit to web-platform-tests/wpt that referenced this pull request Mar 29, 2022
* Add two cases to preload SRI

* Add test for stronger/weaker digest

* Fix expected results for video loading from multiple origins

See whatwg/html#7655

When loading video from multiple opaque origins (by a middleman service-worker),
video loading should fail rather than be alllowed and taint the canvas.

That's because some of the video responses may contain metadata such as duration that
would leak to the subsequent requests.

See whatwg/html#2814 (comment) for further details.

This change makes the test case pass in all browsers.

* Add another case
@domenic domenic removed the needs tests Moving the issue forward requires someone to write tests label Mar 31, 2022
@domenic domenic merged commit f495c02 into whatwg:main Mar 31, 2022
Copy link

@karlt karlt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I missed that these questions were not submitted with my other comment a few days ago.

source Outdated
Comment on lines 34597 to 34598
<li><p>If <var>internalResponse</var>'s <span data-x="concept-response-status">status</span> is
not 206, then return false.</p></li>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because A server MAY ignore the Range header field, a 200 (OK) response would be a valid response.
Is a 200 response intentionally rejected here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I missed that these questions were not submitted with my other comment a few days ago.

Thanks for the notes! I'll check (probably next week) how implementations currently handle these cases and submit WPT/spec PRs accordingly.

source Outdated
Comment on lines 34637 to 34638
<var>byteRange</var>[1] is neither "<code data-x="">until end</code>" or <var>end</var>,
return false.</p></li>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the requirement that end match byteRange[1] when not "until end" necessary?
It seems inconsistent with allowing a truncated response with "until end".

The 206 (Partial Content) status code indicates that the server is successfully fulfilling a range request for the target resource by transferring one or more parts of the selected representation that correspond to the satisfiable ranges found in the request's Range header field (Section 3.1).. i.e. I'm not aware of any requirement that a response should include the entirety of a requested range.

There is precendent for expecting a response containing only a portion of the requested range to be aligned with the start of the requested range, so I guess that would be less problematic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently implementations today don't verify that the request range matches the response range.
Added a PR to remove that verification.

Also, Gecko & Chromium allow 200 responses for range requests. Added that to the PR. It's a known bug in Safari.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Apr 2, 2022
Automatic update from web-platform-tests
Add a few cases to preload SRI (#33326)

* Add two cases to preload SRI

* Add test for stronger/weaker digest

* Fix expected results for video loading from multiple origins

See whatwg/html#7655

When loading video from multiple opaque origins (by a middleman service-worker),
video loading should fail rather than be alllowed and taint the canvas.

That's because some of the video responses may contain metadata such as duration that
would leak to the subsequent requests.

See whatwg/html#2814 (comment) for further details.

This change makes the test case pass in all browsers.

* Add another case
--

wpt-commits: 6a214e8155265b0c4e13471302d35f27386ef550
wpt-pr: 33326
noamr added a commit to noamr/html that referenced this pull request Apr 4, 2022
User-agents don't actually perform these verifications.
The only important verification is origin of responses.

See whatwg#7655 (comment)
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Apr 5, 2022
Automatic update from web-platform-tests
Add a few cases to preload SRI (#33326)

* Add two cases to preload SRI

* Add test for stronger/weaker digest

* Fix expected results for video loading from multiple origins

See whatwg/html#7655

When loading video from multiple opaque origins (by a middleman service-worker),
video loading should fail rather than be alllowed and taint the canvas.

That's because some of the video responses may contain metadata such as duration that
would leak to the subsequent requests.

See whatwg/html#2814 (comment) for further details.

This change makes the test case pass in all browsers.

* Add another case
--

wpt-commits: 6a214e8155265b0c4e13471302d35f27386ef550
wpt-pr: 33326
noamr added a commit to noamr/html that referenced this pull request Apr 11, 2022
Verifying media responses performs 3 verifications:
* The response should also be a range response
* The response's range should match the request's range
* There should not be mixed origins with opaque response

While the last verification is mandatory for same-origin policy, the first two
are optional, and may be skipped if the user-agent decides to be lenient towards
servers who don't handle range requests properly.

See whatwg#7655 (comment)
noamr added a commit to noamr/html that referenced this pull request Apr 22, 2022
Verifying media responses performs 3 verifications:
* The response should also be a range response
* The response's range should match the request's range
* There should not be mixed origins with opaque response

While the last verification is mandatory for same-origin policy, the first two
are optional, and may be skipped if the user-agent decides to be lenient towards
servers who don't handle range requests properly.

See whatwg#7655 (comment)
noamr added a commit to noamr/html that referenced this pull request May 4, 2022
Verifying media responses performs 3 verifications:
* The response should also be a range response
* The response's range should match the request's range
* There should not be mixed origins with opaque response

While the last verification is mandatory for same-origin policy, the first two
are optional, and may be skipped if the user-agent decides to be lenient towards
servers who don't handle range requests properly.

See whatwg#7655 (comment)
domenic pushed a commit that referenced this pull request May 23, 2022
f495c02 added some verification to media range responses, but in reality this verification is not present in most implementations. See discussion in #7655 (comment).
mfreed7 pushed a commit to mfreed7/html that referenced this pull request Jun 3, 2022
f495c02 added some verification to media range responses, but in reality this verification is not present in most implementations. See discussion in whatwg#7655 (comment).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants