-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Replace cross origin renderer (usercontent.riot.im) with something better #6173
Comments
it's a wrapper used to host and display content decrypted from e2e in a dedicated domain to prevent XSS attacks - after all, you don't want someone to send you some HTML+JS over e2e, which you then decrypt in JS in your browser and end up executing when you display it to the user. You could also host your own, as long as it's on a separate domain to your riot-web. in general people don't bother, given it's an entirely trivial static system (a bit like matrix.to), although you might want to if you want to have full control. |
The site
This is then injected into the current site as an (of course, not sandboxed!) iframe in order to show the content in it! And why do you always do so in encrypted chats? This whole thing makes no sense… That is not only basically a backdoor for all self-hosted instances, because in order to get any downloaded image in an e2e chat it contacts a site of yours, which could just serve any JavaScript. Again, the iframe is not sandboxed, so it when the JS is malicious it may do bad things. At the very least you could easily intercept all attachments. To repeat: You could intercept e2e encrypted images as you use a centralized domain! WTF… And again your breaking the whole decentralization here. I thought you are making a decentralized product and not a "we decentralize it, but hide some nefarious requests to centralized domains, so we can still take it over if we want" product. |
Just don't display HTML/JS files and images and stuff can be displayed in img tag where they can do nothing.
BTW currently thanks to the missing sandbox I would not say this protection even works. If one can inject JS then okay, then one can only execute JS in the iframe. That still means that JS can stil do things like redirect the parent(!) website and such stuff. You have to use iframes properly. |
Okay, good that I can self-host it, but that is still useless. An iframe without sandbox attribute does not help much. Also why do you only iframe e2e encrypted content, not for regular chats? Are regular chats worth less or what? There, content is directly put into a div on the site. So what? |
So here is what to do:
In a sandbox it cannot even do form submissions, so AFAIK it cannot do anything bad here. Also, of course, you should not display HTML content or so inside the app anyway. (and you don't do so), so that is no big problem. It's just a link, for what's it's worth. I see now why you do so. For the same reason why you recommend hosting the synapse server at a different domain. But if you properly sandbox the content and don't do obvious stupid things like displaying HTML files in the app (which you don't do currently), that is all safe! |
Browsers have a length limitation on this, some more strict than others. It would be a real shame to break someone's upload because they wanted to share a panoramic view of their city. |
I mean you also load text into your chat application, right? So you hopefully properly sanitize that, yes? So why don't you put that into a untrusted domain name and iframe it? It's the same for these images. If you just put a base64-encoded blob into the site in an img tag, what could be done? Nothing… So maybe sandbox that media stuff, right, but then in an iframe with |
If you have found a hole in the current sandboxing, please disclose the details to [email protected]. Otherwise, please read the comments in the code for an explanation of why we sandbox in this manner, and why we don't use the sandbox attribute on iframes, and why this applies to E2E content rather than non-E2E stuff. Closing this given it's not actually a bug. |
Never experienced a length problem there. And looking it up, it also seems that (also for blob URIs) very big files are supported. |
Okay, so restart. Thanks for the link to the comments, that was really helpful. But I've looked into the source. And I still don't understand some things. First, the comments:
Note that CSP only applies when you access the file directly. I.e. the CSP, which is sent on the site you access in the browser ( But the good news is: The loading is, of course, still secure, as you just use Okay, I understand the Chrome limitation (or I take it as given, have not tested it) and you may use Blob URIs, fine.
You get a PR; where I apply a sandbox. It's not "overly restrictive". You can easily specify what should be allowed to in the sandbox atttribut. And we need I've also had a look on the Telegram and WhatsApp attacks again. As you can see the problem occurs when you access So I tried to open it in a new tab via right-click and it did not work. Wow, great! Good job! I have no idea how you did it, but it does not work. But there is one thing, I've also noticed. That whole cross-origin/sandbox thing only applies to unknown file types. Images and videos, e.g., are included as data URIs. SO:
Yes, and it would still do in your case, as you use data URIs for images. Only the download button for any file type uses this iframe. So finally some questions:
|
Please re-open this issue for clarification! Thanks. |
@rugk kindly provided a proof of concept for how we might be able to get rid of usercontent.riot.im in favour of a dynamically generated iframe with appropriate iframe sandboxing; we are hoping to dig into it & test/understand it over the coming month. When we tried this 1-2 years ago we couldn't get it to work due to overzealous sandboxing support in browsers, but it's quite possible that we missed something. This would have the advantage of even better sandboxing the content (assuming that you are on a browser which fully implements iframe sandboxing) |
Not needed, I know they are doing something behind the scenes. At least that's what they told. |
i assume our messages crossed, but yup: we're doing something behind the scenes (i.e. analysing @rugk's findings) |
(just checking in to confirm that this is still on our radar). |
I've just been looking at the concerns here. Let me start by clarifying the purpose of the cross origin renderer and elaborate a little on what Matthew said above. Having an iframe with a different origin allows us to set the origin of the blob URI we create to an origin other than whatever you host riot on, which means any HTML/JS it might execute is in a separate origin and so can't access your private keys. As for sandboxing, the reason the iframe isn't sandboxed is that it doesn't work with a sandboxed iframe in the current stable version of Chrome (65): the created blob URI can't then be downloaded. There were also two specific issues @rugk raised that are related to this cross-origin renderer. The first was lack of checking on the origin of messages in the cross-origin renderer code meaning that, in theory, an attacker could post a message to the cross-origin iframe and execute code. I'm not aware of any way this could actually happen since the attacker would need a The second was a proof-of-concept phishing attack whereby the cross-origin iframe changes the location of its parent, redirecting the user's browser to a malicious site when the tab is backgrounded. It is reliant upon getting malicious code into the cross origin iframe, but given, as above, there is no way for a site to get a It’s true that the host of the cross-origin renderer (ie. usercontent.riot.im by default) could inject malicious code: this isn't something we've tried to protect against although we agree it’s pretty undesirable. If this is a concern, please host your own cross-origin renderer as per the cross_origin_renderer_url config parameter at https://github.com/vector-im/riot-web#configjson The good news is that Chrome 68 now does allow you to download blob URLs created in sandboxed iframes, so once this is generally available we will be very happy to move to sandboxed iframes and we'll no longer need the cross origin renderer mess. Many thanks to @rugk for digging into this and responsibly disclosing the various possible attack vectors here and encouraging us to move over to sandboxed iframes as soon as we can. |
@dbkr my concern about this issue is about the user being tracked whenever it makes a new post with attachment to riot... how many hacks a sysadmin needs to do, in order to setup his own usercontent host? If this only affects riot-web how does riot-android and other matrix clients exchange attachments? from what I understand (and I have to be clear that I don't understand all of the arguments you are putting here), this does not look like a good approach, but just a hack so that we can have (encrypted) attachments for now in riot! |
it's trivial to set up your own usercontent host, and it's is a valid solution to securing E2E attachments. good news is that once Chrome 68 is mainstream we should be able to switch over to using sandboxed iframes and remove the usercontent host entirely. |
@albjeremias riot-android isn't a web client and so doesn't have to worry about script origins, etc. You're correct though, it's far from the ideal approach, but right now it's the only way to do e2e attachments securely at all. |
It does not matter here, but that's not true. Browsers allow redirecting the parent frame site even though the origin is different. That's a crazy "feature", I know, but it's true. This is the whole thing that phishing attack was based on. Anyway I'll publish the full report soon. |
Random idea: What if the homeserver (synapse) serves the v1.html? That would remove the |
nowadays it should be doable without such an external resource using a sandboxed iframe |
The challenge with |
the eval is only needed to make use of the generic usercontent sandbox via postmessage, if we can use a data iframe we should be able to do without eval |
This comment has been minimized.
This comment has been minimized.
Great there is a fix now. FYI as promised before but long forgotten, here is the full report disclosure (I just added the dates at the end, everything else is the original report from https://gist.github.com/rugk/2e5caef4a266f15d71eae1b53ff8c29a |
Summary: Update Riot to 1.5.10 **Changes:** - Get rid of dependence on usercontent.riot.im (for details see [here](element-hq/element-web#6173)) Test Plan: Sent a few messages, stickers and files, made a short conference call Reviewers: #triage_team, JoshStrobl Reviewed By: #triage_team, JoshStrobl Subscribers: JoshStrobl Differential Revision: https://dev.getsol.us/D8306
For what is usercontent.riot.im used? I just saw it my instance tried to make a connection to it, but why?
Actually it should be self-hosted and save images or so to Synapse, so what does it do?
I saw it only in encrypted group chats.
STR:
-> Instead of Synpase,
usercontent.riot.im
is contacted.The text was updated successfully, but these errors were encountered: