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

HTML tags not rendered anymore? #1672

Closed
tlwt opened this issue Mar 13, 2018 · 16 comments
Closed

HTML tags not rendered anymore? #1672

tlwt opened this issue Mar 13, 2018 · 16 comments
Assignees
Labels
discussion 💬 Issue concerns a discussion.

Comments

@tlwt
Copy link

tlwt commented Mar 13, 2018

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 Rokt33r added the discussion 💬 Issue concerns a discussion. label Mar 13, 2018
@Rokt33r
Copy link
Member

Rokt33r commented Mar 13, 2018

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 Rokt33r self-assigned this Mar 13, 2018
@Rokt33r
Copy link
Member

Rokt33r commented Mar 13, 2018

If someone has another idea, let me know.

@tlwt
Copy link
Author

tlwt commented Mar 13, 2018

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
Copy link

0d-gg commented Mar 14, 2018

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
Copy link
Member

Rokt33r commented Mar 14, 2018

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
Copy link
Member

Rokt33r commented Mar 14, 2018

@tlwt Done. #1677

@emrusso
Copy link

emrusso commented Mar 14, 2018

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

@Rokt33r
Copy link
Member

Rokt33r commented Mar 14, 2018

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

@Redsandro
Copy link
Contributor

Redsandro commented Mar 14, 2018

@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 #1644 (comment)

@0d-gg
Copy link

0d-gg commented Mar 14, 2018

@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
Copy link
Contributor

Redsandro commented Mar 15, 2018

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 #1644 (comment).

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

<instant update> 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
Copy link
Contributor

Cross-post #1644 (comment)

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.

@0d-gg
Copy link

0d-gg commented Mar 15, 2018

@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
Copy link
Contributor

@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 #1644 (comment)

@Rokt33r
Copy link
Member

Rokt33r commented Mar 15, 2018

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
Copy link
Member

Rokt33r commented Mar 20, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion 💬 Issue concerns a discussion.
Projects
None yet
Development

No branches or pull requests

5 participants