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

Another Filter bypass leading to XSS #348

Closed
TheGrandPew opened this issue Apr 13, 2020 · 12 comments · Fixed by #353
Closed

Another Filter bypass leading to XSS #348

TheGrandPew opened this issue Apr 13, 2020 · 12 comments · Fixed by #353

Comments

@TheGrandPew
Copy link

On the latest release (2.3.8) a payload like this one can lead to xss and bypass safe_mode when set to true.

<lol@/ //id="pwn"//onclick="alert(1)"//**abc**

The Problem:
I think its due to just bad regex's not detecting non alphanumeric tags.

poc

@avramit
Copy link

avramit commented Apr 13, 2020

It seems like the parser doesn't escape tags that don't match the following pattern, so everything that isn't [a-zA-Z0-9_] can be used to bypass the escaping mechanism

_incomplete_tags_re = re.compile("<(/?\w+[\s/]+?)")

The following payload will also work:

<x- onclick="alert(1)"*Click Me*

@TheGrandPew
Copy link
Author

TheGrandPew commented Apr 14, 2020

Has been assigned CVE-2020-11888.

@xurble
Copy link

xurble commented May 2, 2020

I didn't see the PR from @v1dhun before I submitted mine. However, having had more time to think about it, they're both flawed.

Mine can be defeated by this:
<x:// onclick="alert(1)"*Click Me*

The other can be defeated by this
<x- onclick ="alert(1)"*Click Me*
Also, it breaks a bunch of unit tests.

This needs a little more thought

@xurble xurble mentioned this issue May 2, 2020
@v1dhun
Copy link

v1dhun commented May 3, 2020

HI @xurble ,
I updated the code with checking white-spaces, Now its working fine.Could you please check it

@huntr-helper
Copy link

👋 Hey! We've recently opened a bug bounty against this issue, so if you want to get rewarded 💰 for fixing this vulnerability 🕷, head over to https://huntr.dev!

@xurble
Copy link

xurble commented May 4, 2020

@v1dhun I think we've both fixed it right now. Either would be OK, or the maintainers might have a better idea altogether.

@kravietz
Copy link

kravietz commented May 4, 2020

Regex is going to be always bypassed as it assumes specific syntax while combinations for a renderable HTML are practically unlimited. This is one of the fundamental recommendations from OWASP XSS Prevention Cheat-sheet.

I'd recommend running the input text through bleach which is a whitelist-based HTML sanitizer.

I had a look at the markdown2 code but to be honest I don't know the code base enough to be able to see see an obvious place to plug it in.

@nicholasserra
Copy link
Collaborator

I merged @xurble PR as it was on the main repo. LGTM, thank you!

I'm a little hesitant to introduce another library like bleach to sanitize final output. But it may be a good solution. Needs investigating.

@kravietz
Copy link

kravietz commented May 4, 2020

@nicholasserra Yes, that's the classic dilemma. As developer I would probably prefer a big fat notice in the documentation stating that the main purpose of the library is not to sanitize untrusted code but to render Markdown, just to set the expectations right.

@thmo
Copy link

thmo commented May 8, 2020

Does this warrant a new release?
The 2.3.8 release is about a year old...

@xurble
Copy link

xurble commented May 9, 2020

If it sways your thinking, the library is flagged by sentry, which is how I discovered the issue.

@nicholasserra
Copy link
Collaborator

2.3.9 is now released

onegreyonewhite added a commit to vstconsulting/polemarch that referenced this issue May 12, 2020
Changelog:
*  Fix CVE-2020-11888 in markdown2.

Additional info:
*  trentm/python-markdown2#348
*  GHSA-fv3h-8x5j-pvgq

See merge request polemarch/ce!194
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants