-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
XSS with HTML entities #592
Conversation
👍 This seems pretty significant. |
I assume this project is dead. I cant see letting an XSS go for this long otherwise. I have moved to a new Lib in my project. |
@matt- any recommendation for alternatives? |
https://github.com/jonschlinkert/remarkable has been great for me. |
@matt- thanks a ton! |
That moment when you realize you didn't comment on an open issue, but rather an open PR. Whelp. |
when will this be pulled? |
marked has a pretty nasty XSS vulnerability, and doesn't appear to be an active project anymore. (see markedjs/marked#592)
+1 Can this please be merged in so we can remove the advisory? |
+1 |
The marked library that we were using to render markdown has been abandoned by its creator and contains a critical XSS vulnerability. markedjs/marked#592 https://www.npmjs.com/package/remarkable
+1 |
@matt- Remarkable looks great (very very clean) but it's about 6x the size of Marked. For some use-cases (client-side) it seems like Marked would be preferable on those grounds (though for anything else I'm probably going to be using remarkable given the focus on performance). |
the question is, where is the repo maintainer? |
+1 |
Vacation for a year? Must be nice. |
@matt or anyone else... Any chance that you would be willing to help the Modernizr js (https://github.com/Modernizr/Modernizr) swap out its dependency from Marked to Remarkable? I'm too new at programming javascript to take on the task myself. |
EDIT: I was mistaken, this is still valid. Just had some more post processing that was causing it to not be reproducible in my app. |
"Latest commit 88ce4df on Jul 31, 2015" This issue is not resolved and has not been even been touched. The exact example you gave is the XSS issue. (Put that HTML in a browser and click on it.) javascript: (in any form even with html entities) should be blocked in sanitize mode. Example where marked works correctly:
Example this PR resolves with bad entities:
If you think this is still some how resolved please read this blog to better understand the issue: |
From the first sentence in this PR: "With the sanitize option on" This lib has a sanitize mode that is intended to block normal HTML and prevent xss. https://github.com/chjj/marked#sanitize It also filters "javascript:" and "vbscript:" as intended in this mode. This is an example of bypassing that with html entities. Executing javascript (an XSS) is much MUCH worse than "plain old hyperlinks". This is an abandoned project with an open and very clear security issue.. not exactly what I consider FUD. |
@matt- I was able to reproduce this in indeed. I may have been confused due to the fact that post-process links and images, mitigating this issue. |
With the sanitize option on it is possible to create a link with a javascript: protocol with the following:
[URL](javascript:document;alert(1))
.HTML entities in the browser are not strict and parse what they can and leaving the rest behind. For example
&#xNNanything;
would parse the NN hex values but leave behind the string "anything;"."
javascript:document;
" with the regex/&([#\w]+);/
returns "58document
" and is parsed by String.fromCharCode to "". Because of this the later tests only sees the javascript keyword without the :. However the browser parses this to: "javascript:document;
".