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 the regular expression for function clean in utils.js #4770

merged 2 commits into from Aug 25, 2022


Copy link

Description of the Change

Fix the regular expression for function clean in utils.js to avoid potential Regular Expression Denial of Service (ReDoS) vulnerabilities.

Alternate Designs

The ReDoS vulnerabilities of the regex /^function(?:\s*|\s+[^(]*)\([^)]*\)\s*\{((?:.|\n)*?)\s*\}$|^\([^)]*\)\s*=>\s*(?:\{((?:.|\n)*?)\s*\}|((?:.|\n)*))$/ are mainly due to the overlapped sub-patterns \s+[^(]* and ((?:.|\n)*?)\s*.
We can ignore \s first, because in the end, we can also deal with \s using the trim function (see the code below)

return str.trim();

So I am willing to suggest that you replace the ReDoS-vulnerable regex /^function(?:\s*|\s+[^(]*)\([^)]*\)\s*\{((?:.|\n)*?)\s*\}$|^\([^)]*\)\s*=>\s*(?:\{((?:.|\n)*?)\s*\}|((?:.|\n)*))$/ with the safe regex /^function(?:\s*|\s[^(]*)\([^)]*\)\s*\{((?:.|\n)*?)\}$|^\([^)]*\)\s*=>\s*(?:\{((?:.|\n)*?)\}|((?:.|\n)*))$/


It achieves functional equivalence and is not subject to ReDoS attacks.

Applicable issues

See the link

Copy link

psmoros commented Jan 17, 2022

hey @boneskull someone from the team asked @yetingli to open a PR with a security fix; do you guys plan on merging it sometime soon?

Copy link

This PR hasn't had any recent activity, and I'm labeling it stale. Remove the label or comment or this PR will be closed in 14 days. Thanks for contributing to Mocha!

@github-actions github-actions bot added the stale will be closed after two (2) weeks of inactivity label May 18, 2022
@github-actions github-actions bot closed this Jun 1, 2022
Copy link

I am voting for this to be merged ASAP.

Copy link
Contributor Author

yetingli commented Jul 13, 2022

Reported in huntr

Please mark as valid and confirm fix in huntr once it's merged, so that I will get rewarded for their efforts!
Thanks! cc @psmoros

@outsideris outsideris reopened this Aug 21, 2022
Copy link

linux-foundation-easycla bot commented Aug 21, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@outsideris outsideris added security involving vulnerabilities and removed stale will be closed after two (2) weeks of inactivity labels Aug 21, 2022
Copy link

Coverage Status

Coverage decreased (-0.1%) to 94.326% when pulling 2a3005c on yetingli:master into 111467f on mochajs:master.

Copy link

outsideris commented Aug 21, 2022

@yetingli We missed it. Sorry for the late response.
Can you sign the CLA?
I confirmed this ReDos vulnerability and addressed it with this fix.

@mochajs/core I will merge this in a few days after CLA.
Because mocha is a test framework, ReDoS is not very likely in a production environment, but it needs to be fixed.

After merging this PR, I will add a test for ReDos.

Copy link


Copy link

@outsideris outsideris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


Copy link
Contributor Author

Hi @outsideris, thank you for your response! And I have signed the CLA.

Please mark as valid and confirm fix in huntr ( once it's merged, so that I will get rewarded for their efforts!
Thanks! cc @psmoros

Copy link

@yetingli Okay, It's my first time using Huntr. I will try it after merging this.

@outsideris outsideris merged commit 61b4b92 into mochajs:master Aug 25, 2022
18 of 19 checks passed
@outsideris outsideris added this to the next milestone Aug 28, 2022
Copy link

Which mocha version has this fix? 10.0.0 is still giving the same issue.

Copy link

@outsideris sorry to bother, but can a new version be published with this fix? Our security tools are complaining...

Copy link

Banhawy commented Oct 18, 2022

@sauravlikhar @heidemn-faro the fix is now published in version 10.1.0

Copy link

is it fixed??? 10.2.0 is still giving this error

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
security involving vulnerabilities
None yet

Successfully merging this pull request may close these issues.

None yet

9 participants