From c4847ebe7d14540bb28a8b932a9ce1b9ecbfee1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Lipi=C5=84ski?= Date: Tue, 26 Jan 2021 23:17:05 +0100 Subject: [PATCH] Improve performance of `toNumber`, `trim` and `trimEnd` on large input strings This prevents potential ReDoS attacks using `_.toNumber` and `_.trim*` as potential attack vectors. Closes #5065. --- lodash.js | 43 ++++++++++++++++++++++++++++++++++++------- test/test.js | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 7 deletions(-) diff --git a/lodash.js b/lodash.js index 278c702043..379ce88cf0 100644 --- a/lodash.js +++ b/lodash.js @@ -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?/, @@ -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. * @@ -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. * @@ -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) @@ -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; @@ -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; diff --git a/test/test.js b/test/test.js index d699ee514c..fcb3a168ed 100644 --- a/test/test.js +++ b/test/test.js @@ -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'); + }); }); /*--------------------------------------------------------------------------*/ @@ -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);