Navigation Menu

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

Jump to bottom

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

Closed
stjj89 opened this issue Mar 1, 2017 · 4 comments
Closed

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

stjj89 opened this issue Mar 1, 2017 · 4 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.9Maybe

Comments

@stjj89
Copy link
Contributor

stjj89 commented Mar 1, 2017

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

stjj89 commented Mar 1, 2017

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 stjj89 mentioned this issue Mar 1, 2017
Closed
@stjj89
Copy link
Contributor Author

stjj89 commented Mar 1, 2017

@mikesamuel

This was referenced Mar 1, 2017
Closed
Closed
@stjj89
Copy link
Contributor Author

stjj89 commented Mar 1, 2017

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.

@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 2, 2017
@gopherbot
Copy link

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

@bradfitz bradfitz added this to the Go1.9Maybe milestone Mar 21, 2017
lparth pushed a commit to lparth/go that referenced this issue Apr 13, 2017
@lparth
html/template: panic if predefined escapers are found in pipelines du…
4ecbb02 
…ring rewriting

Report an error if a predefined escaper (i.e. "html", "urlquery", or "js")
is found in a pipeline that will be rewritten by the contextual auto-escaper,
instead of trying to merge the escaper-inserted escaping directives
with these predefined escapers. This merging behavior is a source
of several security and correctness bugs (eee golang#19336, golang#19345, golang#19352,
and golang#19353.)

This merging logic was originally intended to ease migration of text/template
templates with user-defined escapers to html/template. Now that
migration is no longer an issue, this logic can be safely removed.

NOTE: this is a backward-incompatible change that fixes known security
bugs (see linked issues for more details). It will explicitly break users
that attempt to execute templates with pipelines containing predefined
escapers.

Fixes golang#19336, golang#19345, golang#19352, golang#19353

Change-Id: I46b0ca8a2809d179c13c0d4f42b63126ed1c3b49
Reviewed-on: https://go-review.googlesource.com/37880
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
@stjj89 stjj89 mentioned this issue Apr 13, 2017
Closed
@golang golang locked and limited conversation to collaborators Apr 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Go1.9Maybe
Development

No branches or pull requests
4 participants
@bradfitz @ALTree @stjj89 @gopherbot