From 967d68c00a97c69009dd97a72c1400cbe2b14355 Mon Sep 17 00:00:00 2001 From: Mike Samuel Date: Fri, 23 Sep 2011 09:25:10 -0700 Subject: [PATCH] exp/template/html: tighten rules on dynamic attr names. R=nigeltao CC=golang-dev https://golang.org/cl/5076049 --- src/pkg/exp/template/html/Makefile | 1 + src/pkg/exp/template/html/attr.go | 184 +++++++++++++++++++++++ src/pkg/exp/template/html/content.go | 4 + src/pkg/exp/template/html/escape_test.go | 40 +++++ src/pkg/exp/template/html/html.go | 16 +- src/pkg/exp/template/html/transition.go | 39 ++--- 6 files changed, 253 insertions(+), 31 deletions(-) create mode 100644 src/pkg/exp/template/html/attr.go diff --git a/src/pkg/exp/template/html/Makefile b/src/pkg/exp/template/html/Makefile index 9032aead87..2ccbdd3e85 100644 --- a/src/pkg/exp/template/html/Makefile +++ b/src/pkg/exp/template/html/Makefile @@ -6,6 +6,7 @@ include ../../../../Make.inc TARG=exp/template/html GOFILES=\ + attr.go\ clone.go\ content.go\ context.go\ diff --git a/src/pkg/exp/template/html/attr.go b/src/pkg/exp/template/html/attr.go new file mode 100644 index 0000000000..cc57f8bd8a --- /dev/null +++ b/src/pkg/exp/template/html/attr.go @@ -0,0 +1,184 @@ +// Copyright 2011 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package html + +// attrType[n] describes the value of the given attribute. +// If an attribute affects (or can mask) the encoding or interpretation of +// other content, or affects the contents, idempotency, or credentials of a +// network message, then the value in this map is contentTypeUnsafe. +// This map is derived from HTML5, specifically +// http://www.w3.org/TR/html5/Overview.html#attributes-1 and +// http://www.w3.org/TR/html5/Overview.html#event-handlers-on-elements-document-objects-and-window-objects +// as well as "%URI"-typed attributes from +// http://www.w3.org/TR/html4/index/attributes.html +var attrType = map[string]contentType{ + "accept": contentTypePlain, + "accept-charset": contentTypeUnsafe, + "action": contentTypeURL, + "alt": contentTypePlain, + "archive": contentTypeURL, + "async": contentTypeUnsafe, + "autocomplete": contentTypePlain, + "autofocus": contentTypePlain, + "autoplay": contentTypePlain, + "background": contentTypeURL, + "border": contentTypePlain, + "checked": contentTypePlain, + "cite": contentTypeURL, + "challenge": contentTypeUnsafe, + "charset": contentTypeUnsafe, + "class": contentTypePlain, + "classid": contentTypeURL, + "codebase": contentTypeURL, + "cols": contentTypePlain, + "colspan": contentTypePlain, + "content": contentTypeUnsafe, + "contenteditable": contentTypePlain, + "contextmenu": contentTypePlain, + "controls": contentTypePlain, + "coords": contentTypePlain, + "crossorigin": contentTypeUnsafe, + "data": contentTypeURL, + "datetime": contentTypePlain, + "default": contentTypePlain, + "defer": contentTypeUnsafe, + "dir": contentTypePlain, + "dirname": contentTypePlain, + "disabled": contentTypePlain, + "draggable": contentTypePlain, + "dropzone": contentTypePlain, + "enctype": contentTypeUnsafe, + "for": contentTypePlain, + "form": contentTypeUnsafe, + "formaction": contentTypeURL, + "formenctype": contentTypeUnsafe, + "formmethod": contentTypeUnsafe, + "formnovalidate": contentTypeUnsafe, + "formtarget": contentTypePlain, + "headers": contentTypePlain, + "height": contentTypePlain, + "hidden": contentTypePlain, + "high": contentTypePlain, + "href": contentTypeURL, + "hreflang": contentTypePlain, + "http-equiv": contentTypeUnsafe, + "icon": contentTypeURL, + "id": contentTypePlain, + "ismap": contentTypePlain, + "keytype": contentTypeUnsafe, + "kind": contentTypePlain, + "label": contentTypePlain, + "lang": contentTypePlain, + "language": contentTypeUnsafe, + "list": contentTypePlain, + "longdesc": contentTypeURL, + "loop": contentTypePlain, + "low": contentTypePlain, + "manifest": contentTypeURL, + "max": contentTypePlain, + "maxlength": contentTypePlain, + "media": contentTypePlain, + "mediagroup": contentTypePlain, + "method": contentTypeUnsafe, + "min": contentTypePlain, + "multiple": contentTypePlain, + "name": contentTypePlain, + "novalidate": contentTypeUnsafe, + "onabort": contentTypeJS, + "onblur": contentTypeJS, + "oncanplay": contentTypeJS, + "oncanplaythrough": contentTypeJS, + "onchange": contentTypeJS, + "onclick": contentTypeJS, + "oncontextmenu": contentTypeJS, + "oncuechange": contentTypeJS, + "ondblclick": contentTypeJS, + "ondrag": contentTypeJS, + "ondragend": contentTypeJS, + "ondragenter": contentTypeJS, + "ondragleave": contentTypeJS, + "ondragover": contentTypeJS, + "ondragstart": contentTypeJS, + "ondrop": contentTypeJS, + "ondurationchange": contentTypeJS, + "onemptied": contentTypeJS, + "onended": contentTypeJS, + "onerror": contentTypeJS, + "onfocus": contentTypeJS, + "oninput": contentTypeJS, + "oninvalid": contentTypeJS, + "onkeydown": contentTypeJS, + "onkeypress": contentTypeJS, + "onkeyup": contentTypeJS, + "onload": contentTypeJS, + "onloadeddata": contentTypeJS, + "onloadedmetadata": contentTypeJS, + "onloadstart": contentTypeJS, + "onmousedown": contentTypeJS, + "onmousemove": contentTypeJS, + "onmouseout": contentTypeJS, + "onmouseover": contentTypeJS, + "onmouseup": contentTypeJS, + "onmousewheel": contentTypeJS, + "onpause": contentTypeJS, + "onplay": contentTypeJS, + "onplaying": contentTypeJS, + "onprogress": contentTypeJS, + "onratechange": contentTypeJS, + "onreadystatechange": contentTypeJS, + "onreset": contentTypeJS, + "onscroll": contentTypeJS, + "onseeked": contentTypeJS, + "onseeking": contentTypeJS, + "onselect": contentTypeJS, + "onshow": contentTypeJS, + "onstalled": contentTypeJS, + "onsubmit": contentTypeJS, + "onsuspend": contentTypeJS, + "ontimeupdate": contentTypeJS, + "onvolumechange": contentTypeJS, + "onwaiting": contentTypeJS, + "open": contentTypePlain, + "optimum": contentTypePlain, + "pattern": contentTypeUnsafe, + "placeholder": contentTypePlain, + "poster": contentTypeURL, + "profile": contentTypeURL, + "preload": contentTypePlain, + "pubdate": contentTypePlain, + "radiogroup": contentTypePlain, + "readonly": contentTypePlain, + "rel": contentTypeUnsafe, + "required": contentTypePlain, + "reversed": contentTypePlain, + "rows": contentTypePlain, + "rowspan": contentTypePlain, + "sandbox": contentTypeUnsafe, + "spellcheck": contentTypePlain, + "scope": contentTypePlain, + "scoped": contentTypePlain, + "seamless": contentTypePlain, + "selected": contentTypePlain, + "shape": contentTypePlain, + "size": contentTypePlain, + "sizes": contentTypePlain, + "span": contentTypePlain, + "src": contentTypeURL, + "srcdoc": contentTypeHTML, + "srclang": contentTypePlain, + "start": contentTypePlain, + "step": contentTypePlain, + "style": contentTypeCSS, + "tabindex": contentTypePlain, + "target": contentTypePlain, + "title": contentTypePlain, + "type": contentTypeUnsafe, + "usemap": contentTypeURL, + "value": contentTypeUnsafe, + "width": contentTypePlain, + "wrap": contentTypePlain, + + // TODO: data-* attrs? Recognize data-foo-url and similar. +} diff --git a/src/pkg/exp/template/html/content.go b/src/pkg/exp/template/html/content.go index 8b9809b982..dcaff8c15c 100644 --- a/src/pkg/exp/template/html/content.go +++ b/src/pkg/exp/template/html/content.go @@ -64,6 +64,10 @@ const ( contentTypeJS contentTypeJSStr contentTypeURL + // contentTypeUnsafe is used in attr.go for values that affect how + // embedded content and network messages are formed, vetted, + // or interpreted; or which credentials network messages carry. + contentTypeUnsafe ) // stringify converts its arguments to a string and the type of the content. diff --git a/src/pkg/exp/template/html/escape_test.go b/src/pkg/exp/template/html/escape_test.go index 8a64515dec..1ce66c5fb1 100644 --- a/src/pkg/exp/template/html/escape_test.go +++ b/src/pkg/exp/template/html/escape_test.go @@ -553,11 +553,51 @@ func TestEscape(t *testing.T) { // Treated as JS since quotes are inserted. ``, }, + { + "bad dynamic attribute name 1", + // Allow checked, selected, disabled, but not JS or + // CSS attributes. + ``, + ``, + }, + { + "bad dynamic attribute name 2", + `
`, + `
`, + }, + { + "bad dynamic attribute name 3", + // Allow title or alt, but not a URL. + ``, + ``, + }, + { + "bad dynamic attribute name 4", + // Structure preservation requires values to associate + // with a consistent attribute. + ``, + ``, + }, { "dynamic element name", `...`, `

...`, }, + { + "bad dynamic element name", + // Dynamic element names are typically used to switch + // between (thead, tfoot, tbody), (ul, ol), (th, td), + // and other replaceable sets. + // We do not currently easily support (ul, ol). + // If we do change to support that, this test should + // catch failures to filter out special tag names which + // would violate the structure preservation property -- + // if any special tag name could be substituted, then + // the content could be raw text/RCDATA for some inputs + // and regular HTML content for others. + `<{{"script"}}>{{"doEvil()"}}`, + `<script>doEvil()</script>`, + }, } for _, test := range tests { diff --git a/src/pkg/exp/template/html/html.go b/src/pkg/exp/template/html/html.go index 3924b193db..6ef66dd6c3 100644 --- a/src/pkg/exp/template/html/html.go +++ b/src/pkg/exp/template/html/html.go @@ -7,6 +7,7 @@ package html import ( "bytes" "fmt" + "strings" "utf8" ) @@ -220,10 +221,23 @@ func htmlNameFilter(args ...interface{}) string { if t == contentTypeHTMLAttr { return s } + if len(s) == 0 { + // Avoid violation of structure preservation. + // . + // Without this, if .K is empty then .V is the value of + // checked, but otherwise .V is the value of the attribute + // named .K. + return filterFailsafe + } + s = strings.ToLower(s) + if t := attrType[s]; t != contentTypePlain && attrType["on"+s] != contentTypeJS { + // TODO: Split attr and element name part filters so we can whitelist + // attributes. + return filterFailsafe + } for _, r := range s { switch { case '0' <= r && r <= '9': - case 'A' <= r && r <= 'Z': case 'a' <= r && r <= 'z': default: return filterFailsafe diff --git a/src/pkg/exp/template/html/transition.go b/src/pkg/exp/template/html/transition.go index 3be3a01a8a..dd8cd59a6f 100644 --- a/src/pkg/exp/template/html/transition.go +++ b/src/pkg/exp/template/html/transition.go @@ -105,12 +105,17 @@ func tTag(c context, s []byte) (context, int) { state, attr := stateTag, attrNone if i != j { canonAttrName := strings.ToLower(string(s[i:j])) - if urlAttr[canonAttrName] { + switch attrType[canonAttrName] { + case contentTypeURL: attr = attrURL - } else if strings.HasPrefix(canonAttrName, "on") { - attr = attrScript - } else if canonAttrName == "style" { + case contentTypeCSS: attr = attrStyle + case contentTypeJS: + attr = attrScript + default: + if strings.HasPrefix(canonAttrName, "on") { + attr = attrScript + } } if j == len(s) { state = stateAttrName @@ -532,29 +537,3 @@ func eatWhiteSpace(s []byte, i int) int { } return len(s) } - -// urlAttr is the set of attribute names whose values are URLs. -// It consists of all "%URI"-typed attributes from -// http://www.w3.org/TR/html4/index/attributes.html -// as well as those attributes defined at -// http://dev.w3.org/html5/spec/index.html#attributes-1 -// whose Value column in that table matches -// "Valid [non-empty] URL potentially surrounded by spaces". -var urlAttr = map[string]bool{ - "action": true, - "archive": true, - "background": true, - "cite": true, - "classid": true, - "codebase": true, - "data": true, - "formaction": true, - "href": true, - "icon": true, - "longdesc": true, - "manifest": true, - "poster": true, - "profile": true, - "src": true, - "usemap": true, -}