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

forbid active content when displaying html-messages #2127

Closed
r10s opened this issue Jan 2, 2021 · 14 comments
Closed

forbid active content when displaying html-messages #2127

r10s opened this issue Jan 2, 2021 · 14 comments

Comments

@r10s
Copy link
Member

r10s commented Jan 2, 2021

with #2125 and deltachat/deltachat-android#1763 we're about to add a function that allows displaying the original message on-call.

unfortunately, this original message may be html and html may contain scripts which may be considered to be harmful.

the question is, how to deal with these scripts.

  • my first idea was to iterate over the dom using quick_xml, and to remove these scripts, however, as @dignifiedquire, @link2xt and others pointed out on irc that this is not a good way to move forward. rough quotes from irc:
    • quick_xml is an xml tool and html support is unclear at best
    • the only solid html parser/serializer dealing with "most details" is html5ever/scraper
    • however, even if we use html5ever and reject anything that is not HTML5, it may still be a security disaster: trying to remove <script> tags etc. is not a way to do this, there is a possibility that new meta tags, <iframe> etc. things appear
    • also invalid HTML5 will be parsed differently by browser and rust HTML5 parser - so we will open the door for new bugs we have to target
    • differences in DC core and browser parsing can be exploited
    • so, if we do a complete rewrite based on a whitelist and making the writer incapable of writing dangerous things, that approach may work, but disabling scripts in the embedded browser seems like an easier and better way

i mostly agree with these points, esp. dealing with invalid HTML5 rendered in some way by clients seems to be a things i do not really like to start with, that may be open ended ...

just blocking remote content, btw, is not sufficient, as the script may be embedded in a the message and comes over imap (blocking remote content wrt privacy is another discussion :)

(what still needs some love is dealing with embedded images as <img src=cid:...> - but we can handle that by a regex and replace that by <img src=data:...> that can be handled by browsers as well as the WebViews, rewriting whole dom for that seems to be a bit much)

anyway, i had a look, an, in fact, we're already close, Android's WebView class has disabled scripts by default, on iOS' WKWebView, this is easy to do. for Desktop, this is more tricky, i just had a talk with @Simon-Laux on that, and a to go is to just open the HTML-page in a browser-context.

@Jikstra
Copy link
Contributor

Jikstra commented Jan 3, 2021

On desktop, if we open the html page "just" in a browser context, we have exactly the problems we want to avoid. One approach that we could try on desktop, is serving it on a local webserver and setting the CSP headers. That's easy (we could even do it in the rust core) and the sandboxing is completely handled by the installed browser. And we can disable a lot of things. Not just scripts/iframes, also including remote pictures/videos/css... and all the many things modern web things that could possibly get used to leak user info or do even more evil things.
If we can access a webserver running on 127.0.0.1 both on android and ios, this could be a cross platform solution that just works^TM and the different platforms don't need to care seperately on how to correctly instruct the sandbox to not allow evil things.

@r10s
Copy link
Member Author

r10s commented Jan 3, 2021

On desktop, if we open the html page "just" in a browser context

i think the wording "browser context" i was using is misleading here, sorry. i meant what we were just talking about - to write the file to disk and open it in firefox or another browser.

wrt CSP headers - interesting, thanks for the hint. seems as if a webserver is not needed to set them, you can also use <meta> tags, https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP

at a first glance, however, CSP can only prevent loading remote content, i did not see an option to avoid scripts being executed that are already in the html-file - but this is the thing, we do not want to execute any script, we do even mistrust the "self" page (in the CSP language).

a more hackish idea, maybe more a second defense line (as well as CSP): what about adding <script>crash.now=true:</script> or <script>throw "Bye!"</script> just in front of the whole html-file. iirc, that would result in all scripting to be stopped.

@Jikstra
Copy link
Contributor

Jikstra commented Jan 3, 2021

wrt CSP headers - interesting, thanks for the hint. seems as if a webserver is not needed to set them, you can also use tags, https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP

If this is possible we can just run it from the file. Buuut I somehow have a bad feeling with this. We would need to alter the html file to add the meta tags, and if we fuck something up there, hell is open :D Plus later tags probably overwrite the earlier ones (which would be quite bad and make the meta way a little bit useless against evil html).

at a first glance, however, CSP can only prevent loading remote content, i did not see an option to avoid scripts being executed that are already in the html-file - but this is the thing, we do not want to execute any script, we do even mistrust the "self" page (in the CSP language).

Scripts that are already in the html-file is called "inline". To block any js the header Content-Security-Policy: script-src none; should be enough. This forbids inlined js and any loaded js. I played around with those headers some years ago when I was doing more web development things, so I need to try this out again to make sure all those things work as expected, but in theory the documentation also states this.

a more hackish idea, maybe more a second defense line (as well as CSP): what about adding <script>crash.now=true:</script> or <script>throw "Bye!"</script> just in front of the whole html-file. iirc, that would result in all scripting to be stopped.

Exactly these workarounds I wouldn't trust. We don't know a way around this, but I'm not sure if there isn't a way around it. And probably it's a path that only works in this one browser. I really would not trust in any "sandbox" hack that we try to achieve, no stripping of tags, no injecting of code to break other code, nothing.

@Simon-Laux
Copy link
Member

Simon-Laux commented Jan 3, 2021

also invalid HTML5 will be parsed differently by browser and rust HTML5 parser

html5ever is an HTML parser developed as part of the Servo project.
https://github.com/servo/html5ever

html5ever is made for browsers isn't it?

If this is possible we can just run it from the file. Buuut I somehow have a bad feeling with this.

I also trust the header tags more, but the webview/browser needs support it too. I would go for sanitizing and such headers, but just disabling js in webview is enough for the first opt-in experimental versions.

as far as rust crates for sanitizing go, the only crate I found looking promising is https://github.com/rust-ammonia/ammonia (based on html5ever, but html5ever seems to have two issues that could be a problem rust-ammonia/ammonia#83 (comment))

@r10s
Copy link
Member Author

r10s commented Jan 3, 2021

html5ever is made for browsers isn't it?

sure, but if we generate html5 on our own, we have to deal with bugs the creators did - whether they rely on them or not know about them (@link2xt i think, this was your the point on irc i quoted above, please correct me, if i am wrong :)

I also trust the header tags more

why? in general, header tags outside the html-file seems to be lost easier than some tags written in the html-file.
however, this CSP (as well as my hack idea from above) can only be an additional layer. also it is still unclear that CSP really can avoiding execution of already loaded scripts. [EDIT: the latter is doable, see above] so, i think, that discussion can be postponed.

as far as rust crates for sanitizing go

the point is that sanitizing may be much more complicated and probably opens a lot of new problems we are just not having if we can disable scripting (as we can for android and ios).

btw, i just remembered that, sanitizing tags and attributes is not sufficient - you also have to deal with attribute values as well - unfortunately, there is the javascript: protocol. and you have to deal with. and i am pretty sure, there is even more ...

nevertheless, ammonia sounds interesting, and could probably help, but there are still lots of things to consider. and, sanitizing is probably still not the best first-defense.

all that is doable, however, compared to the effort, and to the fact that there may still be open points because of lots of attack vectors (javascript: in some weird encoding ...) i meanwhile also think that disabling JavaScript is a more secure and more straigthforward way to go.

on systems, where this is not easily doable (as on desktop), the idea is to open the html in firefox or so (as mutt does) - at least for now.

@Simon-Laux
Copy link
Member

I vote for both in the end, sanitizing and disabling js.

@Jikstra
Copy link
Contributor

Jikstra commented Jan 3, 2021

why? in general, header tags outside the html-file seems to be lost easier than some tags written in the html-file.

Header tags only get lost if you don't send them, and that only happens if our web server code is crappy, which we can test. Tags written in the html file act in the same level as our possible malicious content. We can not test against malicious content we don't even know exists. Headers are on a level above, they are getting sent outside the html file, you could say in a parent scope of the html. And from what I know it's not possible to overwrite a header with something inside the html file. If so, we can of course not use this solution.

just disabling js in webview is enough for the first opt-in experimental versions.

I don't think so. <img src='http://myserver.evil/some-picture-which-logs-headers-and-ip.png'></img>

on systems, where this is not easily doable (as on desktop), the idea is to open the html in firefox or so (as mutt does) - at least for now.

Probably we could even do it inside electron with either a sandboxed window or a sandboxed iframe. The question is of course if we want to do this. https://www.html5rocks.com/en/tutorials/security/sandboxed-iframes/

EDIT: Just to show again that CSP is not a hack for this, but an intended tool, here a quote of an example from "developer.mozilla.org" about CSP:

A web site administrator of a web mail site wants to allow HTML in email, as well as images loaded from anywhere, but not JavaScript or other potentially dangerous content.

Content-Security-Policy: default-src 'self' *.mailsite.com; img-src *

Note that this example doesn't specify a script-src; with the example CSP, this site uses the setting specified by the default-src directive, which means that scripts can be loaded only from the originating server.

Source: https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP#Example_5

@link2xt
Copy link
Collaborator

link2xt commented Jan 3, 2021

@Simon-Laux

as far as rust crates for sanitizing go, the only crate I found looking promising is https://github.com/rust-ammonia/ammonia (based on html5ever, but html5ever seems to have two issues that could be a problem rust-ammonia/ammonia#83 (comment))

Their approach looks good, except for DoS attacks based on higher-than-linear parsing of unclosed tags. This is similar to pandoc -f html -t html, but with a richer internal representation (DOM). And pandoc also has parsers with similar problems due to backtracking by default in Parsec (parser combinator library) parsers.

@Jikstra

One approach that we could try on desktop, is serving it on a local webserver and setting the CSP headers.

Local webserver can be accessed by any other application running on the same device. See related SyncThing discussion, where I commented: syncthing/syncthing#3357 (comment) Even if we implement this token authentication, I think there should be a better way. I'm not familiar with Electron, but hope there is a better way to create an isolated browsing context without loading from a separate domain/port.

@r10s
Copy link
Member Author

r10s commented Jan 3, 2021

i also have the feeling that using a webserver just to deliver the html-file opens new attack-vectors.

however, https://www.html5rocks.com/en/tutorials/security/sandboxed-iframes/ sounds good, also the quoted CSP quoted by @Jikstra - seems as if that can go to the iframe attribute, so we do not need meta/headers (but even if not, i still think, meta could do the job - even if evil-page adds some less-strict rules, the most-strict wins, https://w3c.github.io/webappsec-csp/#meta-element)

just disabling js in webview is enough for the first opt-in experimental versions.

I don't think so. <img src='http://myserver.evil/some-picture-which-logs-headers-and-ip.png'></img>

but myserver.evil cannot execute a script on ther users machine that way. this issue is not about blocking remote content wrt privacy.

@Jikstra
Copy link
Contributor

Jikstra commented Jan 3, 2021

@r10s

but myserver.evil cannot execute a script on ther users machine that way. this issue is not about blocking remote content wrt privacy.

It cannot execute scripts or anything but leaks the ip address, user agent... Which can be dangerous enough already or completely fine.

@Jikstra - seems as if that can go to the iframe attribute, so we do not need meta/headers (but even if not, i still think, meta could do the job - even if evil-page adds some less-strict rules, the most-strict wins, https://w3c.github.io/webappsec-csp/#meta-element)

The thing i'm worried about with this solution is that we have to add the tags at the right place. If some malicious html can trick our parser into adding it at a place where the browser ignores it, for whatever reason (because the browser sees it in the body or whatever) all our protection is gone. This is not easily doable with headers. The format of the html cannot trick us into putting the tags at the wrong position. I don't want to spread scare/fear of this, i just want to tell, let's be very careful about those ideas because there are a lot of things to oversee/implement in a inseucre way.

however, https://www.html5rocks.com/en/tutorials/security/sandboxed-iframes/ sounds good, also the quoted CSP quoted by

If we want to go for the iframe appraoch, we would need to use the srcdoc attribute, put the potential evil html in it, take care of escaping... Otherwise we would need to serve it from another file (if that is possible) or from a local webserver again. All this points speak about the idea to open a browser with the url "file://path/to/the/sandboxed-potential-evil.html"

@link2xt

Local webserver can be accessed by any other application running on the same device. See related SyncThing discussion, where I commented: syncthing/syncthing#3357 (comment) Even if we implement this token authentication, I think there should be a better way.

Of course we would need to tokenize the files and not just serve all the emails. We could only serve the requested files and tokenize them in a way like : http://localhost:8080/g8j3kf3h1ddj239uf3841.html or http://localhost:8080/g8j3kf3h1ddj239uf3841.html. I think with SyncThing the problem is a little bit different, as an attacker can actually control syncthing through the webui. With deltachat this is not (yet? :D) the case.

@link2xt
Copy link
Collaborator

link2xt commented Jan 3, 2021

Open port 8080 on localhost will still indicate that Delta Chat is running, and this information will be available to every page you visit.

With WebView it is possible to intercept all requests and even rewrite cid: URLs on the fly: https://medium.com/@madmuc/intercept-all-network-traffic-in-webkit-on-android-9c56c9262c85 Together with JavaScript disabled, this should be enough.

As for Electron, I don't know.

@Simon-Laux
Copy link
Member

in electron you can also register schemes protocol.registerBufferProtocol, so theoretically the cid(if they allow for shemes without the // behind the :, which I haven't tested yet) rewrite and even an remote content http/s rewrite could be done.

@r10s
Copy link
Member Author

r10s commented Jan 4, 2021

my idea for cid: was to convert them to data: in core - that way, all systems get the images without additional effort. also, that way, you can open the file in firefox or so. data: as such seems to be fine and is often discussed as an alternative to cid in html-mail-context.

the conversion should not be too complicated, i would iterate over attached images and then do a regex-replace on the html on the concrete id, sth. as /(<img[^>]*src[^>]*=[^>]*)(cid:1234567890)([^>]*>)/${1}data:base64data${3}/

i do not think this adds much risks. both, html and image come from the same source, data could also be added to the img-tag directly.

@r10s r10s added the discussion label Jan 9, 2021
@r10s
Copy link
Member Author

r10s commented Jan 9, 2021

closing this for now, no actionable item left for core for now and i clarified things at 1bfdd90

@r10s r10s closed this as completed Jan 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants