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

Remove dangerous HTML from emails #7

Open
Soreine opened this issue Dec 31, 2019 · 10 comments
Open

Remove dangerous HTML from emails #7

Soreine opened this issue Dec 31, 2019 · 10 comments
Labels
enhancement New feature or request

Comments

@Soreine
Copy link
Member

Soreine commented Dec 31, 2019

To protect the apps (especially the Electron app) from XSS attacks, we want to sanitize the displayed HTML.
DOMPurify is promising, we need to make sure it has not impact on performance.

@Soreine Soreine mentioned this issue Jan 8, 2020
@Soreine
Copy link
Member Author

Soreine commented Jan 8, 2020

After working on this for a while, I concluded that we should not use DOMPurify, but instead do proper sandboxing. This is the simplest way to protect from dangerous HTML. See PR #11 (comment)

Sandboxing with iFrames

As it seems to be done by Mailpile. They sandbox the HTML, and only use DOMPurify to block remote content.

mailpile/Mailpile#631

See their code to display emails

https://github.com/mailpile/Mailpile/blob/babc3e5c3e7dfa3326998d1628ffad5b0bbd27f5/shared-data/default-theme/html/jsapi/message/html-sandbox.js

https://www.html5rocks.com/en/tutorials/security/sandboxed-iframes/

<iframe sandbox seamless srcdoc="{emailHtml}"></iframe>

Sandboxing in special Electron views

Using BrowserView with the necessary webPreferences in particular disable the nodeIntegration and enable contextIsolation

https://electronjs.org/docs/api/browser-view

https://electronjs.org/docs/tutorial/security

Sandboxing with a React Native WebView

Disabling JS and other options are available for WebViews.

https://github.com/react-native-community/react-native-webview/blob/master/docs/Reference.md

@Soreine
Copy link
Member Author

Soreine commented Jan 8, 2020

We can always cleanup the HTML a bit, by removing all scripts resources for example, but that's just optimization.

Edit:
Some other weird things that we could remove, but are not dangerous:

  • content editable

@Soreine Soreine added the enhancement New feature or request label Jan 9, 2020
@Soreine
Copy link
Member Author

Soreine commented Jan 9, 2020

Mailspring has a whitelist of allowed tags and attributes that they use in conjunction with a sandboxed iFrame: source

@0nn0
Copy link
Contributor

0nn0 commented Jan 10, 2020

Thanks for the thorough research and examples!

Sandboxing the IFRAME and disabling JS for the react-native Webview sounds like the way to go. Would setting the necessary webPreferences add additional security?

@Soreine
Copy link
Member Author

Soreine commented Jan 10, 2020

I'm still thinking about this. I'm not a security expert, but I know that several safety nets are better than a single one. And it's best to whitelist things, and only allow what we need. So we need to ensure minimal security protection, and then find a balance between perf/maintenance and added security.

Mobile

For mobile, the minimum is using a WebView with JS disabled ([K-9 only does that](K-9 does just that)). It will act as a sandbox. But as Android doc recommends, WebView can have vulnerability. We could add an iFrame on top of that, if we find there's no added cost.

Here is what I would do for the WebView:

  • javascriptEnabled = false Disable JS
  • mixedContentMode = always Some emails won't show properly otherwise. If possible, we should ask the user when insecure origin are detected.
  • thirdPartyCookiesEnabled = false As long as this WebView does not open web pages, we don't need cookies.
  • geolocationEnabled = false
  • incognito = true No need to store data
  • allowsLinkPreview = false
  • originWhitelist = [] Disable navigation (only open links in mobile browser)

If your WebView component accesses any personal data, then you may want to periodically delete any files that are stored locally, using the clearCache() method. Alternatively, you can prevent your app from caching sensitive content, using the no-cache server-side header.

I don't know how an attacker could access the cache. So I don't know if this affects us or not.

Electron

For Electron, using a BrowserView with nodeIntegration: false and contextIsolation: true is the first and most important step.

⚠️ Under no circumstances should you load and execute remote code with Node.js integration enabled. Instead, use only local files (packaged together with your application) to execute Node.js code. To display remote content, use the tag or BrowserView, make sure to disable the nodeIntegration and enable contextIsolation.

Here is what I would do for the BrowserView:

  • nodeIntegration: false
  • contextIsolation: true
  • webSecurity: false
  • Deny all session permission requests (implement setPermissionRequestHandler)
  • allowRunningInsecureContent: false to disallow mixed content
  • Disable navigation, by calling event.preventDefault() in a will-navigate handler
  • Disable new windows, by calling event.preventDefault() in a new-window handler

And also use a sandboxed iframe inside that BrowserView

<iframe sandbox seamless srcdoc="{emailHtml}"></iframe>

@Soreine
Copy link
Member Author

Soreine commented Jan 10, 2020

Without completely relying on DOMPurify for security, we can still use it to whitelist things as Mailspring does. I'll try their approach

@Soreine
Copy link
Member Author

Soreine commented Jan 13, 2020

In retrospect, DOMPurify is quite heavy.
So if we decide to add it to the stack, we must decide that the additional safety net is worth the performance cost (half as slow as TalonJS, mainly because of JSDom) and risk of broken email (because of important parts may be whitelisted).

@0nn0
Copy link
Contributor

0nn0 commented Jan 13, 2020

Thanks for documenting all suggestions. This is great! Going to try them this week.

It's really hard for me to judge the security quality of DOMPurify vs the others. Perhaps we can start with the low hanging fruit?

@Soreine
Copy link
Member Author

Soreine commented Jan 13, 2020

DOMPurify vs the others

If you mean libs similar to DOMPurify, like
https://github.com/apostrophecms/sanitize-html
https://github.com/YahooArchive/xss-filters

then yes, I agree it's hard to tell. I feel like DOMPurify is quite mature and is the most secure, while still preserving as much of the original HTML as possible. Performance is not their main priority of course

xss-filters is not adapted for our use case

And sanitize-html seems simpler, with safe default options, although it's aimed a HTML fragments, rather than whole documents. I added it to the benchmarks and it's fast. We may use instead of DOMPurify for this suggestion.

I don't know other alternatives.

@Soreine
Copy link
Member Author

Soreine commented Apr 29, 2020

Other sanitizing lib https://github.com/bevacqua/insane

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants