Skip to content
This repository has been archived by the owner on Dec 19, 2023. It is now read-only.

Fix redos #2

Closed
wants to merge 2 commits into from
Closed

Fix redos #2

wants to merge 2 commits into from

Conversation

Richienb
Copy link

Signed-off-by: Richie Bendall [email protected]

📊 Metadata *

Bounty URL:

https://www.huntr.dev/bounties/1-npm-node-dns-sync

⚙️ Description *

Parse the hostname properly instead of using regexes.

💻 Technical Description *

Swap out the regex solution with an npm package that parses it regex-free.

🐛 Proof of Concept (PoC) *

See skoranga#5

🔥 Proof of Fix (PoF) *

A regex is no longer used.

👍 User Acceptance Testing (UAT)

Unit tests pass.

Copy link

@mufeedvh mufeedvh left a comment

Choose a reason for hiding this comment

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

Hello @Richienb,

A good approach to fix the issue but under the hood, the dependency/package you used to validate the hostname is also vulnerable to ReDoS.

The package is-valid-hostname uses two Regex patterns:

const validHostnameChars = /^([a-zA-Z0-9-.]+){1,253}$/g

and

const validLabelChars = /^([a-zA-Z0-9-]+)$/g

The first Regex pattern is vulnerable to ReDoS which makes the fix insufficient.

Screenshot from 2020-08-17 19-54-27

To check whether a Regex is vulnerable or not, you can use:

huntr sheriff

@JamieSlome
Copy link

@Richienb - thank you for your submission!

We would love to see more of the contributions in the future! 🍰

If you have any questions, get in touch!

@JamieSlome JamieSlome closed this Aug 20, 2020
@huntr-helper
Copy link

Sorry Richienb, we enjoyed reviewing your fix but it has not been selected this time. If this bounty has not been closed, please feel free to try again with a new pull request!

We appreciate your effort and look forward to reviewing more of your fixes in the future! 🔨😎

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

Successfully merging this pull request may close these issues.

4 participants