html/template: XSS risk with external JSON

[neelance]

Go version: go version go1.6 darwin/amd64

The following program highlights an XSS risk when injecting externally obtained JSON via template.JS:

package main

import (
    "html/template"
    "os"
)

func main() {
    validExternalJSON := `{field: "</script><script>alert('You have been pwned!');</script><script>"}`

    t := template.Must(template.New("").Parse(`<script>var x = {{.}}; alert(x.field);</script>`))
    if err := t.Execute(os.Stdout, template.JS(validExternalJSON)); err != nil {
        panic(err)
    }
}

This is somewhat unexpected, since the documentation of template.JS says "JS encapsulates a known safe EcmaScript5 Expression", which intuitively any valid JSON should fulfill. The documentation should at least include a warning about this use case. Even better for avoiding this situation would be a template.JSON type which automatically gets sanitized via json.HTMLEscape before rendering.

[dominikh]

JS encapsulates a known safe EcmaScript5 Expression

Means that you have verified that it is safe, not that Go makes sure that it is safe.

[neelance]

Then "safe" should be defined properly. A JSON value is "JS code that is safe to run without causing any harm", but apparently not "safe to parse in an HTML context". I think right now this is a pitfall.

[neelance]

@OneOfOne Thanks for your opinion, but one reaction would have sufficed. Edit: Thx.

I still think that when it comes to security, one should err on the side of caution.

[valyala]

@neelance, it looks like this XSS may be fixed by substituting < char with \u003c inside JS strings like quicktemplate does for {%q } output tag.

[minux]

The document says "known safe" for a reason. (Known to the developer of the code/template to be safe, i.e. not controlled by the outside attacker.) In the validExternalJSON example, it's obviously not safe, so using template.JS is not appropriate. Because it's known safe, I don't think the template package should do any validation or transformation on the string.

[dmitshur]

I always read "known to be safe" as in known to be valid JavaScript, which does not purposefully execute anything malicious. I found the fact that "it cannot contain unescaped string sequences </script> or else unexpected things will happen" to be quite surprising.

From what I understand, I agree there is indeed a risk of unintended self-XSS and I maybe a fix is to clarify what is meant by "safe".

Prior to this, I would've thought the following is "safe" JavaScript:

validJS := template.JS(`var msg = "This string contains a harmless </script>, right?";`)

[neelance]

The purpose of the html/template package is to avoid human error. If all XSS situations would be "obviously not safe", then we could all just use text/template instead of html/template. But I think everyone agrees that this is not the case, that it is better to make the machine ensure safety than a human.

We had this XSS vulnerability in our product. I wasn't the author, but I have to admit that if I would have reviewed this code, I would not have spotted this vulnerability. Right now I would, but that is because now I know about the "string contents can be dangerous" fact. Still, I think this fact is not intuitive and thus has a high risk of human error if the same situation has not been experienced in the past. It would be in the spirit of the html/template package to reduce that risk, either by extending the safety that the machine can check or at least actively passing that knowledge via documentation.

[gopherbot]

CL https://golang.org/cl/22824 mentions this issue.

[adg]

@neelance said on the CL:

This improves the situation, but in my opinion it does not resolve #15399. The fallacy described in that issue is still quite likely: I can have a "trusted source" which always gives me valid JSON and not some bad JS. I could even see that it gets "included verbatim" in the output. Still, it is not trivially apparent to me that the >content of a string< inside of the JSON can break my neck.

To include JSON you should parse the JSON with json.Unmarshal and then pass the resultant object into the template, where it will be converted back to JSON when included in a JavaScript context.

By its very nature, template.JS is an escape hatch for people who understand the risks. And it's for that reason that I would never use it myself with any user-provided content: I don't fully understand the risks.

We could further document, in the case of template.JS, that a JSON object may contain strings that can escape the JS context, but that seems like belaboring the point to me. There are many, many ways that such escape hatches can be abused. Should we enumerate them all in the docs? Opinions?

[neelance]

I have seen this issue in production. I have a very high opinion of my coworker who wrote that code, still he did this mistake. A more inexperienced programmer might even be more likely to do this mistake. My goal is to find a way to make this more unlikely in the future and thus make this world's software a little bit more secure.

I know that strict checks are not in the spirit of Go. If you really want to shoot yourself in the foot, Go allows you to do so. Still, I think that one of the goals of Go and especially of the html/template package is to make it easier to write good and safe code. We do have slice boundary checks and we make it very explicit with unsafe to circumvent them. One may argue that template.JS is at the same level as unsafe, but to me it seems like one is quicker to use template.JS than to use unsafe.

Maybe a good solution here would be to mention the json.Unmarshal path in the docs instead of mentioning the individual attack vectors? I think people discard this option too quickly because it looks like wasted CPU cycles if you can also directly include the JSON.

[adg]

Maybe a good solution here would be to mention the json.Unmarshal path in the docs instead of mentioning the individual attack vectors? I think people discard this option too quickly because it looks like wasted CPU cycles if you can also directly include the JSON.

Where should this be mentioned? On the docs for template.JS? How would you word it?

[neelance]

English is not my first language, but maybe something like this:
"Using JS for including JSON has risks. A safe alternative is to parse the JSON with json.Unmarshal and then pass the resultant object into the template, where it will be converted back to JSON when included in a JavaScript context."

[adg]

Thanks. I have updated the CL. ​

[neelance]

Cool, thanks!