Sanitize css properties of style attributes (#1)

microcosm-cc#43

policy.go

@@ -94,6 +94,9 @@ type Policy struct {
 	setOfElementsAllowedWithoutAttrs map[string]struct{}
 
 	setOfElementsToSkipContent map[string]struct{}
+
+	// allowed style property names
+	allowedStyleProperties map[string]struct{}
 }
 
 type attrPolicy struct {
@@ -121,6 +124,7 @@ func (p *Policy) init() {
 		p.allowURLSchemes = make(map[string]urlPolicy)
 		p.setOfElementsAllowedWithoutAttrs = make(map[string]struct{})
 		p.setOfElementsToSkipContent = make(map[string]struct{})
+		p.allowedStyleProperties = make(map[string]struct{})
 		p.initialized = true
 	}
 }
@@ -431,6 +435,20 @@ func (p *Policy) AllowElementsContent(names ...string) *Policy {
 	return p
 }
 
+// AllowStyleProperties allows the given style properties in the style
+// attribute (note: does not style in inline <style> tags)
+//
+// ex. AllowStyleProperties("font-size", "text-align")
+func (p *Policy) AllowStyleProperties(properties ...string) *Policy {
+	p.init()
+
+	for _, property := range properties {
+		p.allowedStyleProperties[property] = struct{}{}
+	}
+
+	return p
+}
+
 // addDefaultElementsWithoutAttrs adds the HTML elements that we know are valid
 // without any attributes to an internal map.
 // i.e. we know that <table> is valid, but <bdo> isn't valid as the "dir" attr

sanitize.go

@@ -36,6 +36,8 @@ import (
 	"strings"
 
 	"golang.org/x/net/html"
+
+	cssparser "github.com/aymerick/douceur/parser"
 )
 
 // Sanitize takes a string that contains a HTML fragment or document and applies
@@ -256,6 +258,11 @@ func (p *Policy) sanitizeAttrs(
 	// whitelisted explicitly or globally.
 	cleanAttrs := []html.Attribute{}
 	for _, htmlAttr := range attrs {
+		// sanitize style attr
+		if htmlAttr.Key == "style" {
+			htmlAttr.Val = p.sanitizeCSSDeclarations(htmlAttr.Val)
+		}
+
 		// Is there an element specific attribute policy that applies?
 		if ap, ok := aps[htmlAttr.Key]; ok {
 			if ap.regexp != nil {
@@ -537,3 +544,19 @@ func linkable(elementName string) bool {
 		return false
 	}
 }
+
+func (p *Policy) sanitizeCSSDeclarations(declarations string) string {
+	decs, err := cssparser.ParseDeclarations(declarations)
+	if err != nil {
+		return "" // TODO error handling?
+	}
+
+	strs := make([]string, 0, len(decs))
+	for _, dec := range decs {
+		if _, allowed := p.allowedStyleProperties[dec.Property]; allowed {
+			strs = append(strs, dec.String())
+		}
+	}
+
+	return strings.Join(strs, "")
+}

sanitize_test.go

@@ -1514,3 +1514,69 @@ func TestTargetBlankNoOpener(t *testing.T) {
 	}
 	wg.Wait()
 }
+
+func TestAllowStyles(t *testing.T) {
+	p := UGCPolicy()
+	p.AllowStyleProperties("text-align", "font-size")
+	p.AllowAttrs("style").OnElements("div")
+
+	tests := []test{
+		{
+			in:       `<div style="background: javascript:alert('xss');text-align: right;font-size: small;">no xss</div>`,
+			expected: `<div style="text-align: right;font-size: small;">no xss</div>`,
+		},
+		{
+			in:       `<div style="text-align: right;background: javascript:alert('xss');font-size: small;">no xss</div>`,
+			expected: `<div style="text-align: right;font-size: small;">no xss</div>`,
+		},
+		{
+			in:       `<div style="text-align: right;font-size: small;background: javascript:alert('xss');">no xss</div>`,
+			expected: `<div style="text-align: right;font-size: small;">no xss</div>`,
+		},
+		{
+			in:       `<div style="text-align: right;font-size: small;background: javascript:alert('xss')">no xss</div>`,
+			expected: `<div style="text-align: right;font-size: small;">no xss</div>`,
+		},
+		{
+			in:       `<div style="background: javascript:alert('xss');">no xss</div>`,
+			expected: `<div style="">no xss</div>`,
+		},
+		{
+			in:       `<div style="background: javascript:alert('xss')">no xss</div>`,
+			expected: `<div style="">no xss</div>`,
+		},
+		{
+			in:       `<div style="text-align;background: javascript:alert('xss')">no xss</div>`,
+			expected: `<div style="">no xss</div>`,
+		},
+		{
+			in:       `<div style="text-align">no xss</div>`,
+			expected: `<div style="">no xss</div>`,
+		},
+		{
+			in:       `<div style="text-align: right !important;)">no xss</div>`,
+			expected: `<div style="text-align: right !important;">no xss</div>`,
+		},
+	}
+
+	// These tests are run concurrently to enable the race detector to pick up
+	// potential issues
+	wg := sync.WaitGroup{}
+	wg.Add(len(tests))
+	for ii, tt := range tests {
+		go func(ii int, tt test) {
+			out := p.Sanitize(tt.in)
+			if out != tt.expected {
+				t.Errorf(
+					"test %d failed;\ninput   : %s\noutput  : %s\nexpected: %s",
+					ii,
+					tt.in,
+					out,
+					tt.expected,
+				)
+			}
+			wg.Done()
+		}(ii, tt)
+	}
+	wg.Wait()
+}