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

XSS with HTML entities #592

Merged
merged 3 commits into from Jul 29, 2016
Merged

XSS with HTML entities #592

merged 3 commits into from Jul 29, 2016

Conversation

matt-
Copy link
Contributor

@matt- matt- commented May 19, 2015

With the sanitize option on it is possible to create a link with a javascript: protocol with the following: [URL](javascript&#58document;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&#58document;" 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;".

@gscottolson
Copy link

@gscottolson gscottolson commented Sep 24, 2015

👍 This seems pretty significant.

@matt-
Copy link
Contributor Author

@matt- matt- commented Sep 24, 2015

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

@matt- matt- commented Jan 25, 2016

screenshot 2016-01-25 12 34 30

I created a fork and this PR, but the real solution is to use something else. I am recommending that the node security project mark this project as vulnerable and abandoned.

@alexkravets
Copy link

@alexkravets alexkravets commented Jan 25, 2016

@matt- any recommendation for alternatives?

@matt-
Copy link
Contributor Author

@matt- matt- commented Jan 25, 2016

https://github.com/jonschlinkert/remarkable has been great for me.

@alexkravets
Copy link

@alexkravets alexkravets commented Jan 26, 2016

@matt- thanks a ton!

@Albert-IV
Copy link

@Albert-IV Albert-IV commented Jan 26, 2016

That moment when you realize you didn't comment on an open issue, but rather an open PR. Whelp.

@uptownhr
Copy link

@uptownhr uptownhr commented Apr 18, 2016

when will this be pulled?

dominicbarnes added a commit to dominicbarnes/deku-forms that referenced this issue Apr 20, 2016
marked has a pretty nasty XSS vulnerability, and doesn't appear to
be an active project anymore. (see markedjs/marked#592)
@ethanrubinson
Copy link

@ethanrubinson ethanrubinson commented Apr 21, 2016

+1 Can this please be merged in so we can remove the advisory?

@oriweingart
Copy link

@oriweingart oriweingart commented Apr 24, 2016

+1

zachmullen added a commit to girder/girder that referenced this issue Apr 25, 2016
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
@danactive
Copy link

@danactive danactive commented Apr 25, 2016

+1

@developit
Copy link

@developit developit commented Apr 27, 2016

@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).

@uptownhr
Copy link

@uptownhr uptownhr commented Apr 27, 2016

the question is, where is the repo maintainer?

@developit
Copy link

@developit developit commented Apr 27, 2016

Perhaps on vacation?

@drastick
Copy link

@drastick drastick commented Apr 27, 2016

+1

@drastick
Copy link

@drastick drastick commented Apr 27, 2016

Vacation for a year? Must be nice.

@emveeoh
Copy link

@emveeoh emveeoh commented Jun 2, 2016

@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.

@timjrobinson
Copy link

@timjrobinson timjrobinson commented Jun 2, 2016

This appears to be no longer reproducible. I believe the author fixed it and just ignored this PR. Can anyone still reproduce it with the latest version of Marked?

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.

@matt-
Copy link
Contributor Author

@matt- matt- commented Jun 3, 2016

"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:

> var md = require("marked");
> md.setOptions({sanitize: true})
> md("[URL](javascript:alert(1))");
'<p></p>\n'

Example this PR resolves with bad entities:

> var md = require("marked");
> md.setOptions({sanitize: true})
> md("[URL](javascript&#58document;alert&#40;1&#41;)");
'<p><a href="javascript&#58document;alert&#40;1&#41;">URL</a></p>\n'

If you think this is still some how resolved please read this blog to better understand the issue:
https://snyk.io/blog/marked-xss-vulnerability/

@matt-
Copy link
Contributor Author

@matt- matt- commented Jun 3, 2016

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.

@mvhenten
Copy link

@mvhenten mvhenten commented Jun 3, 2016

@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.

@timjrobinson
Copy link

@timjrobinson timjrobinson commented Jun 3, 2016

@matt- sorry I work with @mvhenten and we thought it was resolved but it was only not affecting us due to post-processing. This is still a valid issue. Thanks for the test cases.

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

Successfully merging this pull request may close these issues.

None yet