html/template: user-supplied escapers can undermine contextual auto-escaping

[stjj89]

version: go version devel +4cce27a3fa Sat Jan 21 03:20:55 2017 +0000 linux/amd64
env: linux amd64

html/template rewrites pipelines and adds calls to the escaping functions necessary to correctly and safely embed the output of these pipelines in their HTML contexts. For example, the following pipeline:

<a href={{ . | formatAsLink }}>

is rewritten as

<a href={{ . | formatAsLink | _html_template_urlfilter | _html_template_urlnormalizer | _html_template_nospaceescaper }}>

by the html/template escaper, where formatAsLink is the name of a user-defined function, and the rest of the _html_* names are names of internally-defined escapers.

However, if the pipeline already contains any of the predefined escaper functions (i.e. html or urlquery), and some of the escapers-to-be-inserted are considered equivalent, the html/template escaper will attempt to merge the inserted escapers with the existing predefined escapers, in order. For example,

<a href={{ . | html | formatAsLink }}>

is rewritten as

<a href={{ . | _html_template_urlfilter | _html_template_urlnormalizer | html | formatAsLink }}>

since _html_template_nospaceescaper and html are considered equivalent, and the escaper preserves the order of the escaping operations (i.e. URL escaping first, then HTML escaping). However, in this example, the user-defined formatAsLink function determines the final output of the pipeline. If formatAsLink contains a bug that causes it to emit an unsafe URL, the rendered HTML will be vulnerable to Cross-Site Scripting (XSS).

In general, users can shoot themselves in the foot if they manually escape their pipelines with html or urlquery because of this escaper-merging behavior. This behavior should either be explicitly documented or changed.

[stjj89]

One way to fix this behavior is to do away with the escaper-merging behavior, and delete existing escapers from the pipeline if they are equivalent to any of the inserted escapers. For example, the following pipeline:

<a href={{ . | html | formatAsLink }}>

should be rewritten as

<a href={{ . | formatAsLink | _html_template_urlfilter | _html_template_urlnormalizer | _html_template_nospaceescaper }}>

Since html is equivalent to _html_template_nospaceescaper, we remove it from the pipeline, since the latter function will be inserted at the end of the pipeline, and will perform the same escaping operation. This ensures that escapers will always appear at the end of rewritten pipelines, and therefore always escape the final pipeline output.

Caveat: This change will break existing html/template users who rely on this implied escaper-merging behavior. However, this security issue probably justifies this compatibility breakage. This change will only affect potentially insecure templates (i.e. templates whose rewritten pipelines do not end with the inserted escapers).

[stjj89]

@mikesamuel

[stjj89]

After finding several other bugs associated with this escaper-merging logic, I think the best and simplest way to solve this would be to remove all user-supplied escapers (i.e. urlquery and html) before automatically adding internal escaping directives. Do not trust the user to manually escape data with predefined escapers; always defer to the contextual auto-escaper's judgement. This might break some users, but those users are probably improperly escaping their template data anyway.

[gopherbot]

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