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
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@stjj89
Copy link
Contributor

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

stjj89 commented Mar 23, 2017

@mikesamuel

@bradfitz bradfitz added this to the Go1.9Maybe milestone Mar 23, 2017
@mikesamuel
Copy link
Contributor

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

stjj89 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 added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jul 20, 2017
@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
Copy link
Contributor

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

@stjj89
Copy link
Contributor Author

stjj89 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 ianlancetaylor modified the milestones: Go1.12, Unplanned Jun 29, 2018
@ianlancetaylor ianlancetaylor added help wanted NeedsFix The path to resolution is known, but the work has not been done. and removed Security labels Jun 29, 2018
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jun 29, 2018
@ianlancetaylor
Copy link
Contributor

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants