From: Samuel Tan Date: Mon, 17 Apr 2017 23:10:54 +0000 (-0700) Subject: html/template: allow safe usage of predefined escapers in pipelines X-Git-Tag: go1.9beta1~330 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=3a2fee0389e8459e482e987a98f226a71c2ad5fb;p=gostls13.git html/template: allow safe usage of predefined escapers in pipelines Allow the predefined escapers "html", "urlquery", and "js" to be used in pipelines when they have no potential to affect the correctness or safety of the escaped pipeline output. Specifically: - "urlquery" may be used if it is the last command in the pipeline. - "html" may be used if it is the last command in the pipeline, and the pipeline does not occur in an unquoted HTML attribute value context. - "js" may be used in any pipeline, since it does not affect the merging of contextual escapers. This change will loosens the restrictions on predefined escapers introduced in golang.org/cl/37880, which will hopefully ease the upgrade path for existing template users. This change brings back the escaper-merging logic, and associated unit tests, that were removed in golang.org/cl/37880. However, a few notable changes have been made: - "_html_template_nospaceescaper" is no longer considered equivalent to "html", since the former escapes spaces, while the latter does not (see #19345). This change should not silently break any templates, since pipelines where this substituion will happen will already trigger an explicit error. - An "_eval_args_" internal directive has been added to handle pipelines containing a single explicit call to a predefined escaper, e.g. {{html .X}} (see #19353). Also, the HTMLEscape function called by the predefined text/template "html" function now escapes the NULL character as well. This effectively makes it as secure as the internal html/template HTML escapers (see #19345). While this change is backward-incompatible, it will only affect illegitimate uses of this escaper, since the NULL character is always illegal in valid HTML. Fixes #19952 Change-Id: I9b5570a80a3ea284b53901e6a1f842fc59b33d3a Reviewed-on: https://go-review.googlesource.com/40936 Reviewed-by: Russ Cox Run-TryBot: Brad Fitzpatrick TryBot-Result: Gobot Gobot --- diff --git a/src/html/template/doc.go b/src/html/template/doc.go index cb89812743..35d171c3fc 100644 --- a/src/html/template/doc.go +++ b/src/html/template/doc.go @@ -65,8 +65,10 @@ functions to each simple action pipeline, so given the excerpt At parse time each {{.}} is overwritten to add escaping functions as necessary. In this case it becomes - {{. | html}} + {{. | htmlescaper}} +where urlescaper, attrescaper, and htmlescaper are aliases for internal escaping +functions. Errors diff --git a/src/html/template/error.go b/src/html/template/error.go index 3b70ba1ec8..0e527063ea 100644 --- a/src/html/template/error.go +++ b/src/html/template/error.go @@ -186,23 +186,30 @@ const ( // ErrPredefinedEscaper: "predefined escaper ... disallowed in template" // Example: - // + //
Hello
// 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. + // pipeline output using the predefined escapers "html" or "urlquery" is + // unnecessary, and may affect the correctness or safety of the escaped + // pipeline output in Go 1.8 and earlier. + // + // In most cases, such as the given example, this error can be resolved by + // simply removing the predefined escaper from the pipeline and letting the + // contextual autoescaper handle the escaping of the pipeline. In other + // instances, where 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" + // return `link` // consider refactoring the surrounding template to make use of the // contextual autoescaper, i.e. - // link + // link + // + // To ease migration to Go 1.9 and beyond, "html" and "urlquery" will + // continue to be allowed as the last command in a pipeline. However, if the + // pipeline occurs in an unquoted attribute value context, "html" is + // disallowed. Avoid using "html" and "urlquery" entirely in new templates. ErrPredefinedEscaper ) diff --git a/src/html/template/escape.go b/src/html/template/escape.go index 3e8b455e33..3037e07b29 100644 --- a/src/html/template/escape.go +++ b/src/html/template/escape.go @@ -44,6 +44,21 @@ func escapeTemplate(tmpl *Template, node parse.Node, name string) error { return nil } +// evalArgs formats the list of arguments into a string. It is equivalent to +// fmt.Sprint(args...), except that it deferences all pointers. +func evalArgs(args ...interface{}) string { + // Optimization for simple common case of a single string argument. + if len(args) == 1 { + if s, ok := args[0].(string); ok { + return s + } + } + for i, arg := range args { + args[i] = indirectToStringerOrError(arg) + } + return fmt.Sprint(args...) +} + // funcMap maps command names to functions that render their inputs safe. var funcMap = template.FuncMap{ "_html_template_attrescaper": attrEscaper, @@ -60,13 +75,7 @@ var funcMap = template.FuncMap{ "_html_template_urlescaper": urlEscaper, "_html_template_urlfilter": urlFilter, "_html_template_urlnormalizer": urlNormalizer, -} - -// predefinedEscapers contains template predefined escapers. -var predefinedEscapers = map[string]bool{ - "html": true, - "urlquery": true, - "js": true, + "_eval_args_": evalArgs, } // escaper collects type inferences about templates and changes needed to make @@ -150,18 +159,21 @@ func (e *escaper) escapeAction(c context, n *parse.ActionNode) context { // A local variable assignment, not an interpolation. return c } - // Disallow the use of predefined escapers in pipelines. - for _, idNode := range n.Pipe.Cmds { + c = nudge(c) + // Check for disallowed use of predefined escapers in the pipeline. + for pos, 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), + if pos < len(n.Pipe.Cmds)-1 || + c.state == stateAttr && c.delim == delimSpaceOrTagEnd && ident == "html" { + 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 { case stateError: @@ -227,14 +239,51 @@ func (e *escaper) escapeAction(c context, n *parse.ActionNode) context { } // ensurePipelineContains ensures that the pipeline ends with the commands with -// the identifiers in s in order. +// the identifiers in s in order. If the pipeline ends with a predefined escaper +// (i.e. "html" or "urlquery"), merge it with the identifiers in s. func ensurePipelineContains(p *parse.PipeNode, s []string) { if len(s) == 0 { // Do not rewrite pipeline if we have no escapers to insert. return } + // Precondition: p.Cmds contains at most one predefined escaper and the + // escaper will be present at p.Cmds[len(p.Cmds)-1]. This precondition is + // always true because of the checks in escapeAction. + pipelineLen := len(p.Cmds) + if pipelineLen > 0 { + lastCmd := p.Cmds[pipelineLen-1] + if idNode, ok := lastCmd.Args[0].(*parse.IdentifierNode); ok { + if esc := idNode.Ident; predefinedEscapers[esc] { + // Pipeline ends with a predefined escaper. + if len(p.Cmds) == 1 && len(lastCmd.Args) > 1 { + // Special case: pipeline is of the form {{ esc arg1 arg2 ... argN }}, + // where esc is the predefined escaper, and arg1...argN are its arguments. + // Convert this into the equivalent form + // {{ _eval_args_ arg1 arg2 ... argN | esc }}, so that esc can be easily + // merged with the escapers in s. + lastCmd.Args[0] = parse.NewIdentifier("_eval_args_").SetTree(nil).SetPos(lastCmd.Args[0].Position()) + p.Cmds = appendCmd(p.Cmds, newIdentCmd(esc, p.Position())) + pipelineLen++ + } + // If any of the commands in s that we are about to insert is equivalent + // to the predefined escaper, use the predefined escaper instead. + dup := false + for i, escaper := range s { + if escFnsEq(esc, escaper) { + s[i] = idNode.Ident + dup = true + } + } + if dup { + // The predefined escaper will already be inserted along with the + // escapers in s, so do not copy it to the rewritten pipeline. + pipelineLen-- + } + } + } + } // 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)) + newCmds := make([]*parse.CommandNode, pipelineLen, pipelineLen+len(s)) copy(newCmds, p.Cmds) for _, name := range s { newCmds = appendCmd(newCmds, newIdentCmd(name, p.Position())) @@ -242,6 +291,45 @@ func ensurePipelineContains(p *parse.PipeNode, s []string) { p.Cmds = newCmds } +// predefinedEscapers contains template predefined escapers that are equivalent +// to some contextual escapers. Keep in sync with equivEscapers. +var predefinedEscapers = map[string]bool{ + "html": true, + "urlquery": true, +} + +// equivEscapers matches contextual escapers to equivalent predefined +// template escapers. +var equivEscapers = map[string]string{ + // The following pairs of HTML escapers provide equivalent security + // guarantees, since they all escape '\000', '\'', '"', '&', '<', and '>'. + "_html_template_attrescaper": "html", + "_html_template_htmlescaper": "html", + "_html_template_rcdataescaper": "html", + // These two URL escapers produce URLs safe for embedding in a URL query by + // percent-encoding all the reserved characters specified in RFC 3986 Section + // 2.2 + "_html_template_urlescaper": "urlquery", + // These two functions are not actually equivalent; urlquery is stricter as it + // escapes reserved characters (e.g. '#'), while _html_template_urlnormalizer + // does not. It is therefore only safe to replace _html_template_urlnormalizer + // with urlquery (this happens in ensurePipelineContains), but not the otherI've + // way around. We keep this entry around to preserve the behavior of templates + // written before Go 1.9, which might depend on this substitution taking place. + "_html_template_urlnormalizer": "urlquery", +} + +// 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 +} + // redundantFuncs[a][b] implies that funcMap[b](funcMap[a](x)) == funcMap[a](x) // for all x. var redundantFuncs = map[string]map[string]bool{ diff --git a/src/html/template/escape_test.go b/src/html/template/escape_test.go index 43869276c0..865226f855 100644 --- a/src/html/template/escape_test.go +++ b/src/html/template/escape_test.go @@ -69,7 +69,17 @@ func TestEscape(t *testing.T) { "<Goodbye>!", }, { - "overescaping", + "overescaping1", + "Hello, {{.C | html}}!", + "Hello, <Cincinatti>!", + }, + { + "overescaping2", + "Hello, {{html .C}}!", + "Hello, <Cincinatti>!", + }, + { + "overescaping3", "{{with .C}}{{$msg := .}}Hello, {{$msg}}!{{end}}", "Hello, <Cincinatti>!", }, @@ -203,6 +213,11 @@ func TestEscape(t *testing.T) { "", ``, }, + { + "jsObjValueNotOverEscaped", + "