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: ReDoS referrer #1611

Merged
merged 2 commits into from Jul 31, 2022
Merged

fix: ReDoS referrer #1611

merged 2 commits into from Jul 31, 2022

Conversation

vovikhangcdv
Copy link
Contributor

Purpose

Fix Inefficient Regular Expression Complexity (ReDoS) in the referrer regex.

Inefficient regular expression complexity regex when trying to match Potentially Trustworthy could lead to a denial of service attack. With a formed payload 'http://' + 'a.a.'.repeat(i) + 'a', 76 characters payload could take 42642 ms time execution.

Changes

  • Change Potentially Trustworthy regex matcher.

Additional information


  • I updated readme
  • I added unit test(s)

@vovikhangcdv vovikhangcdv changed the title fix ReDoS referrer fix: ReDoS referrer Jul 26, 2022
jimmywarting
jimmywarting previously approved these changes Jul 26, 2022
Copy link
Collaborator

@jimmywarting jimmywarting left a comment

Choose a reason for hiding this comment

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

LGTM

src/utils/referrer.js Outdated Show resolved Hide resolved
@jimmywarting jimmywarting self-requested a review July 27, 2022 21:16
@jimmywarting jimmywarting dismissed their stale review July 27, 2022 21:16

dose not match just localhost

Eliminate regex and use string matcher

Co-authored-by: Linus Unnebäck <linus@folkdatorn.se>
@jimmywarting jimmywarting requested a review from LinusU July 28, 2022 19:26
@jimmywarting jimmywarting merged commit 2880238 into node-fetch:main Jul 31, 2022
@github-actions
Copy link

🎉 This PR is included in version 3.2.10 🎉

The release is available on:

Your semantic-release bot 📦🚀

@FlameFractal
Copy link

Can we add this to v2 (CommonJS) as well?

(ref)

@LinusU
Copy link
Member

LinusU commented Aug 1, 2022

Can we add this to v2 (CommonJS) as well?

We would be happy to accept a PR for this

@FlameFractal
Copy link

FlameFractal commented Aug 1, 2022

I went through 2.x code and I could not find this issue there. 2.x doesn't even implement referrer, and there's no check for localhost anywhere else either. Moreover, there's only 10 regex tests and I think none contain this vulnerability (maybe @vovikhangcdv could verify this).

We got a Snyk-bot notification about this issue, but I think node-fetch:2.x is not affected by this CVE.

@vovikhangcdv
Copy link
Contributor Author

Hi there, I confirm that version 2.x is not affected by this vulnerability.

@vovikhangcdv
Copy link
Contributor Author

I suggest we could add a Security Advisory and specify the affected version for this issue.

@JamieSlome
Copy link
Contributor

JamieSlome commented Aug 2, 2022

Just for clarity, we have updated the CVE here:

CVEProject/cvelist#6757

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Found potential security but got no response
6 participants