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

Open
rugk opened this Issue Feb 15, 2018 · 21 comments

Comments

Projects
None yet
7 participants
@rugk
Copy link
Contributor

rugk commented Feb 15, 2018

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:

  1. In an encrypted chat, upload a file.
  2. Click on "Encrypt file".
    -> Instead of Synpase, usercontent.riot.im is contacted.
@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Feb 15, 2018

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.

@rugk

This comment has been minimized.

Copy link
Contributor Author

rugk commented Feb 15, 2018

The site https://usercontent.riot.im/v1.html just returns this code:

<html>
<head>
<script>
window.addEventListener("message", function(e){eval("("+e.data.code+")")(e)})
</script>
</head>
<body></body>
</html>

This is then injected into the current site as an (of course, not sandboxed!) iframe in order to show the content in it!
Why not just create that HTML code and iframe via JavaScript? Why do you have to access yet another of your own domains, outside of the admin's control?

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.

@t3chguy

This comment has been minimized.

Copy link
Collaborator

t3chguy commented Feb 15, 2018

Its right there in the readme, you are able to host your own
image

@rugk

This comment has been minimized.

Copy link
Contributor Author

rugk commented Feb 15, 2018

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.

Just don't display HTML/JS files and images and stuff can be displayed in img tag where they can do nothing.
Also the idea may not be bad (I am all for sandboxing!), but you have to do it in the correct way:

  • Do not load third-party content!
  • Create an iframe via JS (or have it in the index.html).
  • And finally sandbox it.

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.

@rugk

This comment has been minimized.

Copy link
Contributor Author

rugk commented Feb 15, 2018

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?

@rugk

This comment has been minimized.

Copy link
Contributor Author

rugk commented Feb 15, 2018

So here is what to do:

  1. Create an iframe via JS (or have it in the index.html), so that this extra domain is not needed.
  2. Sandbox it.
  3. Do the same for all untrusted stuff you load in there.

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.
And images can be loaded via data: or so in an img tag. No problem. What do you even try to protect against here?

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!

@turt2live

This comment has been minimized.

Copy link
Member

turt2live commented Feb 15, 2018

And images can be loaded via data: or so in an img tag

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.

@rugk

This comment has been minimized.

Copy link
Contributor Author

rugk commented Feb 15, 2018

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?
That could be made, it's just kinda ridiculous.

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 sandbox attribut.

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Feb 15, 2018

If you have found a hole in the current sandboxing, please disclose the details to security@matrix.org.

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.

@ara4n ara4n closed this Feb 15, 2018

@rugk

This comment has been minimized.

Copy link
Contributor Author

rugk commented Feb 15, 2018

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.

Never experienced a length problem there. And looking it up, it also seems that (also for blob URIs) very big files are supported.

@rugk

This comment has been minimized.

Copy link
Contributor Author

rugk commented Feb 15, 2018

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:

// For attachments downloaded directly from the homeserver we can use
// Content-Security-Policy headers to disable script execution.

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 (index.html or so) is applied. When you fetch a image.png there, it does not matter what CSP header that image sends.

But the good news is: The loading is, of course, still secure, as you just use src="https//something…" and that is plain old HTML and does never execute any JS.

Okay, I understand the Chrome limitation (or I take it as given, have not tested it) and you may use Blob URIs, fine.

// Blob URLs are generated using window.URL.createObjectURL and unforuntately
// for our purposes they inherit the origin of the page that created them.
// This means that any scripts that run when the URL is viewed will be able
// to access local storage.

createObjectURL turns bytes into a blob string. Nothing more. There is no attack vector here, so what do you fear could happen?
See below for where I see a potential attack.

// the downside of using a sandboxed iframe is that the browers are overly
// restrictive in what you are allowed to do with the generated URL.

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 allow-scripts here, of course. (toplevel access should be forbitten, however, by doing so)


I've also had a look on the Telegram and WhatsApp attacks again. As you can see the problem occurs when you access blob: or filesystem: URIs in a new tab, as only then they can get to the HTML root and create a fake HTML file.
So that is the only case where createObjectURL could cause problems.

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:

It would be a real shame to break someone's upload because they wanted to share a panoramic view of their city.

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:

  1. How could you prevent it from opening in a new tab? I could not
  2. Do you somehow handle the click on the download button? I just could not click on it when duplicating the element and moving it out of the iframe, so, I just wonder here.
@albjeremias

This comment has been minimized.

Copy link

albjeremias commented Mar 7, 2018

Please re-open this issue for clarification! Thanks.

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Mar 7, 2018

@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)

@ara4n ara4n reopened this Mar 7, 2018

@rugk

This comment has been minimized.

Copy link
Contributor Author

rugk commented Mar 7, 2018

Not needed, I know they are doing something behind the scenes. At least that's what they told.

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Mar 7, 2018

i assume our messages crossed, but yup: we're doing something behind the scenes (i.e. analysing @rugk's findings)

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Apr 2, 2018

(just checking in to confirm that this is still on our radar).

@dbkr

This comment has been minimized.

Copy link
Member

dbkr commented Apr 24, 2018

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 window object to post a message to it. Nonetheless, it makes sense to have an extra layer of security here so I've made matrix-org/matrix-react-sdk#1849 / matrix-org/usercontent#1. Anyone who would like this extra layer of security can update their cross-origin renderer to this code at any time: it's backwards compatible in both directions.

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 window object for the iframe, we don't believe this is possible. The origin lock further protects against this.

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 dbkr changed the title usercontent.riot.im ?? Replace cross origin renderer (usercontent.riot.im) with something better Apr 24, 2018

@dbkr dbkr removed the security label Apr 24, 2018

@albjeremias

This comment has been minimized.

Copy link

albjeremias commented Apr 24, 2018

@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!

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Apr 24, 2018

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.

@dbkr

This comment has been minimized.

Copy link
Member

dbkr commented Apr 24, 2018

@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.

@rugk

This comment has been minimized.

Copy link
Contributor Author

rugk commented Apr 25, 2018

The origin lock further protects against this.

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.

@lampholder lampholder added bug p2 labels Apr 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment