Specify `Content-Type` and `Content-Disposition` usage in the media repo

[zecakeh]

Supersedes #1758.

As per MSC2701 and MSC2702.

Pull Request Checklist

• [x] Pull request includes a changelog file
• [x] Pull request includes a sign off
• [ ] Pull request is classified as 'other changes'

Preview: https://pr1935--matrix-spec-previews.netlify.app

[zecakeh]

I am not sure how to write up https://github.com/matrix-org/matrix-spec/pull/1758#issuecomment-2138035891 as I don't know where it's explained why it is safe.

[tulir]

My understanding on that topic is:

• When a Content-Type header is provided, browsers and such will only render the data using that type. For example, if you serve a HTML file with image/png, browsers will just say it's corrupted, they won't render it as a HTML page. Therefore, there's no risk of trusting the user-defined content type, as long as the Content-Disposition is calculated based on the same type.
• If you were to calculate Content-Disposition with a different type than what is served to downloaders in the Content-Type header, it may open up vulnerabilities: for example, if someone manages to craft a HTML file which your sniffer thinks is a PNG, you'd sniff that it's safe to send as inline. If you then serve that with the uploader's text/html type and inline disposition based on the sniffed content type, browsers will render it as HTML. As long as the Content-Disposition is derived from the type served to clients, it's safe.

[Benjamin-L]

edit: I wrote this and submitted it before seeing tulir's comment. We're saying effectively the same thing, and their explanation is clearer

I am not sure how to write up #1758 (comment) as I don't know where it's explained why it is safe.

I don't know of a place where it's written down yet, but have had this conversation on matrix several times with different people. I believe the reason it's safe is that it should not be possible for a file with a Content-Type from the allow-list to trigger XSS, even with Content-Disposition: inline and with arbitrary content.

When I've seem claims that it is not safe, usually these are thinking about a case where a malicious client uploads a XSS-triggering file and then sets the incorrect Content-Type, causing the server to serve Content-Disposition: inline. For example, uploading an SVG containing a script element while setting Content-Type: image/png. This case is, in fact, safe, because the server will also send Content-Type: image/png to the browser and the browser will not interpret the file as an svg.

The thing that is actually unsafe is mixing and matching between the user-provided Content-Type and a sniffed content type. The Content-Disposition value must be chosen based on the same content type used in the Content-Type header. If the server uses a sniffed value for Content-Type in the response but chooses Content-Disposition based on the user-provided content type or vice versa, it allows XSS.

[Benjamin-L]

This PR doesn't change the Content-Disposition requirements for the federated media endpoints. I assume this is intentional, because the client HS should just treat the value as untrusted anyway and base the disposition on the returned type instead?

[zecakeh]

This PR doesn't change the Content-Disposition requirements for the federated media endpoints. I assume this is intentional, because the client HS should just treat the value as untrusted anyway and base the disposition on the returned type instead?

The reason that I didn't change it is because the description is vague and says:

[…] using Content-Type and Content-Disposition headers as appropriate;

I took that as "use the same rules for those headers as the CS API", but maybe it needs to be spelled out. In any case the SS endpoint should return the filename in Content-Disposition if there is one, so the client HS can reuse it on the /download endpoint.

[Benjamin-L]

I'd definitely be in favor of either explicitly stating that the requirements are the same as the CS API or explicitly stating that they are not. If the filename is the only thing that matters the spec should state that so that implementers aren't guessing whether the vaugeness was a mistake.

[zecakeh]

Thanks to the both of you, I whipped-up a paragraph based on your explications.