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

repair a ReDoS vulnerability in markdown2.py #494

Merged
merged 2 commits into from
Oct 9, 2023

Conversation

DarkTinia
Copy link
Contributor

This PR fixed #493
It uses a loop and some checking to avoid using regex that contains ReDoS vulnerabilities

@Crozzers
Copy link
Contributor

Crozzers commented May 9, 2023

Is there a reason why you included re2 as a dependency instead of the official google-re2? I'm not familiar with either of these so I'm likely ignorant to the obvious answer.

Other than that, is there a way to do this without requiring 3rd party dependencies? One of the main selling points of this library is it's portability and self contained nature, requiring just the lib/markdown2.py file to function.

Looking at the _strong_re regex, I've found that removing the lookbehind was sufficient to negate the ReDoS

- _strong_re = re.compile(r"(\*\*|__)(?=\S)(.+?[*_]*)(?<=\S)\1", re.S)
+ _strong_re = re.compile(r"(\*\*|__)(?=\S)(.*\S)\1", re.S)
- _em_re = re.compile(r"(\*|_)(?=\S)(.+?)(?<=\S)\1", re.S)
+ _em_re = re.compile(r"(\*|_)(?=\S)(.*?\S)\1", re.S)

Is there a particular requirement for the re2 library?

@alsotop
Copy link

alsotop commented Sep 21, 2023

Given there's been a lack of discussion on this topic, but this vulnerability is still being reported by safety/Snyk/etc, is there any potential for implementing the fix in a way that @Crozzers has described?

https://security.snyk.io/vuln/SNYK-PYTHON-MARKDOWN2-3247624

@Crozzers
Copy link
Contributor

Crozzers commented Sep 24, 2023

@DarkTinia, I cloned this branch and added my suggested changes but can't figure out how to submit a PR to your fork. The fork is here if you want to pull the changes in.

If not, I'm happy to submit a PR to the main repo from that branch

@nicholasserra
Copy link
Collaborator

Seems fine to just do our own thing here too if it gets the redos fixed up enough. Can mention this MR in commit or something.

nicholasserra added a commit that referenced this pull request Oct 9, 2023
@nicholasserra nicholasserra merged commit ea615a9 into trentm:master Oct 9, 2023
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.

A ReDoS vulnerability exists in markdown2.py
4 participants