Skip to content

Commit

Permalink
security: Fix REDOS vulnerability
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
davisjam committed Mar 19, 2018
1 parent 1498b0d commit 1495509
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 3 deletions.
26 changes: 23 additions & 3 deletions index.js
Expand Up @@ -6,10 +6,15 @@
module.exports = isUrl;

/**
* Matcher.
* RegExps.
* A URL must match #1 and then at least one of #2/#3.
* Use two levels of REs to avoid REDOS.
*/

var matcher = /^(?:\w+:)?\/\/([^\s\.]+\.\S{2}|localhost[\:?\d]*)\S*$/;
var protocolAndDomainRE = /^(?:\w+:)?\/\/(\S+)$/;

var localhostDomainRE = /^localhost[\:?\d]*(?:[^\:?\d]\S*)?$/
var nonLocalhostDomainRE = /^[^\s\.]+\.\S{2,}$/;

/**
* Loosely validate a URL `string`.
Expand All @@ -19,5 +24,20 @@ var matcher = /^(?:\w+:)?\/\/([^\s\.]+\.\S{2}|localhost[\:?\d]*)\S*$/;
*/

function isUrl(string){
return matcher.test(string);
var match = string.match(protocolAndDomainRE);
if (!match) {
return false;
}

var everythingAfterProtocol = match[1];
if (!everythingAfterProtocol) {
return false;
}

if (localhostDomainRE.test(everythingAfterProtocol) ||
nonLocalhostDomainRE.test(everythingAfterProtocol)) {
return true;
}

return false;
}
11 changes: 11 additions & 0 deletions test/index.js
Expand Up @@ -119,4 +119,15 @@ describe('is-url', function () {
assert(!url('google.com'));
});
});

describe('redos', function () {
it('redos exploit', function () {
// Invalid. This should be discovered in under 1 second.
var attackString = 'a://localhost' + '9'.repeat(100000) + '\t';
var before = process.hrtime();
assert(!url(attackString), 'attackString was valid');
var elapsed = process.hrtime(before);
assert(elapsed[0] < 1, 'attackString took ' + elapsed[0] + ' > 1 seconds');
});
});
});

1 comment on commit 1495509

@cadente-nd
Copy link

Choose a reason for hiding this comment

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

This update cause an error in isUrl function if string is undefined.

Please sign in to comment.