From: Samuel Tan Date: Wed, 5 Apr 2017 01:26:21 +0000 (-0700) Subject: html/template: panic if predefined escapers are found in pipelines during rewriting X-Git-Tag: go1.9beta1~767 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=9ffd9339da503b50571ec6806e5d6d2cf5d5912a;p=gostls13.git html/template: panic if predefined escapers are found in pipelines during rewriting Report an error if a predefined escaper (i.e. "html", "urlquery", or "js") is found in a pipeline that will be rewritten by the contextual auto-escaper, instead of trying to merge the escaper-inserted escaping directives with these predefined escapers. This merging behavior is a source of several security and correctness bugs (eee #19336, #19345, #19352, and #19353.) This merging logic was originally intended to ease migration of text/template templates with user-defined escapers to html/template. Now that migration is no longer an issue, this logic can be safely removed. NOTE: this is a backward-incompatible change that fixes known security bugs (see linked issues for more details). It will explicitly break users that attempt to execute templates with pipelines containing predefined escapers. Fixes #19336, #19345, #19352, #19353 Change-Id: I46b0ca8a2809d179c13c0d4f42b63126ed1c3b49 Reviewed-on: https://go-review.googlesource.com/37880 Run-TryBot: Russ Cox TryBot-Result: Gobot Gobot Reviewed-by: Russ Cox --- diff --git a/src/html/template/error.go b/src/html/template/error.go index cbcaf92e4a..3b70ba1ec8 100644 --- a/src/html/template/error.go +++ b/src/html/template/error.go @@ -183,6 +183,27 @@ const ( // Look for missing semicolons inside branches, and maybe add // parentheses to make it clear which interpretation you intend. ErrSlashAmbig + + // ErrPredefinedEscaper: "predefined escaper ... disallowed in template" + // Example: + // + // Discussion: + // Package html/template already contextually escapes all pipelines to + // produce HTML output safe against code injection. Manually escaping + // pipeline output using the predefined escapers "html", "urlquery", or "js" + // is unnecessary, and might affect the correctness or safety of the escaped + // pipeline output. In the above example, "urlquery" should simply be + // removed from the pipeline so that escaping is performed solely by the + // contextual autoescaper. + // If the predefined escaper occurs in the middle of a pipeline where + // subsequent commands expect escaped input, e.g. + // {{.X | html | makeALink}} + // where makeALink does + // return "link" + // consider refactoring the surrounding template to make use of the + // contextual autoescaper, i.e. + // link + ErrPredefinedEscaper ) func (e *Error) Error() string { diff --git a/src/html/template/escape.go b/src/html/template/escape.go index 0e7d2be143..106067f792 100644 --- a/src/html/template/escape.go +++ b/src/html/template/escape.go @@ -62,14 +62,11 @@ var funcMap = template.FuncMap{ "_html_template_urlnormalizer": urlNormalizer, } -// equivEscapers matches contextual escapers to equivalent template builtins. -var equivEscapers = map[string]string{ - "_html_template_attrescaper": "html", - "_html_template_htmlescaper": "html", - "_html_template_nospaceescaper": "html", - "_html_template_rcdataescaper": "html", - "_html_template_urlescaper": "urlquery", - "_html_template_urlnormalizer": "urlquery", +// predefinedEscapers contains template predefined escapers. +var predefinedEscapers = map[string]bool{ + "html" : true, + "urlquery": true, + "js": true, } // escaper collects type inferences about templates and changes needed to make @@ -133,12 +130,37 @@ func (e *escaper) escape(c context, n parse.Node) context { panic("escaping " + n.String() + " is unimplemented") } +// allIdents returns the names of the identifiers under the Ident field of the node, +// which might be a singleton (Identifier) or a slice (Field or Chain). +func allIdents(node parse.Node) []string { + switch node := node.(type) { + case *parse.IdentifierNode: + return []string{node.Ident} + case *parse.FieldNode: + return node.Ident + case *parse.ChainNode: + return node.Field + } + return nil +} + // escapeAction escapes an action template node. func (e *escaper) escapeAction(c context, n *parse.ActionNode) context { if len(n.Pipe.Decl) != 0 { // A local variable assignment, not an interpolation. return c } + // Disallow the use of predefined escapers in pipelines. + for _, idNode := range n.Pipe.Cmds { + for _, ident := range allIdents(idNode.Args[0]) { + if _, ok := predefinedEscapers[ident]; ok { + return context{ + state: stateError, + err: errorf(ErrPredefinedEscaper, n, n.Line, "predefined escaper %q disallowed in template", ident), + } + } + } + } c = nudge(c) s := make([]string, 0, 3) switch c.state { @@ -204,69 +226,16 @@ func (e *escaper) escapeAction(c context, n *parse.ActionNode) context { return c } -// allIdents returns the names of the identifiers under the Ident field of the node, -// which might be a singleton (Identifier) or a slice (Field or Chain). -func allIdents(node parse.Node) []string { - switch node := node.(type) { - case *parse.IdentifierNode: - return []string{node.Ident} - case *parse.FieldNode: - return node.Ident - case *parse.ChainNode: - return node.Field - } - return nil -} - -// ensurePipelineContains ensures that the pipeline has commands with +// ensurePipelineContains ensures that the pipeline ends with the commands with // the identifiers in s in order. -// If the pipeline already has some of the sanitizers, do not interfere. -// For example, if p is (.X | html) and s is ["escapeJSVal", "html"] then it -// has one matching, "html", and one to insert, "escapeJSVal", to produce -// (.X | escapeJSVal | html). func ensurePipelineContains(p *parse.PipeNode, s []string) { if len(s) == 0 { + // Do not rewrite pipeline if we have no escapers to insert. return } - n := len(p.Cmds) - // Find the identifiers at the end of the command chain. - idents := p.Cmds - for i := n - 1; i >= 0; i-- { - if cmd := p.Cmds[i]; len(cmd.Args) != 0 { - if _, ok := cmd.Args[0].(*parse.IdentifierNode); ok { - continue - } - } - idents = p.Cmds[i+1:] - } - dups := 0 - for _, idNode := range idents { - for _, ident := range allIdents(idNode.Args[0]) { - if escFnsEq(s[dups], ident) { - dups++ - if dups == len(s) { - return - } - } - } - } - newCmds := make([]*parse.CommandNode, n-len(idents), n+len(s)-dups) + // Rewrite the pipeline, creating the escapers in s at the end of the pipeline. + newCmds := make([]*parse.CommandNode, len(p.Cmds), len(p.Cmds)+len(s)) copy(newCmds, p.Cmds) - // Merge existing identifier commands with the sanitizers needed. - for _, idNode := range idents { - pos := idNode.Args[0].Position() - for _, ident := range allIdents(idNode.Args[0]) { - i := indexOfStr(ident, s, escFnsEq) - if i != -1 { - for _, name := range s[:i] { - newCmds = appendCmd(newCmds, newIdentCmd(name, pos)) - } - s = s[i+1:] - } - } - newCmds = appendCmd(newCmds, idNode) - } - // Create any remaining sanitizers. for _, name := range s { newCmds = appendCmd(newCmds, newIdentCmd(name, p.Position())) } @@ -318,17 +287,6 @@ func indexOfStr(s string, strs []string, eq func(a, b string) bool) int { return -1 } -// escFnsEq reports whether the two escaping functions are equivalent. -func escFnsEq(a, b string) bool { - if e := equivEscapers[a]; e != "" { - a = e - } - if e := equivEscapers[b]; e != "" { - b = e - } - return a == b -} - // newIdentCmd produces a command containing a single identifier node. func newIdentCmd(identifier string, pos parse.Pos) *parse.CommandNode { return &parse.CommandNode{ diff --git a/src/html/template/escape_test.go b/src/html/template/escape_test.go index 0c854c31a3..5dfb09b500 100644 --- a/src/html/template/escape_test.go +++ b/src/html/template/escape_test.go @@ -69,17 +69,7 @@ func TestEscape(t *testing.T) { "<Goodbye>!", }, { - "overescaping1", - "Hello, {{.C | html}}!", - "Hello, <Cincinatti>!", - }, - { - "overescaping2", - "Hello, {{html .C}}!", - "Hello, <Cincinatti>!", - }, - { - "overescaping3", + "overescaping", "{{with .C}}{{$msg := .}}Hello, {{$msg}}!{{end}}", "Hello, <Cincinatti>!", }, @@ -213,11 +203,6 @@ func TestEscape(t *testing.T) { "", ``, }, - { - "jsObjValueNotOverEscaped", - "