search

trentm / python-markdown2

markdown2: A fast and complete implementation of Markdown in Python
Other
2.66k stars 433 forks source link

repair a ReDoS vulnerability in markdown2.py #494

Closed DarkTinia closed 1 year ago

DarkTinia commented 1 year ago

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

Crozzers commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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.