SECURITY HOTFIX - Remove XSS attack

[Redsandro]

This will address #1443 at least partially by restricting the type of html tags and attributes that user is allowed to use.

markdown-it themselves advise against enabling html (and it was not my choice to enable it(1)), but I've seen youtube-iframes and img-tags used in the wild.

The set is pretty restrictive because I don't know what kind of html is legitimately used in the wild. This PR is an important first step and more tags can be allowed later if they turn out to be used by a lot of people. I recommend to merge this first because it's a security issue, and wait for upset people that want to use html tag x-y-z later, and consider adding them back one by one.

---

(1): I think no html should be allowed. If people want to e.g. embed youtube-movies, a markdown extension should be made for that. Currently, this is done by means of iframe, so this PR restricts the urls iframe is allowed to load to youtube.com. More allowed urls can be added later if they turn out to be used a lot.

I think img tags should be disallowed too because there are markdown alternatives, but this way at least the onerror the harm is gone.

[RomanKlopsch]

I think the allowed tags, attributes and host names should be read from a config file (lets say json to keep it simple) because that would make it much more comfortable to tweak/extend/improve these restrictions

[RomanKlopsch]

the next step could be to add an option to the settings, that allows to specify, which tags, attributes and hosts are allowed

[Redsandro]

Careful when saying "should" @RomanKlopsch. I just spend hours patching a security hole directly after it came to my attention.

My prefered security fix would be to disable html. According to the developers of the markdown renderer, that is what people "should" do. It's a boolean option; it would be a 1 minute fix. I figured Boostnote had reasons for going against this security advice, so I came up with this solution.

Feel free to build a palace on top of this PR, but let's not start the goldplating discussion while the security hole is still open.

@Rokt33r can add some extra defaults if you plead for certain tags, but I'm hoping he will merge this yesterday.

[RomanKlopsch]

I see your point @Redsandro and i agree, that this fix should be merged as fast as possible.
But disabling nearly every html-tag and -attribute will cause some notes to not render properly, which is bad obviously. I am trying to find a Solution, that fixes the issue but harms the UX as less as possible.

Call me wrong but as far as i know it should last out to remove all html-tags, that define a js-event. These tags do all begin with on so removing all tags containing on should prevent the insertion of js.

[Rokt33r]

I'd just worried about the diagram things. But, everything works smoothly except auto scrolling.

