Safe Mode XSS (escape only)

[JorianWoltjer]

Describe the bug

It's possible to bypass the safe mode (escape only, not replace)

To Reproduce

Run markdown2 with safe_mode="escape" on the following markdown input:

![A](B "<C D="E" onerror=alert(origin) >")

The output HTML is:

<p><img src="B" alt="A" title="&lt;C D="E" onerror=alert(origin) &gt;" /></p>

The browser sees this as an image with src, alt, title and an onerror attribute. The onerror executes JavaScript, causing XSS.

This was found through fuzzing so I'm not sure exactly why it happens, but this was the minimal PoC.

Debug info
Version of library being used: c2d73a3

Any extras being used: no

[JorianWoltjer]

Together with #702 and some previous reports by others, I think this shows a pattern that the current way of preventing XSS is very brittle.

A better solution to handle all cases might be to follow other markdown libraries in ignoring the XSS risk, simplifying this repo in the process, and making clear to users that they should run a well-known sanitizer like bleach on the output themselves (or markdown2 could even do this by default).

[Crozzers]

Yeah. It does feel like fighting a losing battle at times, and I do understand the rationale behind why other libs have gone that route. You're right, it would also massively simplify this library. We could very easily add safe mode sanitisation as an optional extra, like wavedrom or syntax highlighting, and it would probably do a far better job.

That being said, I feel like having at least a "meh, good enough in most cases" implementation is a bit of a value add? I'm not sure how many users feel that way but given that we regularly get XSS bug reports, some people must like it. Maybe at some point I could find a sanitisation library and run its test suite against this lib, see how it stacks up and whether it's worth maintaining if it's nowhere close.

Doing a brief search through the issues, there have been ~132 raised since 1st Jan 2022, of which only 10 mention XSS, so maybe 8%. And 3 of them (#691, #699 and this one) are down to roughly the same issue - smuggling things into image attributes.

---

Overall I don't know whether it's worth giving up on it yet. I definitely think it's worth recommending a better solution in wiki and docs (although it seems bleach is deprecated).

But simplifying this codebase is a very strong argument. Why are we duplicating the work of sanitisation libs?

@nicholasserra thoughts?

[JorianWoltjer]

I've run the fuzzer on the new branch (https://github.com/Crozzers/python-markdown2/tree/702-703-xss-issues) and found the following extra bypasses:

1. Using HTML-entities in place of the colon gets decoded by the browser, so still executes JavaScript on click:

[x](javascript:alert(origin))

<p><a href="javascript&#58;alert(1)">x</a></p>

2. Using 1/ inside javascript: URI it also doesn't get recognized, XSS on click:

[x](javascript:1/alert(origin))

<p><a href="javascript:1/alert(origin)">x</a></p>

3. Another weird image attribute quoting issue allowing for onerror= injection:

- 
- ![](x '`![](`') onerror=alert(origin) )

<ul>
<li>
<ul>
<li><img src="x" alt="" title="&lt;code&gt;<img src="&lt;/code&gt;&quot; /&gt; onerror=alert(origin) " alt="" /></li>
</ul></li>
</ul>

---

In my opinion having a "meh, good enough in most cases" implementation will lead users to a false sense of security. For now I'm happy to keep searching for different issues but to be confident in there being no weird bypasses, I'd opt for a regular HTML sanitizer pass in safe mode (since markdown generates pretty strict HTML anyway).

[nicholasserra]

I don't have strong opinions. I think no matter what we should probably be warning about XSS and saying to use something else for safety. Given bleach is no-go, what are people even using today?

I guess I like that we make an attempt to sanitize. Could probably do both for now. Add a warning to the main README and also try to keep on top of these.

[Crozzers]

I've run the fuzzer on the new branch (https://github.com/Crozzers/python-markdown2/tree/702-703-xss-issues) and found the following extra bypasses:

On my radar, forgot to go and mark the PR as draft

Given bleach is no-go, what are people even using today?

Not even sure. I'll look into it and add something to the readme or wiki

[Crozzers]

Submitted follow-up PR to fix those issues.

From what I can gather, many people have moved to nh3 for sanitisation, but there are some other options too.

I've added a safe mode wiki page which goes over some of the concerns raised here, and strongly recommends the use of a 3rd party lib

[nicholasserra]

Thank you for the wiki write up