MSC4149: Update CSP Directives for Media Repository #4149
Conversation
tcpipuk
changed the title
| @@ -0,0 +1,101 @@ | |||
| # MSC4149: Update CSP Directives for Media Repository | |||
There was a problem hiding this comment.
Implementation requirements:
- Clients demonstrating near-zero impact.
- 3D Worlds clients (like Third Room) continue to function.
- Review from the Matrix Security Team (will be requested closer to FCP)
There was a problem hiding this comment.
What sort of client testing would you like to see? I can override the CSP in my reverse proxy to prove certain things work correctly in a number of clients if needed?
I'm not sure how the change from the current recommended CSP would affect Third Room, as it's already set to default-src 'none'; and it's unclear whether it requires CSS? Is there someone with knowledge of Third Room's requirements that could weigh-in? I don't know if I have the necessary hardware/software to test this myself.
There was a problem hiding this comment.
I've been using Element Web, SchildiChat Web, FluffyChat mobile, and Element X iOS with no issues on content rendering, thumbnailing, or downloading with this CSP.
A reverse proxy response header override is definitely the easiest way to test if something like Third Room works, but that may need to be someone else's task on even getting Third Room set up, or safely setting up Wireshark and TLS decryption and overriding/adding HTTP response headers using an existing public Third Room instance (I think they're called?).
There was a problem hiding this comment.
This sort of client testing would ideally come from the developers themselves as they will be more aware of the nuances for their users. For Third Room, considering the development team is largely disbanded, the scene/world should load without issue.
There was a problem hiding this comment.
Having discussed with Nico (of Nheko) it appears he doesn't embed a web browser, and the client downloads other media to play locally, so doesn't use/respect CSPs for media requests.
I assume it will be a similar story on Android/iOS apps, so testing will primarily need to focus on web-based/Electron clients.
turt2live
added
proposal
| [MSC2702](https://github.com/matrix-org/matrix-doc/pull/2702) explicitly disallows PDFs, making | ||
| this directive unnecessary and potentially misleading. | ||
|
|
||
| #### Remove `style-src 'unsafe-inline';` |
There was a problem hiding this comment.
I have a nagging feeling that this was included for a good reason - perhaps the ability to share all-in-one HTML files and have the style render correctly when viewed in browser (rather than having to mess around uploading the CSS for the HTML to a separate repo location, and then link to it, when sharing HTML)? But this is a wild guess; i can't remember the specifics.
There was a problem hiding this comment.
I believe it was for when inline HTML was allowed before MSC2702, in general: #4149 (comment)
There was a problem hiding this comment.
sounds plausible. i think the motivation here was to ensure that HTML and PDFs loaded straight into browser tabs from web clients (rather than ending up annoying saved to disk, and having to be manually viewed from there). However, the shift away from web clients to Electron etc makes that much more moot.
There was a problem hiding this comment.
As the recommended CSP in the spec, I'd assume we want to recommend people can't load AIO HTML files directly, but that server administrators might want to choose to ignore the recommended CSP if they have specific requirements?
There was a problem hiding this comment.
In the current case, someone can upload a HTML file that uses formatting tags, but should we be allowing the full gamut of CSS just in case someone wants to view full HTML pages directly from the media repo?
There was a problem hiding this comment.
somewhat related: previews like that are going to break with MSC3916 (unless clients invent local proxies using service workers)
There was a problem hiding this comment.
Just added a comment to this effect at the bottom, but is this MSC considered redundant now? I'm not aware of a timeline for the deprecated endpoints to be retired yet, so should we look to get this in place to secure them in the meantime?
|
|
||
| #### Remove `plugin-types application/pdf;` | ||
|
|
||
| Modern browsers no longer use the `plugin-types` directive. It was originally intended for use with |
There was a problem hiding this comment.
"Modern browsers" feels like it's answering a question we haven't asked in the spec yet: at what point do we consider a client "unsupported"? Currently, the CSP is intended to protect old Internet Explorer users from various attack vectors, though even with that there's still holes. Should we be dropping support for IE?
There was a problem hiding this comment.
I feel like if Internet Explorer support was a target demographic, there would be recommendations of some more ancient headers like X-Download-Options,X-Permitted-Cross-Domain-Policies, and X-XSS-Protection: 1; mode=block (causes vulnerabilities in modern browsers).
Though, my honest answer to this:
Should we be dropping support for IE?
Is yes. Microsoft has dropped support for it in 2022 and perhaps when a vendor/OEM drops general public support for it, that should be a hard limit on when a browser or client is officially considered "unsupported". Other factors could be argued, but if the developers themselves say it's unsupported then it's unsupported full stop.
There was a problem hiding this comment.
Given that Microsoft ended support for IE in June 2022 I'd say it's reasonable to deprecate support for a browser that hasn't had security updates in two years.
I'm not sure it's a question that needs to be answered during this MSC though - the current CSP already blocks scripts, for example. We're simply saying if scripts are considered unsafe in uploaded media, then it'd be safer to include plugins, CSS, and fonts in that too
There was a problem hiding this comment.
The question is more about what the protocol's position is on supporting things like IE. That conversation doesn't need to happen here on the MSC, but I think it does block the MSC.
There was a problem hiding this comment.
Understood. How should this be raised? Is this something Josh and/or the governing board should discuss, or something that would be agreed within the SCT? What can I do to help that conversation happen?
There was a problem hiding this comment.
This thread is currently hosting the conversation - leaving it here is best for now in my opinion, though opening an issue on matrix-spec and linking it in this thread would also be fine.
The SCT will provide the bulk of the opinion, though will be shaped by the community's response as well. This should happen as part of normal MSC review.
There was a problem hiding this comment.
Just for reference when the discussions happen here:
The spec recommended ("should") CORS header Cross-Origin-Resource-Policy on the media content repo is not an IE-supported header.
Additionally, CSP on IE is only supported by the legacy X-Content-Security-Policy header. So the existing CSP would not do anything on Internet Explorer as they would need to send the X- prefixed header.
https://en.wikipedia.org/wiki/Content_Security_Policy#Status
Internet Explorer 10 and Internet Explorer 11 also support CSP, but only sandbox directive, using the experimental X-Content-Security-Policy header.
|
Is this MSC considered redundant now MSC3916 has been merged, or should we consider progressing as servers still support the deprecated unauthenticated media endpoints? |
|
The authenticated endpoints could still end up needing a CSP depending on how they're used - updating it doesn't seem like a bad idea. |
|
(please also avoid force-pushing after review has been given. It makes it harder to see what's changed, and we squash commits anyways) |
Rendered
Signed-off-by: Tom Foster [email protected]