Skip to content

G
go
Commit 9ffd9339 authored by Samuel Tan's avatar Samuel Tan Committed by Russ Cox
Browse files
Options

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 <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: default avatarRuss Cox <rsc@golang.org>
parent 221541ec
Hide whitespace changes
Inline Side-by-side
Showing with 81 additions and 148 deletions
src/html/template/error.go
View file @ 9ffd9339
......@@ -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:
// <a href="{{.X | urlquery}}">
// 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 "<a href='+input+'>link</a>"
// consider refactoring the surrounding template to make use of the
// contextual autoescaper, i.e.
// <a href='{{.X}}'>link</a>
ErrPredefinedEscaper
)
func (e *Error) Error() string {
......
src/html/template/escape.go
View file @ 9ffd9339
......@@ -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{
......
src/html/template/escape_test.go
View file @ 9ffd9339
......@@ -69,17 +69,7 @@ func TestEscape(t *testing.T) {
"&lt;Goodbye&gt;!",
},
{
"overescaping1",
"Hello, {{.C | html}}!",
"Hello, &lt;Cincinatti&gt;!",
},
{
"overescaping2",
"Hello, {{html .C}}!",
"Hello, &lt;Cincinatti&gt;!",
},
{
"overescaping3",
"overescaping",
"{{with .C}}{{$msg := .}}Hello, {{$msg}}!{{end}}",
"Hello, &lt;Cincinatti&gt;!",
},
......@@ -213,11 +203,6 @@ func TestEscape(t *testing.T) {
"<script>alert({{.A}})</script>",
`<script>alert(["\u003ca\u003e","\u003cb\u003e"])</script>`,
},
{
"jsObjValueNotOverEscaped",
"<button onclick='alert({{.A | html}})'>",
`<button onclick='alert([&#34;\u003ca\u003e&#34;,&#34;\u003cb\u003e&#34;])'>`,
},
{
"jsStr",
"<button onclick='alert(&quot;{{.H}}&quot;)'>",
......@@ -233,12 +218,6 @@ func TestEscape(t *testing.T) {
`<button onclick='alert({{.M}})'>`,
`<button onclick='alert({&#34;\u003cfoo\u003e&#34;:&#34;O&#39;Reilly&#34;})'>`,
},
{
"jsStrNotUnderEscaped",
"<button onclick='alert({{.C | urlquery}})'>",
// URL escaped, then quoted for JS.
`<button onclick='alert(&#34;%3CCincinatti%3E&#34;)'>`,
},
{
"jsRe",
`<button onclick='alert(/{{"foo+bar"}}/.test(""))'>`,
......@@ -970,8 +949,32 @@ func TestErrors(t *testing.T) {
`<a=foo>`,
`: expected space, attr name, or end of tag, but got "=foo>"`,
},
{
`Hello, {{. | html}}!`,
// Piping to html is disallowed.
`predefined escaper "html" disallowed in template`,
},
{
`Hello, {{. | html | print}}!`,
// html is disallowed, even if it is not the last command in the pipeline.
`predefined escaper "html" disallowed in template`,
},
{
`Hello, {{html .}}!`,
// Calling html is disallowed.
`predefined escaper "html" disallowed in template`,
},
{
`Hello, {{. | urlquery | html}}!`,
// urlquery is disallowed; first disallowed escaper in the pipeline is reported in error.
`predefined escaper "urlquery" disallowed in template`,
},
{
`<script>function do{{. | js}}() { return 1 }</script>`,
// js is disallowed.
`predefined escaper "js" disallowed in template`,
},
}
for _, test := range tests {
buf := new(bytes.Buffer)
tmpl, err := New("z").Parse(test.input)
......@@ -1518,61 +1521,16 @@ func TestEnsurePipelineContains(t *testing.T) {
".X",
[]string{},
},
{
"{{.X | html}}",
".X | html",
[]string{},
},
{
"{{.X}}",
".X | html",
[]string{"html"},
},
{
"{{.X | html}}",
".X | html | urlquery",
[]string{"urlquery"},
},
{
"{{.X | html | urlquery}}",
".X | html | urlquery",
[]string{"urlquery"},
},
{
"{{.X | html | urlquery}}",
".X | html | urlquery",
[]string{"html", "urlquery"},
},
{
"{{.X | html | urlquery}}",
".X | html | urlquery",
[]string{"html"},
},
{
"{{.X | urlquery}}",
".X | html | urlquery",
[]string{"html", "urlquery"},
},
{
"{{.X | html | print}}",
".X | urlquery | html | print",
[]string{"urlquery", "html"},
},
{
"{{($).X | html | print}}",
"($).X | urlquery | html | print",
[]string{"urlquery", "html"},
},
{
"{{.X | print 2 | .f 3}}",
".X | print 2 | .f 3 | urlquery | html",
[]string{"urlquery", "html"},
},
{
"{{.X | html | print 2 | .f 3}}",
".X | urlquery | html | print 2 | .f 3",
[]string{"urlquery", "html"},
},
{
// covering issue 10801
"{{.X | js.x }}",
......@@ -1605,11 +1563,7 @@ func TestEnsurePipelineContains(t *testing.T) {
func TestEscapeMalformedPipelines(t *testing.T) {
tests := []string{
"{{ 0 | $ }}",
"{{ 0 | $ | urlquery }}",
"{{ 0 | $ | urlquery | html }}",
"{{ 0 | (nil) }}",
"{{ 0 | (nil) | html }}",
"{{ 0 | (nil) | html | urlquery }}",
}
for _, test := range tests {
var b bytes.Buffer
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7