XSS reported by npm audit

[Hypnosphi]

https://www.npmjs.com/advisories/1219

All versions of simple-markdown are vulnerable to Cross-Site Scripting. Due to insufficient input sanitization the package may render output containing malicious JavaScript. This vulnerability can be exploited through input of links containing data or VBScript URIs and a base64-encoded payload.

[Hypnosphi]

I have no idea why it refers to simple-markdown though

[EoghanBonass]

I have no idea why it refers to simple-markdown though

Could be because markdown-to-jsx is a fork of simple-markdown.

[quantizor]

It is technically, though I've rewritten the majority of the library. If anyone wants to take a stab at fixing it, open to a PR. Not looking to add much payload for it though.

[esimons]

I'm confused by the advisory -- wouldn't the more obvious XSS exploit be to simply inline a <script> tag? markdown-to-jsx doesn't advertise itself as doing any sanitization, and unless it intends to start doing that I don't see that addressing the specific attack vector detailed in the advisory, in isolation, seems like it mitigates the overall concern.
Maybe the solution here is to make the lack of sanitization / possibility for XSS explicit in docs? I'm curious what the NPM advisory policy is around this.

[quantizor]

I agree. 🤷‍♂️

[quantizor]

A potential solution would be to not render script tags by default without an opt-in option I guess.

[esimons]

Secure-by-default is always a good position. :)
... unless there's a specific technical limitation around bundle size that prevents inclusion of sanitization logic within the same package -- meaning you have to split into "unsafe" and "safe" packages rather than providing the option at runtime. In which case I'd think that, so long as the package is explicitly advertised as not being safe for use with untrusted content, it wouldn't be subject to this kind of advisory... but that's supposition on my part, I really don't have a clear understanding of npm's policy around this.

[fabb]

Well I guess npm‘s support can be reasoned with 😊: [email protected]

[esimons]

@fabb I'm actually drafting an email right now :)

[ariabuckles]

Hey, I maintain simple-markdown.

npm audit is unfortunately the first I'm hearing of this? I put in a bunch of anti-xss validation years ago but after this fork, so it might be helpful to look through those. That said, simple-markdown also explicitly does not support inline html

I can take a look and see if i can fix the vulns here; do any contributors to this project have more context/repros?

[ariabuckles]

Ok, after looking at this more, I believe that advisory is not about script tags (although... that would be worth considering too), but just about malicious data: or vscript: urls. #307 pulls some fixes for those cases from simple-markdown and adds test cases

[ariabuckles]

Just published 6.11.4, which I believe mitigates this vulnerability. I'll keep an eye on the status of this and check that the vulnerability report agrees with that.

If you run into trouble with 6.11.4 or believe it didn't solve the issue; please comment here or make a new issue :).

(I'll look into the script tag vulnerability next week, as that may take a bit more time to understand.)

[ariabuckles]

And thanks @probablyup for your help writing up how to contribute and helping me publish this ❤️

[ariabuckles]

Well, the advisory says it still considers 6.11.4 is vulnerable, but I haven't found any more information about how or why (whether that's I need to somehow mark the version as safe, or if there's an automated script checking the vuln and where I can find out more about it).

If anyone has experience with these I'd love their help, otherwise I'll be asking around on twitter to learn what the right next steps are.

[Bazze]

Well, the advisory says it still considers 6.11.4 is vulnerable, but I haven't found any more information about how or why (whether that's I need to somehow mark the version as safe, or if there's an automated script checking the vuln and where I can find out more about it).

If anyone has experience with these I'd love their help, otherwise I'll be asking around on twitter to learn what the right next steps are.

Looking at another security issue I'm following it seems that you might have to reach out to npm to get this new version cleared!

[ariabuckles]

@Bazze ah, really helpful! thank you!

[KevinOl]

Any updates on this one?

[acroyear]

well, as we've been on a holiday weekend here in the states and other spots like the UK(even with most tech workers at home) it may be that it won't get reviewed by NPM until after tomorrow.

[ariabuckles]

No response from npm yet; if they don't respond tomorrow I'll follow up. As @acroyear says, it's a holiday here so npm support is likely not working until tomorrow

In the meantime, please upgrade to 6.11.4, which should be safe from this vulnerability, and I think will pass npm audit once npm support looks at it.

[fabb]

6.11.4 has been whitelisted 🎉

Thanks everyone!

[quantizor]

Awesome! Great job to everyone involved ❤️

[rwhogg]

Hello everyone: I'm listed as the reporter of https://www.npmjs.com/advisories/1219, so I'll comment here even though this is already closed. I apologize for not doing so sooner - I only learned yesterday that this issue was opened.

First of all, big thanks to @ariabuckles and @probablyup . I'm very glad to see this issue resolved and the speed with which you responded to this issue is very impressive.

Second, I can confirm from my end that 6.11.4 fixes the issue I reported. I'll refrain from posting the specific details here (unless Evan would prefer I do so) but I will say that from my point of view this is definitely resolved.

Third, I apologize for mishandling the communication around this issue. I should have been watching this repo and also should have reached out to npm about this issue more frequently, but unfortunately did not. I'm sorry for the pain and frustration this has caused.

When I initially contacted npm, I used the following language in describing the issue:

This bug seems to be inherited from the simple-markdown package - it's
basically the same as https://www.npmjs.com/advisories/815

This is the reason that the advisory mentioned VBScript and data URI's in particular: because I was specifically checking if https://www.npmjs.com/advisories/815 applied to markdown-to-jsx. I now feel this language was probably too ambiguous.

I agree that advertising that the library does not guarantee untrusted data will be sanitized would also have been an appropriate fix.

I have not personally communicated with npm about this issue since October. It is entirely possible that they attempted to contact me but the email never reached me, or I otherwise lost it. Either way, I should have been significantly more proactive in speaking with them about this.

I realize most of the hard work was already done before I commented, but if there is any assistance I can provide: please feel free to reach out.

[ariabuckles]

Hi @rwhogg !

Thanks for reporting this! Although I was confused initially, I'm really glad to get the chance to address it and think it's important to! I figured out the vbscript/data thing once I started looking at it since I was also familiar with https://www.npmjs.com/advisories/815 :). Thanks for the comment and for the offer to help!

And it's really helpful to have your confirmation that 6.11.4 fixes this!

Unrelatedly: npm responded to me and it seems like my initial reach out to them at [email protected] didn't make it to their issue tracker until I CC'd [email protected] ; I'll follow up with them about that so that maybe next time it gets resolved a bit faster :).

[glasser]

Is it the goal of markdown-to-jsx that you should be able to render arbitrary untrusted Markdown and include in your React app without creating XSS vulnerabilities (and any exception to this is a bug in markdown-to-jsx, not a misuse)? I see various issues like this out about fixing XSS bugs but the docs don't actually say whether one ought to feel comfortable using this package on untrusted Markdown in the first place.

[quantizor]

Is it the goal of markdown-to-jsx that you should be able to render arbitrary untrusted Markdown and include in your React app without creating XSS vulnerabilities (and any exception to this is a bug in markdown-to-jsx, not a misuse)? I see various issues like this out about fixing XSS bugs but the docs don't actually say whether one ought to feel comfortable using this package on untrusted Markdown in the first place.

At the end of the day, allowing user input universally creates vulnerability. We do common sense things in the library to handle some typical attack vectors but the overall goal of the library is to be lightweight and extensible.