XSS Refactor: Replace showdown-xss with in-house sanitizer
#724
Conversation
There was a problem hiding this comment.
Looking really good @dmsnell! I tested with a bunch of malicious content and it was all stripped or removed. There is a conflict now to take care of 🙄
Found a few things that might be good to fix here or we can do so in followup PRs.
| @@ -251,7 +242,9 @@ export const NoteEditor = React.createClass({ | |||
| <div | |||
| style={printStyle} | |||
| className="note-print note-detail-markdown" | |||
| dangerouslySetInnerHTML={{ __html: noteContent }} | |||
| dangerouslySetInnerHTML={{ | |||
| __html: markdownEnabled ? renderNoteToHtml(content) : content, | |||
There was a problem hiding this comment.
I noticed that my malicious note would be rendered quickly when printing (if markdown was disabled for the note), maybe we should always pass this through renderNoteToHtml for safety's sake?
There was a problem hiding this comment.
oh you know what…we shouldn't be setting __html to content if Markdown is disabled. it's not HTML so it shouldn't be treated that way - this is a separate issue so let's do it in another PR (because we'll have to make sure the styling all works out
{ markdownEnabled ? (
<div
style={printStyle}
className="note-print note-detail-markdown"
dangerouslySetInnerHTML={{ __html: renderNoteToHtml( content ) }}
/>
) : (
<div style={printStyle} className="note-print note-detail">{ content }</div>
) }
lib/utils/sanitize-html.js
Outdated
| ({ name, value }) => | ||
| !isAllowedAttr(tagName, name) || | ||
| // only valid http(s) URLs are allowed | ||
| (('href' === name || 'src' === name) && !validUrl.isWebUri(value)) |
There was a problem hiding this comment.
How about mailto links? The markdown for that is typically:
[emailme](mailto:dennis@raddude.com)
| const tagName = node.nodeName.toLowerCase(); | ||
|
|
||
| if ('input' === tagName) { | ||
| return 'checkbox' === node.getAttribute('type'); |
There was a problem hiding this comment.
I noticed that checkboxes render with bullets, I think we should remove those so they look more like GitHub:
There was a problem hiding this comment.
I'd be more keen to fix this in a separate PR since this one right now is solely there for security. The rendering of bullets is going to be more related to how showdown generates the Markdown than how we sanitize it.
dmsnell
force-pushed
the
refactor/xss-protection
branch
from
9e4eef8 to
4a7eee2
Compare
See #681 See #694 See #721 The `showdown-xss` library wasn't really doing for us what we wanted. It was transforming our checkbox inputs and making them display as plain text instead of as actual HTML. In this patch I've removed `showdown-xss` and created a new centralized function to render HTML which calls our custom sanitization. Why do it ourselves? We don't need the kind of sanitiation which only removes the malicious code and leaves as much as it can untouched. Instead we are free here to strip out basically everything except for a few white-listed tags and attributes since we ourselves are the ones producing the output; we don't have to support full HTML in the notes. This patch should guard against everything on the OWASP list of XSS attacks. It will remove significant styling and custom HTML but when a tag is removed it will usually just take out the tag itself and leave the inner content as plain text. Some tags are "forbidden" and all their children are removed with them.
dmsnell
force-pushed
the
refactor/xss-protection
branch
from
4a7eee2 to
3a7574d
Compare
|
Updated @roundhill to allow email links but I left the other comments for separate issues. Thoughts? |
|
Nice, I think we're good to go here. |
See #681
See #694
See #721
The
showdown-xsslibrary wasn't really doing for us what we wanted.It was transforming our checkbox inputs and making them display as plain
text instead of as actual HTML.
In this patch I've removed
showdown-xssand created a new centralizedfunction to render HTML which calls our custom sanitization.
Why do it ourselves? We don't need the kind of sanitiation which only
removes the malicious code and leaves as much as it can untouched.
Instead we are free here to strip out basically everything except for
a few white-listed tags and attributes since we ourselves are the ones
producing the output; we don't have to support full HTML in the notes.
This patch should guard against everything on the OWASP list of XSS
attacks. It will remove significant styling and custom HTML but when a
tag is removed it will usually just take out the tag itself and leave
the inner content as plain text. Some tags are "forbidden" and all their
children are removed with them.
cc: @Jackymancs4
With the contributions and discussion about the checkboxes I really wanted
to get that bug fixed but I was also uneasy about the proposed solutions in
#721 and #714. I was uneasy because our XSS libraries were already opaque
and less-maintained than we would want. Digging into libraries also revealed
custom HTML parsing code that I wasn't sure I trusted.
In this proposal we're relying on the browser to parse the HTML and walk
through it node-by-node giving us the best possible parse with the protection
the browser already has coded into it against attacks that target the parser…
…since we're generating markup and we don't promise "everything Markdown
can do is available here" then we can produce whatever subset of Markdown
we want to support. I have chosen to try and mimic the allowed tags and attributes
as is currently supported in the web version of Simplenote. We can add more
tags such as
<abbr>and<acronym>and I think if we wanted to we couldopen up some styling or class attributes but for now I'm reaching for harmony
across the platforms more than one being better than others.
Testing
Try to break rendering with malicious attacks and also verify that checkboxes
and expected output appears.