Fixes ReDOS vulnerabilities.

Jamie Davis (@davisjam) from Virginia Tech reported that clean-css
suffers from ReDOS vulnerability [0] when fed with crafted input.

Since not so many people use clean-css allowing untrusted input such
cases may be rare, but this commit reworks vulnerable code to prevent
such attacks.

It also limits certain whitespace blocks to sane length of 31 characters
in validation regexes to prevent similar issues.

[0] https://snyk.io/blog/redos-and-catastrophic-backtracking

diff --git a/History.md b/History.md
index 98596bb..52ac33a 100644 (file)
--- a/History.md
+++ b/History.md
@@ -5,6 +5,7 @@
 * Fixed issue [#861](https://github.com/jakubpawlowicz/clean-css/issues/861) - new `transition` property optimizer.
 * Fixed issue [#895](https://github.com/jakubpawlowicz/clean-css/issues/895) - ignoring specific styles.
 * Fixed issue [#947](https://github.com/jakubpawlowicz/clean-css/issues/947) - selector based filtering.
+* Fixes ReDOS vulnerabilities in validator code.
 
 [4.1.10 / 2018-03-05](https://github.com/jakubpawlowicz/clean-css/compare/v4.1.9...v4.1.10)
 ==================

diff --git a/README.md b/README.md
index 83b91fa..43e48c3 100644 (file)
--- a/README.md
+++ b/README.md
@@ -743,6 +743,7 @@ Sorted alphabetically by GitHub handle:
 * [@alexlamsl](https://github.com/alexlamsl) (Alex Lam S.L.) for testing early clean-css 4 versions, reporting bugs, and suggesting numerous improvements.
 * [@altschuler](https://github.com/altschuler) (Simon Altschuler) for fixing `@import` processing inside comments;
 * [@ben-eb](https://github.com/ben-eb) (Ben Briggs) for sharing ideas about CSS optimizations;
+* [@davisjam](https://github.com/davisjam) (Jamie Davis) for disclosing ReDOS vulnerabilities;
 * [@facelessuser](https://github.com/facelessuser) (Isaac) for pointing out a flaw in clean-css' stateless mode;
 * [@grandrath](https://github.com/grandrath) (Martin Grandrath) for improving `minify` method source traversal in ES6;
 * [@jmalonzo](https://github.com/jmalonzo) (Jan Michael Alonzo) for a patch removing node.js' old `sys` package;

diff --git a/lib/optimizer/level-2/can-override.js b/lib/optimizer/level-2/can-override.js
index ceac482..3dae08f 100644 (file)
--- a/lib/optimizer/level-2/can-override.js
+++ b/lib/optimizer/level-2/can-override.js
@@ -199,6 +199,24 @@ function unitOrKeywordWithGlobal(propertyName) {
   };
 }
 
+function unitOrNumber(validator, value1, value2) {
+  if (!understandable(validator, value1, value2, 0, true) && !(validator.isUnit(value2) || validator.isNumber(value2))) {
+    return false;
+  } else if (validator.isVariable(value1) && validator.isVariable(value2)) {
+    return true;
+  } else if ((validator.isUnit(value1) || validator.isNumber(value1)) && !(validator.isUnit(value2) || validator.isNumber(value2))) {
+    return false;
+  } else if (validator.isUnit(value2) || validator.isNumber(value2)) {
+    return true;
+  } else if (validator.isUnit(value1) || validator.isNumber(value1)) {
+    return false;
+  } else if (validator.isFunction(value1) && !validator.isPrefixed(value1) && validator.isFunction(value2) && !validator.isPrefixed(value2)) {
+    return true;
+  }
+
+  return sameFunctionOrValue(validator, value1, value2);
+}
+
 function zIndex(validator, value1, value2) {
   if (!understandable(validator, value1, value2, 0, true) && !validator.isZIndex(value2)) {
     return false;
@@ -217,7 +235,8 @@ module.exports = {
     propertyName: propertyName,
     time: time,
     timingFunction: timingFunction,
-    unit: unit
+    unit: unit,
+    unitOrNumber: unitOrNumber
   },
   property: {
     animationDirection: keywordWithGlobal('animation-direction'),

diff --git a/lib/optimizer/level-2/compactable.js b/lib/optimizer/level-2/compactable.js
index 57a5863..73f42a1 100644 (file)
--- a/lib/optimizer/level-2/compactable.js
+++ b/lib/optimizer/level-2/compactable.js
@@ -681,7 +681,7 @@ var compactable = {
     defaultValue: 'auto'
   },
   'line-height': {
-    canOverride: canOverride.generic.unit,
+    canOverride: canOverride.generic.unitOrNumber,
     defaultValue: 'normal',
     shortestValue: '0'
   },

diff --git a/lib/optimizer/level-2/remove-unused-at-rules.js b/lib/optimizer/level-2/remove-unused-at-rules.js
index 23d4d7c..798d393 100644 (file)
--- a/lib/optimizer/level-2/remove-unused-at-rules.js
+++ b/lib/optimizer/level-2/remove-unused-at-rules.js
@@ -8,7 +8,7 @@ var Token = require('../../tokenizer/token');
 var animationNameRegex = /^(\-moz\-|\-o\-|\-webkit\-)?animation-name$/;
 var animationRegex = /^(\-moz\-|\-o\-|\-webkit\-)?animation$/;
 var keyframeRegex = /^@(\-moz\-|\-o\-|\-webkit\-)?keyframes /;
-var importantRegex = /\s*!important$/;
+var importantRegex = /\s{0,31}!important$/;
 var optionalMatchingQuotesRegex = /^(['"]?)(.*)\1$/;
 
 function normalize(value) {

diff --git a/lib/optimizer/validator.js b/lib/optimizer/validator.js
index b393707..48c8afc 100644 (file)
--- a/lib/optimizer/validator.js
+++ b/lib/optimizer/validator.js
@@ -4,19 +4,24 @@ var variableRegexStr = 'var\\(\\-\\-[^\\)]+\\)';
 var functionAnyRegexStr = '(' + variableRegexStr + '|' + functionNoVendorRegexStr + '|' + functionVendorRegexStr + ')';
 
 var calcRegex = new RegExp('^(\\-moz\\-|\\-webkit\\-)?calc\\([^\\)]+\\)$', 'i');
+var decimalRegex = /[0-9]/;
 var functionAnyRegex = new RegExp('^' + functionAnyRegexStr + '$', 'i');
-var hslColorRegex = /^hsl\(\s*[\-\.\d]+\s*,\s*[\.\d]+%\s*,\s*[\.\d]+%\s*\)|hsla\(\s*[\-\.\d]+\s*,\s*[\.\d]+%\s*,\s*[\.\d]+%\s*,\s*[\.\d]+\s*\)$/;
+var hslColorRegex = /^hsl\(\s{0,31}[\-\.]?\d+\s{0,31},\s{0,31}\.?\d+%\s{0,31},\s{0,31}\.?\d+%\s{0,31}\)|hsla\(\s{0,31}[\-\.]?\d+\s{0,31},\s{0,31}\.?\d+%\s{0,31},\s{0,31}\.?\d+%\s{0,31},\s{0,31}\.?\d+\s{0,31}\)$/;
 var identifierRegex = /^(\-[a-z0-9_][a-z0-9\-_]*|[a-z][a-z0-9\-_]*)$/i;
 var longHexColorRegex = /^#[0-9a-f]{6}$/i;
 var namedEntityRegex = /^[a-z]+$/i;
 var prefixRegex = /^-([a-z0-9]|-)*$/i;
-var rgbColorRegex = /^rgb\(\s*[\d]{1,3}\s*,\s*[\d]{1,3}\s*,\s*[\d]{1,3}\s*\)|rgba\(\s*[\d]{1,3}\s*,\s*[\d]{1,3}\s*,\s*[\d]{1,3}\s*,\s*[\.\d]+\s*\)$/;
+var rgbColorRegex = /^rgb\(\s{0,31}[\d]{1,3}\s{0,31},\s{0,31}[\d]{1,3}\s{0,31},\s{0,31}[\d]{1,3}\s{0,31}\)|rgba\(\s{0,31}[\d]{1,3}\s{0,31},\s{0,31}[\d]{1,3}\s{0,31},\s{0,31}[\d]{1,3}\s{0,31},\s{0,31}[\.\d]+\s{0,31}\)$/;
 var shortHexColorRegex = /^#[0-9a-f]{3}$/i;
-var timeRegex = new RegExp('^(\\-?\\+?\\.?\\d+\\.?\\d*(s|ms))$');
 var timingFunctionRegex = /^(cubic\-bezier|steps)\([^\)]+\)$/;
+var validTimeUnits = ['ms', 's'];
 var urlRegex = /^url\([\s\S]+\)$/i;
 var variableRegex = new RegExp('^' + variableRegexStr + '$', 'i');
 
+var DECIMAL_DOT = '.';
+var MINUS_SIGN = '-';
+var PLUS_SIGN = '+';
+
 var Keywords = {
   '^': [
     'inherit',
@@ -386,7 +391,7 @@ function isNamedEntity(value) {
 }
 
 function isNumber(value) {
-  return value.length > 0 && ('' + parseFloat(value)) === value;
+  return scanForNumber(value) == value.length;
 }
 
 function isRgbColor(value) {
@@ -407,7 +412,10 @@ function isVariable(value) {
 }
 
 function isTime(value) {
-  return timeRegex.test(value);
+  var numberUpTo = scanForNumber(value);
+
+  return numberUpTo == value.length && parseInt(value) === 0 ||
+    numberUpTo > -1 && validTimeUnits.indexOf(value.slice(numberUpTo + 1)) > -1;
 }
 
 function isTimingFunction() {
@@ -418,8 +426,13 @@ function isTimingFunction() {
   };
 }
 
-function isUnit(compatibleCssUnitRegex, value) {
-  return compatibleCssUnitRegex.test(value);
+function isUnit(validUnits, value) {
+  var numberUpTo = scanForNumber(value);
+
+  return numberUpTo == value.length && parseInt(value) === 0 ||
+    numberUpTo > -1 && validUnits.indexOf(value.slice(numberUpTo + 1)) > -1 ||
+    value == 'auto' ||
+    value == 'inherit';
 }
 
 function isUrl(value) {
@@ -432,13 +445,38 @@ function isZIndex(value) {
     isKeyword('^')(value);
 }
 
+function scanForNumber(value) {
+  var hasDot = false;
+  var hasSign = false;
+  var character;
+  var i, l;
+
+  for (i = 0, l = value.length; i < l; i++) {
+    character = value[i];
+
+    if (i === 0 && (character == PLUS_SIGN || character == MINUS_SIGN)) {
+      hasSign = true;
+    } else if (i > 0 && hasSign && (character == PLUS_SIGN || character == MINUS_SIGN)) {
+      return i - 1;
+    } else if (character == DECIMAL_DOT && !hasDot) {
+      hasDot = true;
+    } else if (character == DECIMAL_DOT && hasDot) {
+      return i - 1;
+    } else if (decimalRegex.test(character)) {
+      continue;
+    } else {
+      return i - 1;
+    }
+  }
+
+  return i;
+}
+
 function validator(compatibility) {
   var validUnits = Units.slice(0).filter(function (value) {
     return !(value in compatibility.units) || compatibility.units[value] === true;
   });
 
-  var compatibleCssUnitRegex = new RegExp('^(\\-?\\.?\\d+\\.?\\d*(' + validUnits.join('|') + '|)|auto|inherit)$', 'i');
-
   return {
     colorOpacity: compatibility.colors.opacity,
     isAnimationDirectionKeyword: isKeyword('animation-direction'),
@@ -471,12 +509,13 @@ function validator(compatibility) {
     isLineHeightKeyword: isKeyword('line-height'),
     isListStylePositionKeyword: isKeyword('list-style-position'),
     isListStyleTypeKeyword: isKeyword('list-style-type'),
+    isNumber: isNumber,
     isPrefixed: isPrefixed,
     isPositiveNumber: isPositiveNumber,
     isRgbColor: isRgbColor,
     isStyleKeyword: isKeyword('*-style'),
     isTime: isTime,
-    isUnit: isUnit.bind(null, compatibleCssUnitRegex),
+    isUnit: isUnit.bind(null, validUnits),
     isUrl: isUrl,
     isVariable: isVariable,
     isWidth: isKeyword('width'),

diff --git a/lib/tokenizer/tokenize.js b/lib/tokenizer/tokenize.js
index 2d34e4d..8d8c63c 100644 (file)
--- a/lib/tokenizer/tokenize.js
+++ b/lib/tokenizer/tokenize.js
@@ -59,7 +59,7 @@ var EXTRA_PAGE_BOXES = [
   '@right'
 ];
 
-var REPEAT_PATTERN = /^\[\s*\d+\s*\]$/;
+var REPEAT_PATTERN = /^\[\s{0,31}\d+\s{0,31}\]$/;
 var RULE_WORD_SEPARATOR_PATTERN = /[\s\(]/;
 var TAIL_BROKEN_VALUE_PATTERN = /[\s|\}]*$/;
 

diff --git a/test/module-test.js b/test/module-test.js
index 8aede41..bca1159 100644 (file)
--- a/test/module-test.js
+++ b/test/module-test.js
@@ -869,5 +869,27 @@ vows.describe('module tests').addBatch({
     'should raise no errors': function (error, minified) {
       assert.isEmpty(minified.errors);
     }
+  },
+  'vulnerabilities': {
+    'ReDOS in time units': {
+      'topic': function () {
+        var prefix = '-+.0';
+        var pump = [];
+        var suffix = '-0';
+        var input;
+        var i;
+
+        for (i = 0; i < 10000; i++) {
+          pump.push('0000000000');
+        }
+
+        input = '.block{animation:1s test;animation-duration:' + prefix + pump.join('') + suffix + 's}';
+
+        return new CleanCSS({ level: { 1: { replaceZeroUnits: false }, 2: true } }).minify(input);
+      },
+      'finishes in less than a second': function (error, minified) {
+        assert.isTrue(minified.stats.timeSpent < 1000);
+      }
+    }
   }
 }).export(module);