I agree with @Redsandro. Even we disable nodeIntegration of BrowserWindow, local files(file://~~) are still vulnerable.

[Rokt33r]

(1): I think no html should be allowed. If people want to e.g. embed youtube-movies, a markdown extension should be made for that. Currently, this is done by means of iframe, so this PR restricts the urls iframe is allowed to load to youtube.com. More allowed urls can be added later if they turn out to be used a lot.

Definitely we should do.

@Rokt33r can add some extra defaults if you plead for certain tags, but I'm hoping he will merge this yesterday.

The current configuration looks good to me.

[Rokt33r]

@Redsandro Could you fix auto scrolling again? If you don't have time to do it, I'm going to disable it temporary.

Anyway, I'm going to release the next version on this Thursday.

[0d-gg]

Hey guys, I reported the referenced batch of security bugs - thanks to everyone involved for your effort in fixing. I'll definitely re-test upon release and report any new XSS if I find some as a new issue. I agree with @Redsandro on pretty much all fronts. Allowing HTML while trying to prevent JS is a very painful path to go down. The side effect of this XSS allowing arbitrary code exec makes it even more dangerous. I think the whitelist approach is a good compromise. I don't think it's necessarily a bad decision to allow some html/css - it allows for a ton of cool features. Even my workflow depends on some html to format my notes appropriately, but I may be misusing the product a bit, haha.

I regretfully don't have much time to help during the development phase, as my bandwidth is a little limited at the moment, but I can try to help on the pentesting front as much as I can.

[Redsandro]

@Rokt33r said:

@Redsandro Could you fix auto scrolling again?

I pulled master and the scroll sync works fine on my machine? What branch are you referring to?

-update- I just did a fresh clone from master. Scroll sync still works fine. Could it be that you were on a wrong branch?

[Redsandro]

@RomanKlopsch said:

disabling nearly every html-tag and -attribute will cause some notes to not render properly

Properly written notes should not have a problem. I think if a note depends on html a lot, one can argue that it's not written properly, but we have been getting away with it because of a bug or misguided feature. However, this discussion is not very productive without examples of problems caused. Show us broken usage patterns, and we can figure out how to deal with them, preferably in a new issue, and one problem per issue.

Call me wrong but as far as i know it should last out to remove all html-tags, that define a js-event.

There are actually many ways to do xss, depending on features, plugins, and dependencies used by Boostnote, and their respective security issues. There can be zero-day exploits where you can write xss where it is not logically supposed to be possible due to errors in said dependencies.

removing all tags containing on should prevent the insertion of js

Those are only the obvious ones. And electron picks up a lot of non-obvious ones, but there will always be new ones. Here are some other common ones:

• src
• dynsrc
• lowsrc
• background
  • e.g. <table background="javascript:alert('xss')">
• style
  • e.g. <img src="error" style="xss:expression(alert('xss'))">
• data
• seek controls on html5 players
• size
• href
• content

XSS is more complex than just the 'legal' ways to execute scripts.

I would actually prefer to remove img tag too, because I don't know what is possible when a dropbox hacker inserts an image that is 302 redirected to an .exe file that he just put in the same dropbox folder. Electron should ignore this, but that's the thing. Zero days and buffer overflows are discovered all the time.

For now this PR seems a sane place to start.

[Redsandro]

@pmood Thanks for reporting. I wanted to get this fixed but I'm on a 'time budget' as well, so I only did shallow testing. I'm especially interested to see if style is safe indeed, since it's so versatile, theoretically supporting urls and expressions and such.

[0d-gg]

Expressions should be impossible since that only works with old versions of IE as far as I know. Being limited to arbitrary CSS, the worst someone could do would probably be a keylogger. For ex: https://css-tricks.com/css-keylogger/

But even that's no big deal really since it would require you to type your sensitive information into that markdown file (like, honestly, what's the attack vector there?). You may be able to use it to load things like local images or fonts into the page. Again, not really a big deal. You can probably crash the program by loading huge images over and over... but that's pretty meh. You could have it automatically make requests upon opening the file which would reveal your IP if the attacker controls the server.

The thing I am most interested in testing is the change to iframe and the filtering. I also started looking into maybe finding an issue to get XSS (or just flat code exec) using the existing markdown features rather than relying on inputting html directly.

[libeanim]

@Redsandro Thanks for your work and the explaination 👍 seems to be a real pain to solve xss without restricting users too much!

Let me quickly outline some broken usage patterns caused by this fix here (please correct me if I'm wrong, I can then transfer them into a new issue if required)

• only iframes from youtube.com are allowed. Iframes to any other video platform (e.g. vimeo) don't work anymore. It seems odd to favour one platform.
• video tags are not allowed. Notes that link to a video on a server or a video file at local path won't work. (Same is true for audio)
• If a user uses <p> to introduce a specific text styling/aligment that won't work.
• Also divs which can be used to for example allow multi column text or to enable other text layouts, won't work.
• <mark> to highlight text or <ins> or <del>.
• setting width and height of images
• tables which allow multiline rows need to use the table tag (and <tr>, <td>)
• And three things I do (might not be applicable to the majority):
  • iframe to import zoomable graphs (local html file) into my notes when I document data analysis results.
  • iframe to google drive maps for travel planning.
  • iframe to embed podcasts I wrote a summary for so I can quickly replay a part inside the note

This list can be endless and there will be almost certainly a broken usage pattern as long as the markdown implementation doesn't provide all possibilities that html does. Guess the question is what usage patterns boostnote wants to encourage (+ the majority needs) and which it wants to restrict.

My guess: Boostnote is specifically advertised as "note-taking app for programmers that focuses on customizability" so I guess in the long run restricting all html tags when there are no alternatives in markdown might be annoying for some users. What about allowing to deactivate sanitisation but activate it by default?
But I agree it's better to extend the markdown implementation so people can write everything in it.

Let me know what you think about that.

[rayou]

Hi @Redsandro

yarn.lock was not updated, could you please update to keep everyone in sync?

Thanks

[Redsandro]

@rayou this branch was merged 2 days ago. You'll have to create a PR upstream.

Please note that I am using the npm workflow. I don't know what yarn is. I just git push what was updated. If this creates a mismatch, perhaps some automation should be written.

[Rokt33r]

I'll take care of it.

[Redsandro]

@libeanim I'll address all points briefly and subjectively, but the fact is that previous usage options were only available at a serious risk. Risky behavior is now disabled, and this is the new status quo. Issues that arise from the inability to do previously risky things should really be treated as feature requests from now on, and have their own separate issues.

1. only iframes from youtube.com are allowed.

Yes. I suggest opening a new issue/whishlist for a few weeks to collect more legal suggestions from users to complement the default legal origins with.

2. Notes that link to a video on a server or a video file at local path won't work.

True. Functionality and Security are opposites, unfortunately. One could argue for a user customizable wishlist, but in my opinion, highest security should always be the default. Please open a separate issue for this if you want to discuss this further.

3. If a user uses <p> to introduce a specific text styling/aligment that won't work.
4. <mark> to highlight text or <ins> or <del>.

True. Markdown should handle paragrapgs, italics, strikethrough, etc.. If you want to write html, use a html editor.

5. Also divs which can be used to for example allow multi column text or to enable other text layouts, won't work.

True. I think divs are safe and can be added to the whitelist, but I have never seen this in a note. Are you using this, or are you thinking of potential repercussions from this PR?

6. setting width and height of images

False. You can use style for that.

7. tables which allow multiline rows need to use the table tag (and <tr>, <td>)

I'd argue that a Markdown editor should not use html tags and for tables there should be Github Flavored Markdown support in Boostnote where you can make basic tables. But as long as you don't use background it seems safe. You can create a PR with table tags added to the whitelist.

8. iframe to import zoomable graphs (local html file)

I can imagine your usecase, but this is impossible without compromising security. Please create a new feature request issue with the option to add custom iframe origins or the option to disable XSS security.

9. iframe to google drive maps for travel planning

Seems like you can safely create a PR with maps as a whitelisted origin.

10. iframe to embed podcasts I wrote a summary for so I can quickly replay a part inside the note

Perhaps if the podcast is on a big famous trusted site, it can be added on the whitelist. Otherwise see response to point 8 above.

What about allowing to deactivate sanitisation but activate it by default?

Can be discussed in a separate issue. Personally I think the option should be worded for what it is:

Options

◽ ⚠️ Allow dangerous html tags in notes

When enabling, a modal pops up:

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!

This is in my opinion the only acceptable implementation of what you are saying. But still, personally I'm against it. Enabling something this dangerous should not be as easy as two clicks. I'm for a whitelist that can only be edited manually so that no one will accidentally mess things up.

---

This will quickly get out of hand and hard to follow. Please create separate PRs for the obvious cases and separate issues/feature-requests for ideas that require further discussion.

[Rokt33r]

Let's discuss this issue on #1644

[libeanim]

@Redsandro Thanks for your response! Yeah, I agree to all of your points. Maybe I have some time in the next weeks to implement the one or other feature ☺️

For your interest regarding no 5:

True. I think divs are safe and can be added to the whitelist, but I have never seen this in a note. Are you using this, or are you thinking of potential repercussions from this PR?

yeah I used it to have an image on one side and text on another. Would be ace if multi-column text can be supported with markdown. I don't know whether there is any specification for that tho. Might look into that / create an issue. Cheers

[emrusso]

A quick +1 to @libeanim - I frequently use spans and divs to style definitions for class notes - I was pretty cranky to update my version in the middle of finals and lose all the styling I use to distinguish between concepts within notes. I originally switched to boostnote from another md app for ease of combining markdown and html.

[Redsandro]

@emrusso I'm sorry to hear that. I hope that you're somewhat understanding to the idea that we were trying to prevent a more dramatic comment that would go something like "I was pretty cranky to learn that my computer was hijacked in the middle of finals by ransomware through security leaks that were known for 3 months now."

As for the problem itself, you can do multiple things:

1. Install the previous version
2. Wait for this to be released
3. Create a PR specific to your wishes
4. Give a quick +1 to the people caring for your security too, to let them know it doesn't exclusively piss people off. 😿

As for the proposal and further discussion, see #1644.

[Rokt33r]

@emrusso
I'm going to allow it. Please wait for v0.11.3. I'm going to release it on Friday. 😢
#1677

[emrusso]

@Redsandro @Rokt33r thanks so much for the quick response! Didn't mean to undercut the importance of security at all - I definitely appreciate (and prefer) the fast action on that, especially considering the prevalence of XSS. My main intent was to give more feedback on what html is used in the wild. Using the git settings for sanitization is a great move to maintain a balance between features and security 😊