Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Cross-site Scripting (XSS) via URI #925

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

Cross-site Scripting (XSS) via URI #925

buglloc opened this issue Aug 15, 2017 · 5 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

Browsers support both lowercase and uppercase x in hexadecimal form of HTML character entity (tested on Chromium && FF).
But marked unescape only lowercase:

// https://github.com/chjj/marked/blob/master/lib/marked.js#L1096-L1108

function unescape(html) {
	// explicitly match decimal, hex, and named HTML entities 
  return html.replace(/&(#(?:\d+)|(?:#x[0-9A-Fa-f]+)|(?:\w+));?/g, function(_, n) {
    n = n.toLowerCase();
    if (n === 'colon') return ':';
    if (n.charAt(0) === '#') {
      return n.charAt(1) === 'x'
        ? String.fromCharCode(parseInt(n.substring(2), 16))
        : String.fromCharCode(+n.substring(1));
    }
    return '';
  });
}

This allow attacker to create link with javascript code.
For example, this code:

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

text = `
lower[click me](javascript:...)lower
upper[click me](javascript:...)upper
`;

console.log(marked(text));

Will render:

<p>lowerlower
upper<a href="javascript&#X3a;...">click me</a>upper</p>

Browser example: https://www.buglloc.com/marked.html
Tested on Marked v0.3.6 + Chromium 60.0.3112.90 and Firefox 55.0.1

@matt-
Copy link
Contributor

matt- commented Aug 29, 2017

You are right. this should should be a case-insensitive match:

html.replace(/&(#(?:\d+)|(?:#x[0-9A-Fa-f]+)|(?:\w+));?/ig

unfortunately the maintainer is not longer around to push a change to npm. I will push a change to the master branch but unfortunately that may be as far as this change goes.

@CSEMike
Copy link

CSEMike commented Oct 25, 2017

Hi Matt -- did this issue ever result in a pull request? Thanks!

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

See #937

@joshbruce joshbruce mentioned this issue Dec 1, 2017
@matt- matt- reopened this Dec 4, 2017
@matt-
Copy link
Contributor

matt- commented Dec 4, 2017

Just tested this and it is still an issue. Why was this closed? what tests were added?

@joshbruce
Copy link
Member

See #926

# for free to join this conversation on GitHub. Already have an account? # 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.

4 participants