html/template: {{value | .Method}} panics and prevents escaping #7379
Labels
Milestone
Comments
|
Labels changed: added release-go1.3, repo-main, size-s. Owner changed to @robpike. Status changed to Accepted. |
|
Comment 2 by rpolzer@google.com: I should add that problem 2 seems a lot more disturbing, as that may be usable for XSS.
This seems to stem from this function:
func escapeTemplates(tmpl *Template, names ...string) error {
e := newEscaper(tmpl)
for _, name := range names {
c, _ := e.escapeTree(context{}, name, 0)
var err error
// ...
tmpl.escaped = true
}
e.commit()
return nil
}
This writes to tmpl.escaped once ANY of the templates is escaped, even if a later one
fails escaping! This assignment should simply not exist, as tmpl.escaped is already set
in Template.Execute when this function returns. The panic BTW happens in commit(), when
escaped is already set.
Even worse, in lookupAndEscapeTemplate, tmpl.escaped is checked for whether to escape
another than the root template. This looks dangerous.
Probably this escaped field should rather exist once per template... otherwise mixing
calls of .Execute() and .ExecuteTemplate() can cause non-escaped data too.
|
|
Comment 3 by rpolzer@google.com: (Deleted and edited my comment, as it was quite wrong... sorry)
I should add that problem 2 seems a lot more disturbing, as that may be usable for XSS.
This seems to stem from this function:
func escapeTemplates(tmpl *Template, names ...string) error {
e := newEscaper(tmpl)
for _, name := range names {
c, _ := e.escapeTree(context{}, name, 0)
var err error
// ...
tmpl.escaped = true
}
e.commit()
return nil
}
This writes to tmpl.escaped before the commit() call, which panics in my case. The
result is that commit() never takes place.
Fix suggestion: moving the tmpl.escaped = true either to both callers (currently only
one caller does it), or to behind the e.commit().
|
CL https://golang.org/cl/85240043 references this issue. |
|
This issue was closed by revision 51fba7d. Status changed to Fixed. |
This issue was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
by rpolzer@google.com:
What steps will reproduce the problem? If possible, include a link to a program on play.golang.org. 1. Instantiate a html/template with {{... | $someObject.SomeMethod ...}}. http://play.golang.org/p/YrYD_SiWV_ is an example. 2. Execute the template 3. ?? 4. PANIC! What is the expected output? Attempt 0: <html><0></html> Attempt 1: <html><0></html> Attempt 2: <html><0></html> What do you see instead? Attempt 0: PANICKED: interface conversion: parse.Node is *parse.FieldNode, not *parse.IdentifierNode Attempt 1: <html><0></html> Attempt 2: <html><0></html> Problem 1: the first run of the template panics. Problem 2: HTML escaping isn't performed in later runs, possibly being a security issue. Web servers that catch panics on HTTP requests may run into this. The "expected output" can be seen when changing my example from {{0 | .SomeMethod}} to {{.SomeMethod 0}}. Which compiler are you using (5g, 6g, 8g, gccgo)? 8g Which operating system are you using? Ubuntu Linux Precise Which version are you using? (run 'go version') go version go1.2 linux/amd64 Please provide any additional information below. I believe this is to be fixed by this diff to ensurePipelineContains: --- /usr/share/go/src/pkg/html/template/escape.go 2012-04-06 15:08:32.000000000 +0200 +++ escape.go 2014-02-21 13:09:08.038932046 +0100 @@ -228,6 +228,7 @@ } } idents = p.Cmds[i+1:] + break } dups := 0 for _, id := range idents { The issue is that this loop seems to be intended to find the longest trailing sequence of IdentifierNodes (i.e. function calls); however in my case I'm hitting a VariableNode instead when doing a method call: runtime.panic(0x17aa40, 0x1062e8e0) /tmp/sandbox/go/src/pkg/runtime/panic.c:248 +0x160 assertI2Tret(0x183740, 0xfefcc8f0, 0x1062e6a0, 0xfeee1c38) /tmp/sandbox/go/src/pkg/runtime/iface.goc:254 +0x120 runtime.assertI2T(0x183740, 0xfefcc8f0, 0x1062e6a0, 0x3, 0x42900, ...) /tmp/sandbox/go/src/pkg/runtime/iface.goc:234 +0x40 html/template.ensurePipelineContains(0x1061a390, 0x1062e8c0, 0x1, 0x3) /tmp/sandbox/go/src/pkg/html/template/escape.go:233 +0x240 html/template.(*escaper).commit(0x1062e700, 0x0) /tmp/sandbox/go/src/pkg/html/template/escape.go:737 +0x320 html/template.escapeTemplates(0x1062d110, 0xfeee1eac, 0x1, 0x1, 0x106426c0, ...) /tmp/sandbox/go/src/pkg/html/template/escape.go:46 +0x500 html/template.(*Template).escape(0x1062d110, 0xfeee1ef0, 0x0, 0x0) /tmp/sandbox/go/src/pkg/html/template/template.go:55 +0x160 html/template.(*Template).Execute(0x1062d110, 0xfefcc940, 0x10600138, 0x174900, 0x0, ...) /tmp/sandbox/go/src/pkg/html/template/template.go:66 +0x40 main.func·002() /tmpfs/gosandbox-10a1b83d_28ea593c_1c442229_3c6439ba_6dff8a33/prog.go:25 +0xc0The text was updated successfully, but these errors were encountered: