Skip to content

Commit 0440b4a

Browse files
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
1 parent c601ebd commit 0440b4a

File tree

8 files changed

+100
-14
lines changed

8 files changed

+100
-14
lines changed

History.md

+5
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,8 @@
1+
[4.1.11 / 2018-xx-xx](https://github.com/jakubpawlowicz/clean-css/compare/v4.1.10...4.1)
2+
==================
3+
4+
* Fixes ReDOS vulnerabilities in validator code.
5+
16
[4.1.10 / 2018-03-05](https://github.com/jakubpawlowicz/clean-css/compare/v4.1.9...v4.1.10)
27
==================
38

README.md

+1
Original file line numberDiff line numberDiff line change
@@ -702,6 +702,7 @@ Sorted alphabetically by GitHub handle:
702702
* [@alexlamsl](https://github.com/alexlamsl) (Alex Lam S.L.) for testing early clean-css 4 versions, reporting bugs, and suggesting numerous improvements.
703703
* [@altschuler](https://github.com/altschuler) (Simon Altschuler) for fixing `@import` processing inside comments;
704704
* [@ben-eb](https://github.com/ben-eb) (Ben Briggs) for sharing ideas about CSS optimizations;
705+
* [@davisjam](https://github.com/davisjam) (Jamie Davis) for disclosing ReDOS vulnerabilities;
705706
* [@facelessuser](https://github.com/facelessuser) (Isaac) for pointing out a flaw in clean-css' stateless mode;
706707
* [@grandrath](https://github.com/grandrath) (Martin Grandrath) for improving `minify` method source traversal in ES6;
707708
* [@jmalonzo](https://github.com/jmalonzo) (Jan Michael Alonzo) for a patch removing node.js' old `sys` package;

lib/optimizer/level-2/can-override.js

+20-1
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,24 @@ function unitOrKeywordWithGlobal(propertyName) {
191191
};
192192
}
193193

194+
function unitOrNumber(validator, value1, value2) {
195+
if (!understandable(validator, value1, value2, 0, true) && !(validator.isUnit(value2) || validator.isNumber(value2))) {
196+
return false;
197+
} else if (validator.isVariable(value1) && validator.isVariable(value2)) {
198+
return true;
199+
} else if ((validator.isUnit(value1) || validator.isNumber(value1)) && !(validator.isUnit(value2) || validator.isNumber(value2))) {
200+
return false;
201+
} else if (validator.isUnit(value2) || validator.isNumber(value2)) {
202+
return true;
203+
} else if (validator.isUnit(value1) || validator.isNumber(value1)) {
204+
return false;
205+
} else if (validator.isFunction(value1) && !validator.isPrefixed(value1) && validator.isFunction(value2) && !validator.isPrefixed(value2)) {
206+
return true;
207+
}
208+
209+
return sameFunctionOrValue(validator, value1, value2);
210+
}
211+
194212
function zIndex(validator, value1, value2) {
195213
if (!understandable(validator, value1, value2, 0, true) && !validator.isZIndex(value2)) {
196214
return false;
@@ -207,7 +225,8 @@ module.exports = {
207225
components: components,
208226
image: image,
209227
time: time,
210-
unit: unit
228+
unit: unit,
229+
unitOrNumber: unitOrNumber
211230
},
212231
property: {
213232
animationDirection: keywordWithGlobal('animation-direction'),

lib/optimizer/level-2/compactable.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -681,7 +681,7 @@ var compactable = {
681681
defaultValue: 'auto'
682682
},
683683
'line-height': {
684-
canOverride: canOverride.generic.unit,
684+
canOverride: canOverride.generic.unitOrNumber,
685685
defaultValue: 'normal',
686686
shortestValue: '0'
687687
},

lib/optimizer/level-2/remove-unused-at-rules.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ var Token = require('../../tokenizer/token');
88
var animationNameRegex = /^(\-moz\-|\-o\-|\-webkit\-)?animation-name$/;
99
var animationRegex = /^(\-moz\-|\-o\-|\-webkit\-)?animation$/;
1010
var keyframeRegex = /^@(\-moz\-|\-o\-|\-webkit\-)?keyframes /;
11-
var importantRegex = /\s*!important$/;
11+
var importantRegex = /\s{0,31}!important$/;
1212
var optionalMatchingQuotesRegex = /^(['"]?)(.*)\1$/;
1313

1414
function normalize(value) {

lib/optimizer/validator.js

+49-10
Original file line numberDiff line numberDiff line change
@@ -5,18 +5,23 @@ var functionAnyRegexStr = '(' + variableRegexStr + '|' + functionNoVendorRegexSt
55

66
var animationTimingFunctionRegex = /^(cubic\-bezier|steps)\([^\)]+\)$/;
77
var calcRegex = new RegExp('^(\\-moz\\-|\\-webkit\\-)?calc\\([^\\)]+\\)$', 'i');
8+
var decimalRegex = /[0-9]/;
89
var functionAnyRegex = new RegExp('^' + functionAnyRegexStr + '$', 'i');
9-
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*\)$/;
10+
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}\)$/;
1011
var identifierRegex = /^(\-[a-z0-9_][a-z0-9\-_]*|[a-z][a-z0-9\-_]*)$/i;
1112
var longHexColorRegex = /^#[0-9a-f]{6}$/i;
1213
var namedEntityRegex = /^[a-z]+$/i;
1314
var prefixRegex = /^-([a-z0-9]|-)*$/i;
14-
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*\)$/;
15+
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}\)$/;
1516
var shortHexColorRegex = /^#[0-9a-f]{3}$/i;
16-
var timeRegex = new RegExp('^(\\-?\\+?\\.?\\d+\\.?\\d*(s|ms))$');
17+
var validTimeUnits = ['ms', 's'];
1718
var urlRegex = /^url\([\s\S]+\)$/i;
1819
var variableRegex = new RegExp('^' + variableRegexStr + '$', 'i');
1920

21+
var DECIMAL_DOT = '.';
22+
var MINUS_SIGN = '-';
23+
var PLUS_SIGN = '+';
24+
2025
var Keywords = {
2126
'^': [
2227
'inherit',
@@ -394,7 +399,7 @@ function isNamedEntity(value) {
394399
}
395400

396401
function isNumber(value) {
397-
return value.length > 0 && ('' + parseFloat(value)) === value;
402+
return scanForNumber(value) == value.length;
398403
}
399404

400405
function isRgbColor(value) {
@@ -415,11 +420,19 @@ function isVariable(value) {
415420
}
416421

417422
function isTime(value) {
418-
return timeRegex.test(value);
423+
var numberUpTo = scanForNumber(value);
424+
425+
return numberUpTo == value.length && parseInt(value) === 0 ||
426+
numberUpTo > -1 && validTimeUnits.indexOf(value.slice(numberUpTo + 1)) > -1;
419427
}
420428

421-
function isUnit(compatibleCssUnitRegex, value) {
422-
return compatibleCssUnitRegex.test(value);
429+
function isUnit(validUnits, value) {
430+
var numberUpTo = scanForNumber(value);
431+
432+
return numberUpTo == value.length && parseInt(value) === 0 ||
433+
numberUpTo > -1 && validUnits.indexOf(value.slice(numberUpTo + 1)) > -1 ||
434+
value == 'auto' ||
435+
value == 'inherit';
423436
}
424437

425438
function isUrl(value) {
@@ -432,13 +445,38 @@ function isZIndex(value) {
432445
isKeyword('^')(value);
433446
}
434447

448+
function scanForNumber(value) {
449+
var hasDot = false;
450+
var hasSign = false;
451+
var character;
452+
var i, l;
453+
454+
for (i = 0, l = value.length; i < l; i++) {
455+
character = value[i];
456+
457+
if (i === 0 && (character == PLUS_SIGN || character == MINUS_SIGN)) {
458+
hasSign = true;
459+
} else if (i > 0 && hasSign && (character == PLUS_SIGN || character == MINUS_SIGN)) {
460+
return i - 1;
461+
} else if (character == DECIMAL_DOT && !hasDot) {
462+
hasDot = true;
463+
} else if (character == DECIMAL_DOT && hasDot) {
464+
return i - 1;
465+
} else if (decimalRegex.test(character)) {
466+
continue;
467+
} else {
468+
return i - 1;
469+
}
470+
}
471+
472+
return i;
473+
}
474+
435475
function validator(compatibility) {
436476
var validUnits = Units.slice(0).filter(function (value) {
437477
return !(value in compatibility.units) || compatibility.units[value] === true;
438478
});
439479

440-
var compatibleCssUnitRegex = new RegExp('^(\\-?\\.?\\d+\\.?\\d*(' + validUnits.join('|') + '|)|auto|inherit)$', 'i');
441-
442480
return {
443481
colorOpacity: compatibility.colors.opacity,
444482
isAnimationDirectionKeyword: isKeyword('animation-direction'),
@@ -471,12 +509,13 @@ function validator(compatibility) {
471509
isLineHeightKeyword: isKeyword('line-height'),
472510
isListStylePositionKeyword: isKeyword('list-style-position'),
473511
isListStyleTypeKeyword: isKeyword('list-style-type'),
512+
isNumber: isNumber,
474513
isPrefixed: isPrefixed,
475514
isPositiveNumber: isPositiveNumber,
476515
isRgbColor: isRgbColor,
477516
isStyleKeyword: isKeyword('*-style'),
478517
isTime: isTime,
479-
isUnit: isUnit.bind(null, compatibleCssUnitRegex),
518+
isUnit: isUnit.bind(null, validUnits),
480519
isUrl: isUrl,
481520
isVariable: isVariable,
482521
isWidth: isKeyword('width'),

lib/tokenizer/tokenize.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ var EXTRA_PAGE_BOXES = [
5656
'@right'
5757
];
5858

59-
var REPEAT_PATTERN = /^\[\s*\d+\s*\]$/;
59+
var REPEAT_PATTERN = /^\[\s{0,31}\d+\s{0,31}\]$/;
6060
var RULE_WORD_SEPARATOR_PATTERN = /[\s\(]/;
6161
var TAIL_BROKEN_VALUE_PATTERN = /[\s|\}]*$/;
6262

test/module-test.js

+22
Original file line numberDiff line numberDiff line change
@@ -840,5 +840,27 @@ vows.describe('module tests').addBatch({
840840
'should give right output': function (minified) {
841841
assert.equal(minified.styles, '.one{color:red}.three{background-image:url(test/fixtures/partials/extra/down.gif)}');
842842
}
843+
},
844+
'vulnerabilities': {
845+
'ReDOS in time units': {
846+
'topic': function () {
847+
var prefix = '-+.0';
848+
var pump = [];
849+
var suffix = '-0';
850+
var input;
851+
var i;
852+
853+
for (i = 0; i < 10000; i++) {
854+
pump.push('0000000000');
855+
}
856+
857+
input = '.block{animation:1s test;animation-duration:' + prefix + pump.join('') + suffix + 's}';
858+
859+
return new CleanCSS({ level: { 1: { replaceZeroUnits: false }, 2: true } }).minify(input);
860+
},
861+
'finishes in less than a second': function (error, minified) {
862+
assert.isTrue(minified.stats.timeSpent < 1000);
863+
}
864+
}
843865
}
844866
}).export(module);

0 commit comments

Comments
 (0)