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
merged 2 commits into from Aug 25, 2022

Conversation

yetingli
Copy link
Contributor

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)*))$/

Benefits

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

Applicable issues

See the link https://www.huntr.dev/bounties/1d8a3d95-d199-4129-a6ad-8eafe5e77b9e/

@psmoros
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?

@github-actions
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 this has been inactive for a while... label May 18, 2022
@github-actions github-actions bot closed this Jun 1, 2022
@ewrayjohnson
Copy link

I am voting for this to be merged ASAP.

@yetingli
Copy link
Contributor Author

yetingli commented Jul 13, 2022

Reported in huntr https://www.huntr.dev/bounties/1d8a3d95-d199-4129-a6ad-8eafe5e77b9e/

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
@linux-foundation-easycla
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 area: security involving vulnerabilities and removed stale this has been inactive for a while... labels Aug 21, 2022
@coveralls
Copy link

Coverage Status

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

@outsideris
Copy link
Member

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.

@outsideris
Copy link
Member

#4909

Copy link
Member

@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.

LGTM

@yetingli
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 (https://www.huntr.dev/bounties/1d8a3d95-d199-4129-a6ad-8eafe5e77b9e/) once it's merged, so that I will get rewarded for their efforts!
Thanks! cc @psmoros

@outsideris
Copy link
Member

@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
@outsideris outsideris added this to the next milestone Aug 28, 2022
@sauravlikhar
Copy link

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

@heidemn-faro
Copy link

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

@Banhawy
Copy link

Banhawy commented Oct 18, 2022

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

@rahulankit30
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
Labels
area: security involving vulnerabilities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants