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: backticks not treated as string delimiters (CVE-2023-24538) #59234

Closed
rolandshoemaker opened this issue Mar 24, 2023 · 14 comments
Closed
Assignees
Labels
NeedsFix The path to resolution is known, but the work has not been done. Security
Milestone

Comments

@rolandshoemaker
Copy link
Member

rolandshoemaker commented Mar 24, 2023

Templates did not properly consider backticks (`) as Javascript string delimiters, and as such did
not escape them as expected. Backticks are used, since ES6, for JS template literals. If a template
contained a Go template action within a Javascript template literal, the contents of the action could
be used to terminate the literal, injecting arbitrary Javascript code into the Go template.

As ES6 template literals are rather complex, and themselves can do string interpolation, we've decided
to simply disallow Go template actions from being used inside of them (e.g. "var a = {{.}}"), since
there is no obviously safe way to allow this behavior. This takes the same approach as github.com/google/safehtml.
Template.Parse will now return an Error when it encounters templates like this, with a currently unexported
ErrorCode with a value of 12. This ErrorCode will be exported in the next major release.

Users who rely on this behavior can re-enable it using the GODEBUG flag jstmpllitinterp=1, with the
caveat that backticks will now be escaped. This should be used with caution.

Thanks to Sohom Datta, Manipal Institute of Technology, for reporting this issue.

This is CVE-2023-24538 and Go issue https://go.dev/issue/59234.

/cc @golang/security and @golang/release

@rolandshoemaker rolandshoemaker self-assigned this Mar 24, 2023
@dmitshur dmitshur added this to the Go1.21 milestone Mar 24, 2023
@julieqiu
Copy link
Contributor

@gopherbot please open backport issues.

@gopherbot
Copy link

Backport issue(s) opened: #59271 (for 1.19), #59272 (for 1.20).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@julieqiu julieqiu added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 27, 2023
@gopherbot
Copy link

Change https://go.dev/cl/481981 mentions this issue: [release-branch.go1.19] html/template: disallow actions in JS template literals

@gopherbot
Copy link

Change https://go.dev/cl/481987 mentions this issue: [release-branch.go1.19] html/template: disallow actions in JS template literals

@gopherbot
Copy link

Change https://go.dev/cl/481993 mentions this issue: [release-branch.go1.20] html/template: disallow actions in JS template literals

@gopherbot
Copy link

Change https://go.dev/cl/482079 mentions this issue: html/template: disallow actions in JS template literals

gopherbot pushed a commit that referenced this issue Apr 4, 2023
…e literals

ECMAScript 6 introduced template literals[0][1] which are delimited with
backticks. These need to be escaped in a similar fashion to the
delimiters for other string literals. Additionally template literals can
contain special syntax for string interpolation.

There is no clear way to allow safe insertion of actions within JS
template literals, as handling (JS) string interpolation inside of these
literals is rather complex. As such we've chosen to simply disallow
template actions within these template literals.

A new error code is added for this parsing failure case, errJsTmplLit,
but it is unexported as it is not backwards compatible with other minor
release versions to introduce an API change in a minor release. We will
export this code in the next major release.

The previous behavior (with the cavet that backticks are now escaped
properly) can be re-enabled with GODEBUG=jstmpllitinterp=1.

This change subsumes CL471455.

Thanks to Sohom Datta, Manipal Institute of Technology, for reporting
this issue.

Fixes CVE-2023-24538
For #59234
Fixes #59271

[0] https://tc39.es/ecma262/multipage/ecmascript-language-expressions.html#sec-template-literals
[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals

Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1802457
Reviewed-by: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Julie Qiu <julieqiu@google.com>
Reviewed-by: Roland Shoemaker <bracewell@google.com>
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1802612
Run-TryBot: Roland Shoemaker <bracewell@google.com>
Change-Id: Ic7f10595615f2b2740d9c85ad7ef40dc0e78c04c
Reviewed-on: https://go-review.googlesource.com/c/go/+/481987
Auto-Submit: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
gopherbot pushed a commit that referenced this issue Apr 4, 2023
…e literals

ECMAScript 6 introduced template literals[0][1] which are delimited with
backticks. These need to be escaped in a similar fashion to the
delimiters for other string literals. Additionally template literals can
contain special syntax for string interpolation.

There is no clear way to allow safe insertion of actions within JS
template literals, as handling (JS) string interpolation inside of these
literals is rather complex. As such we've chosen to simply disallow
template actions within these template literals.

A new error code is added for this parsing failure case, errJsTmplLit,
but it is unexported as it is not backwards compatible with other minor
release versions to introduce an API change in a minor release. We will
export this code in the next major release.

The previous behavior (with the cavet that backticks are now escaped
properly) can be re-enabled with GODEBUG=jstmpllitinterp=1.

This change subsumes CL471455.

Thanks to Sohom Datta, Manipal Institute of Technology, for reporting
this issue.

Fixes CVE-2023-24538
For #59234
Fixes #59272

[0] https://tc39.es/ecma262/multipage/ecmascript-language-expressions.html#sec-template-literals
[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals

Change-Id: Idff74ec386e9b73d6e9a3c9f71990eabc0ce7506
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1802457
Reviewed-by: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Julie Qiu <julieqiu@google.com>
Reviewed-by: Roland Shoemaker <bracewell@google.com>
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1802688
Run-TryBot: Roland Shoemaker <bracewell@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/481993
Run-TryBot: Michael Knyszek <mknyszek@google.com>
Auto-Submit: Michael Knyszek <mknyszek@google.com>
TryBot-Bypass: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@mknyszek mknyszek changed the title security: fix CVE-2023-24538 html/template: backticks not treated as string delimiters (CVE-2023-24538) Apr 4, 2023
@hundt
Copy link

hundt commented Apr 4, 2023

I do not think the solution merged handles the case where a template literal contains a string interpolation with another template literal, as in:

package main

import (
	"bytes"
	"fmt"
	"html/template"
)

func main() {
	t := template.Must(template.New("test").Parse("<script>var v = `${function(){return `{{.V}}+1`}()}`;</script>"))
	buf := new(bytes.Buffer)
	err := t.Execute(buf, map[string]string{"V": "${alert(1)}"})
	if err != nil {
		panic(err)
	}
	fmt.Printf("%s", buf)
}

Go's simple JS state machine incorrectly thinks that "{{.V}}" appears outside of a JS template literal and escapes it as if it were the javascript context (by just adding quotes around it). As a result the output of this program is

<script>var v = `${function(){return `"${alert(1)}"+1`}()}`;</script>

which, if inserted as HTML in a web page, will run the alert function.

#9200 has a good discussion of the complexities of this issue.

@gopherbot
Copy link

Change https://go.dev/cl/482238 mentions this issue: html/template,mime/multipart: document new GODEBUG settings

@gopherbot
Copy link

Change https://go.dev/cl/482535 mentions this issue: [release-branch.go1.19] html/template,mime/multipart: document new GODEBUG settings

@gopherbot
Copy link

Change https://go.dev/cl/482555 mentions this issue: [release-branch.go1.20] html/template,mime/multipart: document new GODEBUG settings

gopherbot pushed a commit that referenced this issue Apr 5, 2023
This change documents the new GODEBUG settings introduced for
html/template and mime/multipart, released with Go 1.19.8 and Go 1.20.3
as part of a security fix.

Updates #59153.
Updates #59234.

Change-Id: I25f4d8245da3301dccccfb44da8ff1a5985392a4
Reviewed-on: https://go-review.googlesource.com/c/go/+/482238
Auto-Submit: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Apr 5, 2023
…DEBUG settings

This change documents the new GODEBUG settings introduced for
html/template and mime/multipart, released with Go 1.19.8 and Go 1.20.3
as part of a security fix.

Updates #59153.
For #59270.
Updates #59234.
For #59272.

Change-Id: I25f4d8245da3301dccccfb44da8ff1a5985392a4
Reviewed-on: https://go-review.googlesource.com/c/go/+/482555
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
gopherbot pushed a commit that referenced this issue Apr 5, 2023
…DEBUG settings

This change documents the new GODEBUG settings introduced for
html/template and mime/multipart, released with Go 1.19.8 and Go 1.20.3
as part of a security fix.

Updates #59153.
For #59269.
Updates #59234.
For #59271.

Change-Id: I25f4d8245da3301dccccfb44da8ff1a5985392a4
Reviewed-on: https://go-review.googlesource.com/c/go/+/482535
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
@karelbilek
Copy link

As @hundt mentioned, I don't think this should be closed, the vulnerability is still present

@atc0005
Copy link

atc0005 commented Apr 6, 2023

@julieqiu just making sure you saw the remarks by @hundt and @karelbilek.

See also:

@julieqiu
Copy link
Contributor

julieqiu commented Apr 6, 2023

Thank you for letting us know. We will investigate and post updates to #9200.

@gopherbot
Copy link

Change https://go.dev/cl/484075 mentions this issue: html/template: treat nested template literals properly

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

No branches or pull requests

7 participants