fix: memoize css.recursiveCheck to remove exponential-time CSS DoS

Shorthand style property handlers (FontHandler and roughly a dozen
siblings) tokenize a CSS value and feed it to css.recursiveCheck,
which searches for a way to partition the tokens so every group is
accepted by one of a handful of validator functions. The search was
unmemoized: every recursive call is made on a suffix of the token
slice, and the same suffix was re-explored once per partition path
that reached it. For values with several tokens that more than one
validator accepts on their own (e.g. runs of whitespace, which
FontFamilyHandler treats as a valid empty entry), the number of such
paths grows combinatorially with the number of tokens, making
wall-clock cost roughly double per extra token. A short,
attacker-controlled style value can hang a server sanitizing
untrusted HTML.

Memoize recursiveCheck on the length of the remaining token suffix:
distinct suffixes of a given slice always have distinct lengths, and
the outcome for a suffix depends only on its length within one
top-level call (the validator set is constant throughout), so this
caches each of at most len(value) distinct subproblems exactly once.
This does not change any validator's behavior or which values are
accepted or rejected, only how many times each subproblem is solved,
turning the exponential search into a polynomial one.

Closes #225

css/handlers.go

@@ -337,16 +337,32 @@ func multiSplit(value string, seps ...string) []string {
 	return curArray
 }
 
+// recursiveCheck reports whether value can be split, in order, into groups
+// of tokens each accepted by one of funcs. The outcome depends only on how
+// many tokens remain, so memoizing on that length keeps the search
+// polynomial without changing which values are accepted.
 func recursiveCheck(value []string, funcs []func(string) bool) bool {
+	memo := make(map[int]bool, len(value))
+	return recursiveCheckMemo(value, funcs, memo)
+}
+
+func recursiveCheckMemo(value []string, funcs []func(string) bool, memo map[int]bool) bool {
+	if cached, ok := memo[len(value)]; ok {
+		return cached
+	}
+	result := false
+outer:
 	for i := 0; i < len(value); i++ {
 		tempVal := strings.Join(value[:i+1], " ")
 		for _, j := range funcs {
-			if j(tempVal) && (len(value[i+1:]) == 0 || recursiveCheck(value[i+1:], funcs)) {
+			if j(tempVal) && (len(value[i+1:]) == 0 || recursiveCheckMemo(value[i+1:], funcs, memo)) {
-				return true
+				result = true
+				break outer
 			}
 		}
 	}
-	return false
+	memo[len(value)] = result
+	return result
 }
 
 func in(value []string, arr []string) bool {

css/handlers_test.go

@@ -0,0 +1,89 @@
+// Copyright (c) 2019, David Kitchen <david@buro9.com>
+//
+// All rights reserved.
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are met:
+//
+// * Redistributions of source code must retain the above copyright notice, this
+//   list of conditions and the following disclaimer.
+//
+// * Redistributions in binary form must reproduce the above copyright notice,
+//   this list of conditions and the following disclaimer in the documentation
+//   and/or other materials provided with the distribution.
+//
+// * Neither the name of the organisation (Microcosm) nor the names of its
+//   contributors may be used to endorse or promote products derived from
+//   this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+// AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+// IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+// DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE
+// FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+// DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+// SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+// CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+// OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+package css
+
+import (
+	"strings"
+	"testing"
+	"time"
+)
+
+// Regression test for https://github.com/microcosm-cc/bluemonday/issues/225.
+// Whitespace-only tokens give recursiveCheck many equally valid split points;
+// unmemoized it re-explored each one, taking ~4s for the value below. The
+// verdict is unchanged (rejected either way), only the time to reach it.
+func TestIssue225(t *testing.T) {
+	// 20 blank tokens, as the issue #225 payload produced them.
+	pathological := "normal\n" + strings.Repeat(" ", 20) + "16px\nroboto"
+
+	const deadline = 3 * time.Second
+	const want = false
+
+	result := make(chan bool, 1)
+	go func() {
+		result <- FontHandler(pathological)
+	}()
+
+	select {
+	case got := <-result:
+		if got != want {
+			t.Errorf("FontHandler(%q) = %v, want %v", pathological, got, want)
+		}
+	case <-time.After(deadline):
+		t.Fatalf("FontHandler did not return within %v for a 20-blank-token "+
+			"value; recursiveCheck may have regressed to exponential-time "+
+			"behaviour (see issue #225)", deadline)
+	}
+}
+
+// Memoization must not change which values FontHandler accepts, only how
+// fast it decides. Expectations captured from the pre-fix behaviour.
+func TestIssue225AcceptanceUnchanged(t *testing.T) {
+	tests := []struct {
+		value string
+		want  bool
+	}{
+		// Valid "font" shorthand values.
+		{"italic bold 12px/30px Georgia, serif", true},
+		{"icon", true},
+		{"caption", true},
+		{"normal 16px Roboto", true},
+		{"bold 16px/1.5 Arial, sans-serif", true},
+		// Invalid/unsafe values that must stay rejected.
+		{"expression(alert(1))", false},
+		{"javascript:alert(1)", false},
+		{"url(evil.com)", false},
+	}
+	for _, tt := range tests {
+		if got := FontHandler(tt.value); got != tt.want {
+			t.Errorf("FontHandler(%q) = %v, want %v", tt.value, got, tt.want)
+		}
+	}
+}