Skip to content

fix: ReDoS referrer#1611

Merged
jimmywarting merged 2 commits intonode-fetch:mainfrom
doublevkay:main
Jul 31, 2022
Merged

fix: ReDoS referrer#1611
jimmywarting merged 2 commits intonode-fetch:mainfrom
doublevkay:main

Conversation

@doublevkay
Copy link
Contributor

@doublevkay doublevkay commented Jul 26, 2022

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)

@doublevkay doublevkay changed the title fix ReDoS referrer fix: ReDoS referrer Jul 26, 2022
jimmywarting
jimmywarting previously approved these changes Jul 26, 2022
View reviewed changes
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

LinusU
LinusU reviewed Jul 27, 2022
View reviewed changes
@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

@doublevkay @LinusU
Update src/utils/referrer.js
1bbabcb 
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

github-actions bot commented Jul 31, 2022

🎉 This PR is included in version 3.2.10 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot mentioned this pull request Jul 31, 2022
Closed
@FlameFractal
Copy link

FlameFractal commented Aug 1, 2022

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 •
edited
Loading

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.

@doublevkay
Copy link
Contributor Author

doublevkay commented Aug 1, 2022

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

@LinusU LinusU mentioned this pull request Aug 1, 2022
Closed
@doublevkay
Copy link
Contributor Author

doublevkay commented Aug 1, 2022

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

This was referenced Aug 2, 2022
Closed
Closed
Closed
@JamieSlome
Copy link
Contributor

JamieSlome commented Aug 2, 2022 •
edited
Loading

Just for clarity, we have updated the CVE here:

CVEProject/cvelist#6757

@nicodemoraffaele nicodemoraffaele mentioned this pull request Aug 3, 2022
Closed
@MichaelPachec0 MichaelPachec0 mentioned this pull request Aug 5, 2022
Closed
@jenschude jenschude mentioned this pull request Nov 8, 2022
Closed
@data-sync-user data-sync-user mentioned this pull request Oct 12, 2023
Closed
@ntnguyencse ntnguyencse mentioned this pull request May 19, 2024
Open
@mshantanu mshantanu mentioned this pull request May 19, 2024
Open
This was referenced Sep 12, 2024
Open
Open
@NOUIY NOUIY mentioned this pull request Sep 13, 2024
Open
This was referenced Sep 14, 2024
Open
Open
Open
Open
Open
Open
Open
This was referenced Sep 21, 2024
Open
Open
Open
@snyk-io snyk-io bot mentioned this pull request Sep 24, 2024
Open
This was referenced Sep 24, 2024
Open
Open
Open
Open
Open
Open
Open
This was referenced Oct 1, 2024
Open
Open
Open
Open
Open
Open
@ReneMaetzschker ReneMaetzschker mentioned this pull request Oct 26, 2024
Open
@snyk-io snyk-io bot mentioned this pull request Nov 9, 2024
Closed
@mshantanu mshantanu mentioned this pull request Nov 12, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jimmywarting jimmywarting jimmywarting approved these changes

@NotMoni NotMoni NotMoni approved these changes

@LinusU LinusU Awaiting requested review from LinusU

Assignees

No one assigned

Labels

released

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Found potential security but got no response

6 participants

@doublevkay @FlameFractal @LinusU @JamieSlome @jimmywarting @NotMoni