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: dynamic substrings in HTML tags or attributes can result in unsafe HTML output #19669

Open
stjj89 opened this issue Mar 23, 2017 · 6 comments

Comments

@stjj89
Copy link
Contributor

commented Mar 23, 2017

The following template:

<s{{.X}}>alert('pwned')</script>

produces the following HTML output when executed with X = "cript":

<script>alert('pwned')</script>

This happens because:

  • During HTML parsing/escaping time, the parser interprets "s" as the tag name, since the rest of the tagname will only be evaluated at execution-time. This causes the escaper to transition into the plain-text state, rather than the JS state, inside the script element body.
  • During execution time, the htmlNameFilter inserted into {{.X}} (i.e. {{.X | _html_template_htmlnamefilter) sees the text value "cript", deems that it is a safe HTML tag/attribute name, and renders it as-is.

In general, allowing dynamic substrings in HTML tags or attributes may confuse the parser and escaper, since the static and dynamic parts of the name are handled in different phases.

Suggested solution: disallow dynamic substrings in HTML tags or attributes completely.

@stjj89

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2017

@bradfitz bradfitz added the Security label Mar 23, 2017
@bradfitz bradfitz added this to the Go1.9Maybe milestone Mar 23, 2017
@mikesamuel

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2017

The idea initially was to allow switching between a few equivalence sets

  • h{1,2,3,4,5,6}
  • t{head,body,foot}
  • td, th

so it might be worth not including the feature in strict mode or limiting it somehow.

I think

<s{{.}}

is high impact but low frequency if we're correct about our design assumption that template authors are acting in good-faith, so this is definitely bug, but not super high priority.

@stjj89

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2017

so it might be worth not including the feature in strict mode or limiting it somehow.

I definitely plan to disallow this feature in my wrapper package.

is high impact but low frequency if we're correct about our design assumption that template authors are acting in good-faith, so this is definitely bug, but not super high priority.

I agree that this bug is unlikely to occur if we assuming benign developers. Do you have a sense of how we should go about fixing this? The only two solutions I see are either disallowing this feature completely, or adding another escaping pass.

@bradfitz bradfitz modified the milestones: Go1.10, Go1.9Maybe Jul 20, 2017
@rsc rsc modified the milestones: Go1.10, Go1.11 Dec 4, 2017
@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Go1.12 Jun 29, 2018
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2018

ping @stjj89 @mikesamuel What is the status of this? Thanks.

@stjj89

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2018

As per Mike's comment, the probability of this being an issue might be low enough that it is not worth fixing preemptively. We can probably shelve this and revisit it in the future if it proves to be an issue.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2018

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.