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.

Sign up for GitHub

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: predefined HTML escaper does not properly escape user input #19345

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

html/template: predefined HTML escaper does not properly escape user input #19345

stjj89 opened this issue Mar 1, 2017 · 7 comments
Labels
FrozenDueToAge
Milestone
Go1.9Maybe

Comments

@stjj89
Copy link
Contributor

stjj89 commented Mar 1, 2017 •
edited
Loading

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 following template

<a title={{.X}}>

evaluates to

<a title=haha&#32;onclick&#61;evil()>

which is properly escaped. However, the following template:

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

evaluates to

<a title=haha onclick=evil()>

which is improperly escaped, and insecure.

This is happening because the html/template escaper considers the predefined global html function equivalent to its internal _html_template_nospaceescaper escaping directive. Since the former is already present in the pipeline, it does not insert the latter to prevent over-escaping (see original commit of this behavior here). However, the above example shows that these two escaping functions are not equivalent.

We should thoroughly re-evaluate the equivalency of predefined global escapers to internal escapers, or get rid of this logic entirely. If we go with the latter option, we could consider removing all user-supplied escapers before adding internal escaper directives, in order to prevent over-escaping.

See related issue 19336 for another security issue related to this equivalent-escapers logic.
@mikesamuel
Copy link
Contributor

mikesamuel commented Mar 1, 2017 •
edited
Loading

nice catch!

We should thoroughly re-evaluate the equivalency of predefined global escapers
to internal escapers

or get rid of this logic entirely. If we go with the latter option, we could consider
removing all user-supplied escapers before adding internal escaper directives,
in order to prevent over-escaping.

So

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

would become

<a title={{.X | htmlAttrNospace}}>

it seems that removing equivalents and upgrading sounds fine.

I'm a little leery of this in one instance

<iframe srcdoc="{{.X | html | htmlAttr}}">

is semantically quite different from

<iframe srcdoc="{{.X | htmlAttr}}">

but perhaps that's just a weakness in how we handle

<iframe srcdoc="{{.X}}">

@stjj89
Copy link
Contributor Author

stjj89 commented Mar 1, 2017

I'm a little leery of this in one instance

<iframe srcdoc="{{.X | html | htmlAttr}}">

is semantically quite different from

<iframe srcdoc="{{.X | htmlAttr}}">

If I understand correctly, and htmlAttr refers to the internal _html_template_attrescaper directive, then the scenario you highlighted won't ever occur. _html_template_attrescaper is hidden from the user, so the user can only call html. In my proposed solution, both

<iframe srcdoc="{{.X | html}}">

and

<iframe srcdoc="{{.X}}">

will be rewritten to

<iframe srcdoc="{{.X | _html_template_attrescaper}}">

where in the former case, we remove the html directive before adding _html_template_attrescaper.

@gopherbot
Copy link
Contributor

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

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

To @rsc for review.

gopherbot pushed a commit that referenced this issue Apr 10, 2017
@rsc
html/template: panic if predefined escapers are found in pipelines du…
9ffd933 
…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 •
edited
Loading

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

@stjj89 stjj89 closed this as completed Apr 10, 2017
@stjj89
Copy link
Contributor Author

stjj89 commented Apr 13, 2017 •
edited
Loading

I took a closer look at this, and determined that html and _html_template_nospaceescaper are unsafe to use interchangeably, since the former function does not escape whitespace, which is significant in the unquoted-attribute-value context that it is used in.

The rest of the functions in the equivEscapers should be safe to use interchangeably.

@stjj89 stjj89 mentioned this issue Apr 13, 2017
Closed
@gopherbot
Copy link
Contributor

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

gopherbot pushed a commit that referenced this issue May 5, 2017
@bradfitz
html/template: allow safe usage of predefined escapers in pipelines
3a2fee0 
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
@rsc rsc removed their assignment Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.9Maybe
Development

No branches or pull requests
5 participants
@bradfitz @rsc @mikesamuel @stjj89 @gopherbot