BoostIO / BoostNote-Legacy

This repository is outdated and new Boost Note app is available! We've launched a new Boost Note app which supports real-time collaborative writing. https://github.com/BoostIO/BoostNote-App
Other
17.07k stars 1.47k forks source link

HTML tags not rendered anymore? #1672

Closed tlwt closed 6 years ago

tlwt commented 6 years ago

I think since last update HTML tags are not rendered anymore. is that by design? Currently running Version 0.11.1 (0.11.1) on Mac OS High Sierra 10.13.3 (17D102)

<h2>test</h2>
<b>test</b>
<li>test

Exporting it to HTML gives this output. All tags are stripped. test.txt

Rokt33r commented 6 years ago

https://github.com/jch/html-pipeline/blob/master/lib/html/pipeline/sanitization_filter.rb Okay, let's make same settings to the one of github. 😄

Rokt33r commented 6 years ago

If someone has another idea, let me know.

tlwt commented 6 years ago

two additions would be nice - allowing the style attribute ( <div style="...">) to individually format html tags. and the underline <b> tag is helpful, as it is not available within markdown to my understanding.

0d-gg commented 6 years ago

If it's fine to allow style attr, then style tags could be added as well so that people don't have to copy and paste. I don't know of any XSS that would work in CSS in Chrome, so it should be fine.

Rokt33r commented 6 years ago

https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet

I've tried all attacks using style attribute in the above link. But, none of them works. I assume it should be fine to allow it.

Rokt33r commented 6 years ago

@tlwt Done. https://github.com/BoostIO/Boostnote/pull/1677

emrusso commented 6 years ago

@Rokt33r any chance of allowing the style tag and not just attribute?

Rokt33r commented 6 years ago

@emrusso I couldn't find any problem from style tag. @Redsandro How do you think?

Redsandro commented 6 years ago

@Rokt33r I recommend The Open Web Application Security Project says style is not safe and should not be used.

This would be a candidate for https://github.com/BoostIO/Boostnote/issues/1644#issuecomment-372174622

0d-gg commented 6 years ago

@Redsandro @Rokt33r I am going to have to disagree with the comment that <style> is less safe than using style attributes. Anything that can be done within the <style> tag can be done with the style attribute of a specific element (I am not sure about including remote stylesheets - that should probably be excluded for now). If the project allows arbitrary CSS through style attribute, I don't believe there is any additional risk from allowing the style tag. All of the XSS attack vectors that use CSS do not affect webkit's rendering engine, and only apply to ancient versions of Internet Explorer.

I don't disagree with OWASP's advice in theory. If a user can control the stylesheet of your website, they can indeed cause all sorts of chaos. Something as simple as html {display:none} or making a malicious link scale to take over the whole page, or as advanced as a CSS keylogger (source: https://css-tricks.com/css-keylogger/). However, disallowing the style tag prevents none of that from happening. It just means that users will have to copy and paste inline styles to their notes.

Perhaps a "global stylesheet" would be a nice feature - though, this is kind of what themes are already. More of a user-specific one.

Thanks again to you both for the awesome improvements to the security of the app. I spent a few hours yesterday banging on it for a new XSS vector and came up empty. Awesome work! Now it's just about tweaking things to make sure everyone has the features they need while also doing what we can to keep them from shooting themselves in the foot.

Redsandro commented 6 years ago

Yes, I'm divided between what I know is right and the amount of annoyance it causes. I think people would be more understanding if someones computer was actually compromised in some dramatic way.

After all, most people prefer to share notes using Dropbox or some other cloud. Pretty much all cloud storages have been hacked in different ways. Google Drive. Dropbox. iCloud. They have all been breached in the past. And Electron is compromised every other month. This means although we see no vector while trying out known XSS exploits now, it could be a matter of time before a new one is found specific to Electron, and can be exploited.

It's hard to win here without implementing danger as a choice as in #1644; otherwise people are going to be upset either way.

I'd vote against style-tags and style-attributes being available without first explicitly opting-in through the Boostnote options as proposed in https://github.com/BoostIO/Boostnote/issues/1644#issuecomment-372174622.

Until such possibilities are implemented, I'd say stick with 0.10 at your own risk.

\ I mean to the people who find the current restrictions unacceptable and need an immediate fix. I don't mean you personally. \</>

With that said, I'll leave it in the capable hands of @Rokt33r to make a final verdict.

Redsandro commented 6 years ago

Cross-post https://github.com/BoostIO/Boostnote/issues/1644#issuecomment-373241268

Danger Zone


:radio_button: :heavy_check_mark: Only allow secure html tags (recommended) :white_circle: :warning: Allow styles :white_circle: :x: Allow dangerous html tags

:information_source: 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.

0d-gg commented 6 years ago

@Redsandro I definitely respect your conservative approach to the security of the product, especially since I am almost always dealing with the opposite! I agree about Electron - it almost feels like the Adobe Flash of 2018 with the various security issues related to it, haha.

While, sure, it is possible that an XSS vulnerability in Chrome that allows for XSS via CSS could be discovered, this would be a HUGE deal. To my knowledge, Chrome has never allowed JS to be executed via CSS, and no such vector has existed in any modern browser for quite a number of years. There are a lot more high value targets out there that would be picked on first before Boostnote (fortunately for Boostnote!) if such an attack vector was discovered.

All I am really trying to do here is describe the risk as accurately as possible. I am not personally advocating that allowing arbitrary CSS is 100% safe, but rather - if we allow style attributes, I think we should allow the style tag just to save people the pain of using inline styles. The attack surface is the same. Blocking both is another story. I personally feel just as safe with arbitrary styles being allowed as I do with links being added (even if it's part of MD, I found some potential issues that I will share when I get some time).

I understand that a lot of notes are stored on dropbox (or similar), but we should also keep in mind that that such an attack would require a very advanced attacker and/or be heavily targeted. If I had access to your dropbox and wanted to take over your computer, I'd definitely look for executables first before finding a 0-day in a relatively small (but growing!) product. You know what I mean? It's not that your concern is crazy, it's just that I think the likelihood is very low due to all the moving pieces. It'd be different if there was an easy/known way of getting XSS/RCE (like before).

Sorry for being sooooo wordy! I know some of this probably belongs in another thread, but it's all kinda related.

Redsandro commented 6 years ago

@pmood In an attempt to summarize your case, I think you are saying the same things as in your previous message.

  1. if we allow style attributes, (..) we should allow the style tag (too)
  2. the likelihood (of an attack) is very low

My position is the following.

  1. We should not (have) allow(ed) style attributes in the first place
  2. We should not force all people to take a security risk that some people think is unacceptable

And the solution is an option so that users may opt-in to these as proposed in https://github.com/BoostIO/Boostnote/issues/1644#issuecomment-373241268

Rokt33r commented 6 years ago

My position is the following.

We should not (have) allow(ed) style attributes in the first place We should not force all people to take a security risk that some people think is unacceptable

It sounds reasonable.👍

Rokt33r commented 6 years ago

I'm going to make everything optional today. Sorry for being late.