html/template: Paths in attributes not escaped correctly

[ePirat]

What version of Go are you using (go version)?

$ go version
go version go1.14.1 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/epirat/Library/Caches/go-build"
GOENV="/Users/epirat/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/epirat/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/opt/local/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/opt/local/lib/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="/usr/bin/clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/bh/j6gp_mw13pg06g1tt7kl6yvc0000gn/T/go-build013714662=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

The html path escaper does not work properly, so paths are not properly escaped:

t, _ := template.New("foo").Parse(`
	{{define "T"}}<a href="/genres/{{ . }}">Example</a>{{end}}
`)
t.ExecuteTemplate(out, "T", "Example/Genre")

Playground

This snippet should do url.PathEscape on the variable used in the attribute,
but it does some variation that does not properly escapes the / character,
leading to incorrect URLs.

What did you expect to see?

This snippet should do url.PathEscape on the variable used in the attributes path,
resulting in Example%2FGenre.

What did you see instead?

it does some variation of a PathEscape, which does not properly escapes the / character,
leading to incorrect URLs:

<a href="/genres/Example/Genre">Example</a>

There is no easy workaround either I could find, that would lead to it correctly escaping this, as per the documentation it seems it assumes manual escaping is a security issue and should never be done? I could not find any function to escape the variable correctly in my template.

While I can wrap it in a different type to prevent escaping, there is no way to enforce escaping, except giving up on the html templating and do text templating instead which is a lot more dangerous as I would need to take care of all escaping myself. Maybe I am missing something obvious here? If so, clearly the documentation is lacking.

[ePirat]

It seems what I am observing should only be happening if I would do:

t.ExecuteTemplate(out, "T", template.URL("Example/Genre"))

[dmitshur]

/cc @mikesamuel @empijei per owners.

[empijei]

Thanks for reporting this.

The template package has in its threat model for URL only to make it impossible to inject exotic protocols like javascript:.

The escaping you are pointing out in your example is more of a semantic escaping that has hardly something to do with XSS: you are correct in stating that the path could be escaped by the package, but I would argue that this should not be done.

URLs can be specified by user code to have dynamic paths safely and this is a feature of the contextual autoescaper. Patterns like <img src="/static/{{.imgpath}}"> should be expressible. Percent-encoding the paths would break functioning applications, would make this pattern hard to express without disabling contextual autoescaping and I am struggling to see the security benefit in it.

Could you please provide an example of an attack that would rely on this issue to cause an XSS (or other attacks)?

[icza]

I too just faced and was surprised about this "lack" of escaping path elements. IMO it should only skip escaping if template.URL is passed.

I ended up registering a custom pathEscape function being url.PathEscape, and I just use it in the template like this:

t := template.Must(template.New("").Funcs(template.FuncMap{
	"pathEscape": url.PathEscape,
}).Parse(`<a href="/genres/{{pathEscape .}}">Example</a>`))
if err := t.Execute(os.Stdout, "Example/Genre"); err != nil {
	panic(err)
}

This will properly output (try it on the Go Playground):

<a href="/genres/Example%2FGenre">Example</a>

[ePirat]

Hi, thanks a lot for the answer. It's not about an XSS attack that would be possible.

The problem is, illustrated using your example: <img src="/static/{{.imgpath}}"> is already possible to do by making imgpath a template.URL in Go code, which is specifically what template.URL seems to be made for according to the documentation.

What is impossible is to get the behavior where it percent-encodes it. So the current behavior is redundant and makes template.URL kind of pointless. There is no supported way I can find to get percent-encoding to work. So I had to resort to basically the same as @icza did to get it working which feels like a hack, given that the Template engine specifically does not want users to explicitly manually have to escape stuff.

[empijei]

This package tries to protect its user from potential attacks. I don't understand what is the threat you are surfacing.

If there is no threat, is your request to make it possible to percent-escape urls?
If so this is already possible by escaping the URL yourself and promoting it to a template.URL or by doing what icza suggested.

Adding automatic escaping would break a lot of already existing code for a benefit that I fail to see and if we were to change the API as you suggest it would become quite hard to obtain the current behavior without introducing vulnerabilities.

[ePirat]

Adding automatic escaping would break a lot of already existing code for a benefit that I fail to see and if we were to change the API as you suggest it would become quite hard to obtain the current behavior without introducing vulnerabilities.

As you mentioned it is impossible to change the behavior of the package here as it would break existing use-cases, so I'll close the issue as it's impossible to do anything about it.

Maybe a good idea to prevent any future confusion on this would be to show an example of this case in the documentation to make it clearer that no path escaping is happening.
What's your opinion on that?

[empijei]

Do you have an idea on a precise change you'd apply to the documentation?

I am generally in favor on disambiguation in the doc, but at the same time I don't want the doc to need a TL;DR.

Let's keep this open and just change the title to reflect that this is now a discussion on doc changes.