Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

html/template: Paths in attributes not escaped correctly #38837

Closed
ePirat opened this issue May 3, 2020 · 8 comments
Closed

html/template: Paths in attributes not escaped correctly #38837

ePirat opened this issue May 3, 2020 · 8 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@ePirat
Copy link

ePirat commented May 3, 2020

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
Copy link
Author

ePirat commented May 3, 2020

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

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

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 8, 2020
@dmitshur dmitshur added this to the Backlog milestone May 8, 2020
@dmitshur
Copy link
Contributor

dmitshur commented May 8, 2020

/cc @mikesamuel @empijei per owners.

@empijei
Copy link
Contributor

empijei commented May 18, 2020

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
Copy link

icza commented May 18, 2020

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
Copy link
Author

ePirat commented May 18, 2020

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
Copy link
Contributor

empijei commented Jun 3, 2020

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
Copy link
Author

ePirat commented Jun 3, 2020

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?

@ePirat ePirat closed this as completed Jun 3, 2020
@empijei
Copy link
Contributor

empijei commented Jun 3, 2020

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.

@golang golang locked and limited conversation to collaborators Jun 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants