Skip to content

Commit

Permalink
Improve performance of toNumber, trim and trimEnd on large inpu…
Browse files Browse the repository at this point in the history
…t strings

This prevents potential ReDoS attacks using `_.toNumber` and `_.trim*`
as potential attack vectors.

Closes #5065.
  • Loading branch information
falsyvalues authored and bnjmnt4n committed Feb 20, 2021
1 parent 3469357 commit c4847eb
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 7 deletions.
43 changes: 36 additions & 7 deletions lodash.js
Expand Up @@ -153,10 +153,11 @@
var reRegExpChar = /[\\^$.*+?()[\]{}|]/g,
reHasRegExpChar = RegExp(reRegExpChar.source);

/** Used to match leading and trailing whitespace. */
var reTrim = /^\s+|\s+$/g,
reTrimStart = /^\s+/,
reTrimEnd = /\s+$/;
/** Used to match leading whitespace. */
var reTrimStart = /^\s+/;

/** Used to match a single whitespace character. */
var reWhitespace = /\s/;

/** Used to match wrap detail comments. */
var reWrapComment = /\{(?:\n\/\* \[wrapped with .+\] \*\/)?\n?/,
Expand Down Expand Up @@ -1006,6 +1007,19 @@
});
}

/**
* The base implementation of `_.trim`.
*
* @private
* @param {string} string The string to trim.
* @returns {string} Returns the trimmed string.
*/
function baseTrim(string) {
return string
? string.slice(0, trimmedEndIndex(string) + 1).replace(reTrimStart, '')
: string;
}

/**
* The base implementation of `_.unary` without support for storing metadata.
*
Expand Down Expand Up @@ -1339,6 +1353,21 @@
: asciiToArray(string);
}

/**
* Used by `_.trim` and `_.trimEnd` to get the index of the last non-whitespace
* character of `string`.
*
* @private
* @param {string} string The string to inspect.
* @returns {number} Returns the index of the last non-whitespace character.
*/
function trimmedEndIndex(string) {
var index = string.length;

while (index-- && reWhitespace.test(string.charAt(index))) {}
return index;
}

/**
* Used by `_.unescape` to convert HTML entities to characters.
*
Expand Down Expand Up @@ -12507,7 +12536,7 @@
if (typeof value != 'string') {
return value === 0 ? value : +value;
}
value = value.replace(reTrim, '');
value = baseTrim(value);
var isBinary = reIsBinary.test(value);
return (isBinary || reIsOctal.test(value))
? freeParseInt(value.slice(2), isBinary ? 2 : 8)
Expand Down Expand Up @@ -14998,7 +15027,7 @@
function trim(string, chars, guard) {
string = toString(string);
if (string && (guard || chars === undefined)) {
return string.replace(reTrim, '');
return baseTrim(string);
}
if (!string || !(chars = baseToString(chars))) {
return string;
Expand Down Expand Up @@ -15033,7 +15062,7 @@
function trimEnd(string, chars, guard) {
string = toString(string);
if (string && (guard || chars === undefined)) {
return string.replace(reTrimEnd, '');
return string.slice(0, trimmedEndIndex(string) + 1);
}
if (!string || !(chars = baseToString(chars))) {
return string;
Expand Down
32 changes: 32 additions & 0 deletions test/test.js
Expand Up @@ -23783,6 +23783,22 @@

assert.deepEqual(actual, expected);
});

QUnit.test('`_.`' + methodName + '` should prevent ReDoS', function(assert) {
assert.expect(2);

var largeStrLen = 50000,
largeStr = '1' + lodashStable.repeat(' ', largeStrLen) + '1',
maxMs = 1000,
startTime = lodashStable.now();

assert.deepEqual(_[methodName](largeStr), methodName == 'toNumber' ? NaN : 0);

var endTime = lodashStable.now(),
timeSpent = endTime - startTime;

assert.ok(timeSpent < maxMs, 'operation took ' + timeSpent + 'ms');
});
});

/*--------------------------------------------------------------------------*/
Expand Down Expand Up @@ -24368,6 +24384,22 @@
assert.strictEqual(func(string, ''), string);
});

QUnit.test('`_.`' + methodName + '` should prevent ReDoS', function(assert) {
assert.expect(2);

var largeStrLen = 50000,
largeStr = 'A' + lodashStable.repeat(' ', largeStrLen) + 'A',
maxMs = 1000,
startTime = lodashStable.now();

assert.strictEqual(_[methodName](largeStr), largeStr);

var endTime = lodashStable.now(),
timeSpent = endTime - startTime;

assert.ok(timeSpent < maxMs, 'operation took ' + timeSpent + 'ms');
});

QUnit.test('`_.' + methodName + '` should work as an iteratee for methods like `_.map`', function(assert) {
assert.expect(1);

Expand Down

3 comments on commit c4847eb

@celotts
Copy link

Choose a reason for hiding this comment

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

Se cambia

@ajedwards3738
Copy link

Choose a reason for hiding this comment

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

Good job

@ajedwards3738
Copy link

Choose a reason for hiding this comment

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

Good luck

Please sign in to comment.