Skip to content

Commit

Permalink
Handle extremely long and terrible patterns more gracefully
Browse files Browse the repository at this point in the history
Reported by @nstarke, thanks
  • Loading branch information
isaacs committed Jun 17, 2016
1 parent 8ac560e commit 6944abf
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 3 deletions.
18 changes: 15 additions & 3 deletions minimatch.js
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ function braceExpand (pattern, options) {
? this.pattern : pattern

if (typeof pattern === 'undefined') {
throw new Error('undefined pattern')
throw new TypeError('undefined pattern')
}

if (options.nobrace ||
Expand All @@ -261,6 +261,10 @@ function braceExpand (pattern, options) {
Minimatch.prototype.parse = parse
var SUBPARSE = {}
function parse (pattern, isSub) {
if (pattern.length > 1024 * 64) {
throw new TypeError('pattern is too long')
}

var options = this.options

// shortcuts
Expand Down Expand Up @@ -518,7 +522,7 @@ function parse (pattern, isSub) {
for (pl = patternListStack.pop(); pl; pl = patternListStack.pop()) {
var tail = re.slice(pl.reStart + 3)
// maybe some even number of \, then maybe 1 \, followed by a |
tail = tail.replace(/((?:\\{2})*)(\\?)\|/g, function (_, $1, $2) {
tail = tail.replace(/((?:\\{2}){0,64})(\\?)\|/g, function (_, $1, $2) {
if (!$2) {
// the | isn't already escaped, so escape it.
$2 = '\\'
Expand Down Expand Up @@ -615,7 +619,15 @@ function parse (pattern, isSub) {
}

var flags = options.nocase ? 'i' : ''
var regExp = new RegExp('^' + re + '$', flags)
try {
var regExp = new RegExp('^' + re + '$', flags)
} catch (er) {
// If it was an invalid regular expression, then it can't match
// anything. This trick looks for a character after the end of
// the string, which is of course impossible, except in multi-line
// mode, but it's not a /m regex.
return new RegExp('$.')
}

regExp._glob = pattern
regExp._src = re
Expand Down
28 changes: 28 additions & 0 deletions test/redos.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
var t = require('tap')

var minimatch = require('../')

// utility function for generating long strings
var genstr = function (len, chr) {
var result = ''
for (var i = 0; i <= len; i++) {
result = result + chr
}

return result
}

var exploit = '!(' + genstr(1024 * 15, '\\') + 'A)'

// within the limits, and valid match
t.ok(minimatch('A', exploit))

// within the limits, but results in an invalid regexp
exploit = '[!(' + genstr(1024 * 15, '\\') + 'A'
t.notOk(minimatch('A', exploit))

t.throws(function () {
// too long, throws TypeError
exploit = '!(' + genstr(1024 * 64, '\\') + 'A)'
minimatch('A', exploit)
}, TypeError)

2 comments on commit 6944abf

@romanzoller
Copy link

Choose a reason for hiding this comment

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

Could you elaborate a bit on the issue that is addressed here? I'm assuming this is the fix to a RegExp DoS issue that npm warns about.

It says reported by @nstarke, but I could not find a GitHub issue, and I would like to understand how serious this is before going on a PR spree of packages that use an old minimatch dependency.

@isaacs
Copy link
Owner Author

@isaacs isaacs commented on 6944abf Nov 17, 2016

Choose a reason for hiding this comment

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

Yes, it is fixing the regexp dos issue.

There was not a github issue because this was reported privately, as it was a potential security hazard that was not made public until a fix was available.

Please sign in to comment.