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

Fix filter bypass leading to XSS (#362) #434

Merged
merged 1 commit into from
Jul 17, 2022

Conversation

Crozzers
Copy link
Contributor

As reported by @terjanq:

The new regex _incomplete_tags_re = re.compile("<(/?\w+?(?!\w).+?[\s/]+?)") in #351 introduced a more severe bypass on any HTML element by using a new line that does not match to .+.

In [2]: markdown2.markdown('<iframe\nonload=alert()//',safe_mode=True)
Out[2]: '<p><iframe\nonload=alert()//</p>\n'

I was able to reproduce this issue.
The proposed change ignores whitespace between the tag name and the .+ and from my tests this seems to do the trick.

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

However, I will admit that I'm not a security guy so it's possible that this change has it's own issues.

Furthermore, I was unable to reproduce the second example given in #362, so that might already have been fixed.

@nicholasserra
Copy link
Collaborator

Thanks, will take a close look at this soon.

@nicholasserra
Copy link
Collaborator

@terjanq wondering if you'd peek this fix, see if you notice any issues with it

@nicholasserra
Copy link
Collaborator

I wasn't able to reproduce the first issue anymore at all. But this looks fine to merge anyway.

@nicholasserra nicholasserra merged commit 3669913 into trentm:master Jul 17, 2022
@Crozzers Crozzers deleted the fix-xss-filter-bypass branch July 18, 2022 11:24
@HavayaG
Copy link

HavayaG commented Jul 18, 2022

Hey @nicholasserra,
My name is Havaya, and I’m contacting you on behalf of the Snyk security team.
We tried to reach out to you regarding possible vulnerabilities in the package but we couldn’t get an answer, also, there is no SECURITY.md.
I was wondering how or who should we contact to disclose vulnerabilities privately?
Many thanks!

@SuperSandro2000
Copy link

Can you tag a release with this fix? Thanks!

@nicholasserra
Copy link
Collaborator

@SuperSandro2000 2.4.4 has been released with this and another xss fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants