Vulnerable Regular Expression in svnwc.py

[yetingli]

Type of Issue
Potential Regex Denial of Service (ReDoS)

Description
The vulnerable regular expression is located in

py/py/_path/svnwc.py

Line 399 in 1a45e12

rex_blame = re.compile(r'\s*(\d+)\s*(\S+) (.*)')

The ReDOS vulnerabilitiy of the regex is mainly due to the sub-pattern (\d+)\s*(\S+) and can be exploited with the following string
"1"*5000

I think you can limit the input length or modify this regex.

[bluetech]

Ohh nice catch.

I think the ambiguity in this regex is \d+\s*\S+ with zero spaces, that is \d+\S+.

From the examples listed here, the line number and the user name are always separated by at least one space (which makes sense). So we can change the middle \s* to \s+, i.e.

rex_blame = re.compile(r'\s*(\d+)\s+(\S+) (.*)') 

and that should fix it, at least I don't think there is any catastrophic backtracing left here. Does that sound right to you?

[yetingli]

yeah, I think the fixed regex is safe.

[bluetech]

Thanks for confirming. I'll reopen for now and submit a PR to fix this.

[msmeissn]

Is this code reachable by attackers in any form?

e.g. should this be tracked by a CVE?

[yetingli]

Hi @msmeissn,
Thank you for your attention!

Is this code reachable by attackers in any form?

I think attackers may obtain this code by calling py.path.svnwc

should this be tracked by a CVE?

It would be great to assign a CVE for the issue🙂

Best regards,​​
Yeting

[msmeissn]

Mitre assigned CVE-2020-29651 to this issue.

[earthgecko]

HI @bluetech maybe this is a question you may be able to answer.

Although the project is in maintenance mode, does #257 not warrant a new release/tag? Although py may be in maintenance mode it is still a dependency in pytest which is a fairly widely used package. Seeing as the fix was merged on 20 Sept I was just wondering if another release is ever going to be made? Are there plans to remove the py>=1.8.2 dependency in pytest?

[bluetech]

The reason I don't treat it with any priority is that I'm not sure there is any code left in existence which still uses py.path.svn. But I guess you never know. pytest, tox and other well-known py dependents don't use it.

I was just wondering if another release is ever going to be made?

I've never made a py release myself, but since the deploy is automated I may be able to do it, if the token is still valid. Maybe @nicoddemus would know?

Are there plans to remove the py>=1.8.2 dependency in pytest?

We are migrating away from it but it's gonna take a while.

[nicoddemus]

I've never made a py release myself, but since the deploy is automated I may be able to do it, if the token is still valid. Maybe @nicoddemus would know?

It should, although it is not using a token but my actual password. In my experience though Travis is taking forever due to their recent policy changes, so we might need to first port to GitHub actions before doing a new release.

[earthgecko]

I am sure snyk will automate this all at some point in the future. But why do they need to when they have automated people to do it for them? :)

[nicoddemus]

@bluetech I've opened #266 to use GitHub actions to at least make the next release easier/possible.

[bluetech]

Using the new CI that @nicoddemus set up everything went well - 1.10.0 is released now and includes this fix.

[earthgecko]

@bluetech and @nicoddemus thanks for the efforts!