security: Fix REDOS vulnerability

[davisjam]

Problem:
As disclosed by email, the regex in this module was vulnerable to
catastrophic backtracking.
This could be used as a REDOS vector for Node.js servers that use
this module.

Solution:
Use a two-step validation process.
Split the vulnerable regex into three safe ones.

[davisjam]

Credit to @josdejong for spotting this.

[josdejong]

👍

[zeke]

From Travis:

Could not find .travis.yml, using standard configuration.

Default config is for a ruby project. I wonder how Travis ever worked on this repo... 🤔

This project is using a special combination of make test and an abandoned project called component... But if you just run mocha, all the tests are passing on this PR, so this change looks good to me.

To be on the safe side, I would recommend doing a major version bump to avoid breaking any existing users. However that means users won't get this security fix unless they explicitly upgrade to the new major.

We could npm deprecate all the older versions after publishing, so people would see a warning when they installed any pre 2.0.0 version. What do folks think of this?

cc @ianstormtaylor

[davisjam]

I tested by running mocha by hand, yep. No regressions, and the REDOS test I added passes with my change and fails without it.

I don't think a semver-major bump is necessary. I believe my version accepts precisely the same language as the previous unsafe regex did, but I welcome a second pair of eyes on it.

This is what I did. I took the unsafe regex and broke it into two steps for clarity, then tested each of the two domain options separately. But I used what I think are semantically equivalent regexes. The localDomain is slightly modified so that it is safe.

On an unrelated note, given the popularity of this project, it might be nice to set up Travis and Mocha so they work properly. Are there issues open for that?

[davisjam]

This PR is publicizing the possibility of REDOS via this popular npm module. Should I take it down until the maintainers are ready to review it?

[jacktuck]

LGTM

[zeke]

I believe my version accepts precisely the same language as the previous unsafe regex did

Yeah not worried about your changes here being a breaking change so much as wanting to avoid publishing a new version that's missing something indirectly like a browser-friendly build artifact or something. I say this because I think this module is not actively maintained any more.

[zeke]

That said, I do have npm publish rights, so we could just say #yolo, publish it as a patch, and deal with any possible fallout after.

[davisjam]

publish it as a patch, and deal with any possible fallout after

Seems reasonable to me. If there aren't active maintainers who can tell us the details on publishing, the choices seem to be:

1. Deprecate package.
2. Patch with possible mistakes.

Since this is a popular package I think deprecating is not a good route. I vote #yolo.

On a related note, perhaps updating the README with a "looking for a maintainer" advertisement would be good.

[zeke]

$ np patch --yolo 
 ✔ Prerequisite check
 ✔ Git
 ✔ Bumping version using npm
 ✔ Publishing package
 ✔ Pushing tags

 is-url 1.2.3 published 🎉

[zeke]

A perfect opportunity to use np's --yolo flag. Thanks, @sindresorhus 👍