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: inserted escapers are not properly merged with predefined escapers with explicit arguments #19353

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

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

When we have

.X = `haha onclick=evil()`,

the template

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

is rewritten into

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

after contextual auto-escaping, which evaluates to

<a href=haha%20onclick=evil%28%29>

However, the template

<a href={{html .X}}>

is rewritten into

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

after contextual auto-escaping, which evaluates to

<a href=haha onclick=evil()>

which is incorrect, since the URL escapers will be called with no arguments, and will thus have no effect on the user-supplied value .X. This happens because of the html/template escaper's attempt to merge inserted escapers with user-supplied escapers, described in #19336.

@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
gopherbot pushed a commit that referenced this issue Apr 10, 2017
…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 #19336, #19345, #19352,
and #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 #19336, #19345, #19352, #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
Copy link
Contributor Author

stjj89 commented Apr 10, 2017

This is no longer an issue now that golang.org/cl/37880 has landed.

@stjj89 stjj89 closed this as completed Apr 10, 2017
lparth pushed a commit to lparth/go that referenced this issue Apr 13, 2017
…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>
@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue May 5, 2017
Allow the predefined escapers "html", "urlquery", and "js" to be used
in pipelines when they have no potential to affect the correctness or
safety of the escaped pipeline output. Specifically:
- "urlquery" may be used if it is the last command in the pipeline.
- "html" may be used if it is the last command in the pipeline, and
  the pipeline does not occur in an unquoted HTML attribute value
  context.
- "js" may be used in any pipeline, since it does not affect the
  merging of contextual escapers.

This change will loosens the restrictions on predefined escapers
introduced in golang.org/cl/37880, which will hopefully ease the
upgrade path for existing template users.

This change brings back the escaper-merging logic, and associated
unit tests, that were removed in golang.org/cl/37880. However, a
few notable changes have been made:
- "_html_template_nospaceescaper" is no longer considered
  equivalent to "html", since the former escapes spaces, while
  the latter does not (see #19345). This change should not silently
  break any templates, since pipelines where this substituion will
  happen will already trigger an explicit error.
- An "_eval_args_" internal directive has been added to
  handle pipelines containing a single explicit call to a
  predefined escaper, e.g. {{html .X}} (see #19353).

Also, the HTMLEscape function called by the predefined
text/template "html" function now escapes the NULL character as
well. This effectively makes it as secure as the internal
html/template HTML escapers (see #19345). While this change is
backward-incompatible, it will only affect illegitimate uses
of this escaper, since the NULL character is always illegal in
valid HTML.

Fixes #19952

Change-Id: I9b5570a80a3ea284b53901e6a1f842fc59b33d3a
Reviewed-on: https://go-review.googlesource.com/40936
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Apr 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants