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

A middle ground between enabling JS on preview and preventing XSS attack. #1644

Closed
Rokt33r opened this issue Mar 7, 2018 · 6 comments
Closed
Assignees
Labels
discussion 💬 Issue concerns a discussion. help wanted 🆘 Pull request/issue requires extra help from the community. Check these out if you're new!

Comments

@Rokt33r
Copy link
Member

Rokt33r commented Mar 7, 2018

There are lots of feedback.
Someone want to make it possible to use JS on the markdown preview.
But, some other people want strict sanitizing for security.

I'm actually agree with using JS should be fine. The only attack vector is writing attack code on the markdown. I think our most users are developer, so they can prevent the attack by theirselves.

But, Electron has too much power. It can access and manipulate filesystem and shell.

So, I think we should hosting the app on electron via http.(It means making electron main processor as a web server. If we need ipc, we can exploit websocket.) So, all local images, starting with file://~~, and electron instance won't be able to access. And, giving an option to sanitize HTML strictly should be enough.

I believe this should be a nice point of compromise. How do you think guys?

@Rokt33r Rokt33r added the discussion 💬 Issue concerns a discussion. label Mar 7, 2018
@Redsandro
Copy link
Contributor

Redsandro commented Mar 9, 2018

I think our most users are developer, so they can prevent the attack by theirselves.

This is not true. I am a developer, and I want to share my notes with all my devices. So it doesn't matter how smart I am. If I use Dropbox, OwnCloud or SkyDrive, I am vulnerable to their security. And a lot of services by competent developers have already been compromised to a smaller or bigger extend in the past.

So, I think we should hosting the app on electron via http.

What does that entail? Will the notes be stored natively or remotely


I think it would be best to be secure by default (limited set of safe html tags) and allow to opt-in to insecure features (broad set of tags and dangerous attributes) with a clear warning.

For example, an option in the app settings like so:

Options

⚠️ Allow dangerous html tags in notes

When enabling, a modal pops up, where you have to confirm:

Dangerous tags

⚠️ Enabling html also enables anyone with access to any of your devices or synced folders to prepare a note that takes over all your connected computers.

⚪ I understand the implications and want to enable dangerous tags anyway
🔘 Nevermind, keep me safe!

@Rokt33r
Copy link
Member Author

Rokt33r commented Mar 9, 2018

f I use Dropbox, OwnCloud or SkyDrive, I am vulnerable to their security.

Hmm, I didn't notice that. Good point.

What does that entail? Will the notes be stored natively or remotely

As long as I know, if the URL of BrowserWindow is starting from http://~~, global objects of Node.js, like require, and the local images are not available. So, I thought it should be enough.
So, attackers could steal some rendered notes, but they couldn't access local filesystem and shell.

I think it would be best to be secure by default (limited set of safe html tags) and allow to opt-in to insecure features (broad set of tags and dangerous attributes) with a clear warning.

For example, an option in the app settings like so:

Options

◽️ ⚠️ Allow dangerous html tags in notes

When enabling, a modal pops up, where you have to confirm:

Dangerous tags

⚠️ Enabling html also enables anyone with access to any of your devices or synced folders to prepare a note that takes over all your connected computers.

⚪️ I understand the implications and want to enable dangerous tags anyway
🔘 Nevermind, keep me safe!

Sounds reasonable. I agree with it.

@Rokt33r Rokt33r added the help wanted 🆘 Pull request/issue requires extra help from the community. Check these out if you're new! label Mar 9, 2018
@libeanim
Copy link

libeanim commented Mar 12, 2018

Cool thanks @Rokt33r for creating this issue!

My personal opinion is, to have a strict sanitisation as @Redsandro suggests containing some whitelisted html tags and urls (which are safe). We could then add a user configurable whitelist of allowed tags and the option to enable html in general with a clear warning.
So something like this:

Markdown Sanitisation

🔘 ✔️ Only allow secure html tags (recommended)
⚠️ Use my own configuration
⚪ ❌ Allow dangerous html tags

(the last two options could be combined)

Additionally we can add new features to the markdown renderer (provide [youtube-]video support aso) so that the need for html tags is no longer there.

@libeanim
Copy link

@Rokt33r

So, I think we should hosting the app on electron via http

Hmm hosting via an http server seems to be a bit overkill (to my mind) and it would be best practice to sanitising the markdown/html anyways.

As long as I know, if the URL of BrowserWindow is starting from http://~~, global objects of Node.js, like require, and the local images are not available

Not sure if I got your suggestion, but as node is integrated into the electron javascript engine, it doesn't matter whether one loads an html file locally or via http in the BrowserWindow object. So e.g. require('fs') would work in both cases.

In theory one could host the notes via http and reference them via iframes in the main app, but I would not prefer that to sanitisation.

To my mind the best way would be to use proper html sanitisation as @Redsandro did, make it configurable and add more features to the markdown implementation to reduce the need of html tags in general.

@Redsandro
Copy link
Contributor

Danger Zone


🔘 ✔️ Only allow secure html tags (recommended)
⚠️ Allow styles
⚪ ❌ Allow dangerous html tags

ℹ️ Allowing dangerous html tags also enables anyone with access to any of your devices or synced folders to prepare a note that takes over all your connected computers.

@Rokt33r
Copy link
Member Author

Rokt33r commented Mar 15, 2018

@Redsandro Sound reasonable. I'll take care of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion 💬 Issue concerns a discussion. help wanted 🆘 Pull request/issue requires extra help from the community. Check these out if you're new!
Projects
None yet
Development

No branches or pull requests

3 participants