From ecc5ba46117d2e73f65caac27dfe53acb12cf4f3 Mon Sep 17 00:00:00 2001 From: Roland Shoemaker Date: Mon, 20 Mar 2023 11:01:13 -0700 Subject: [PATCH] html/template: disallow actions in JS template 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 Fixes #59234 [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 Run-TryBot: Damien Neil Reviewed-by: Julie Qiu Reviewed-by: Roland Shoemaker Change-Id: Ia221fefdb273bd0f066dffc2abcf2a616801d2f2 Reviewed-on: https://go-review.googlesource.com/c/go/+/482079 TryBot-Bypass: Michael Knyszek Run-TryBot: Michael Knyszek Reviewed-by: Matthew Dempsky Auto-Submit: Michael Knyszek --- src/html/template/context.go | 2 + src/html/template/error.go | 13 ++++++ src/html/template/escape.go | 13 ++++++ src/html/template/escape_test.go | 66 +++++++++++++++++------------- src/html/template/js.go | 2 + src/html/template/js_test.go | 2 +- src/html/template/jsctx_string.go | 9 ++++ src/html/template/state_string.go | 37 ++++++++++++++++- src/html/template/transition.go | 7 +++- src/runtime/metrics.go | 1 + src/runtime/metrics/description.go | 7 ++++ src/runtime/metrics/doc.go | 5 +++ 12 files changed, 131 insertions(+), 33 deletions(-) diff --git a/src/html/template/context.go b/src/html/template/context.go index a97c8be56f958e..c28fb0c5ea8ef3 100644 --- a/src/html/template/context.go +++ b/src/html/template/context.go @@ -120,6 +120,8 @@ const ( stateJSDqStr // stateJSSqStr occurs inside a JavaScript single quoted string. stateJSSqStr + // stateJSBqStr occurs inside a JavaScript back quoted string. + stateJSBqStr // stateJSRegexp occurs inside a JavaScript regexp literal. stateJSRegexp // stateJSBlockCmt occurs inside a JavaScript /* block comment */. diff --git a/src/html/template/error.go b/src/html/template/error.go index 5c51f772cbde36..d7d6f5b3ab589e 100644 --- a/src/html/template/error.go +++ b/src/html/template/error.go @@ -214,6 +214,19 @@ const ( // pipeline occurs in an unquoted attribute value context, "html" is // disallowed. Avoid using "html" and "urlquery" entirely in new templates. ErrPredefinedEscaper + + // errJSTmplLit: "... appears in a JS template literal" + // Example: + // + // Discussion: + // Package html/template does not support actions inside of JS template + // literals. + // + // TODO(rolandshoemaker): we cannot add this as an exported error in a minor + // release, since it is backwards incompatible with the other minor + // releases. As such we need to leave it unexported, and then we'll add it + // in the next major release. + errJSTmplLit ) func (e *Error) Error() string { diff --git a/src/html/template/escape.go b/src/html/template/escape.go index 54fbcdca333549..c20e6b0c342132 100644 --- a/src/html/template/escape.go +++ b/src/html/template/escape.go @@ -8,6 +8,7 @@ import ( "bytes" "fmt" "html" + "internal/godebug" "io" "text/template" "text/template/parse" @@ -160,6 +161,8 @@ func (e *escaper) escape(c context, n parse.Node) context { panic("escaping " + n.String() + " is unimplemented") } +var debugAllowActionJSTmpl = godebug.New("jstmpllitinterp") + // escapeAction escapes an action template node. func (e *escaper) escapeAction(c context, n *parse.ActionNode) context { if len(n.Pipe.Decl) != 0 { @@ -223,6 +226,16 @@ func (e *escaper) escapeAction(c context, n *parse.ActionNode) context { c.jsCtx = jsCtxDivOp case stateJSDqStr, stateJSSqStr: s = append(s, "_html_template_jsstrescaper") + case stateJSBqStr: + if debugAllowActionJSTmpl.Value() == "1" { + debugAllowActionJSTmpl.IncNonDefault() + s = append(s, "_html_template_jsstrescaper") + } else { + return context{ + state: stateError, + err: errorf(errJSTmplLit, n, n.Line, "%s appears in a JS template literal", n), + } + } case stateJSRegexp: s = append(s, "_html_template_jsregexpescaper") case stateCSS: diff --git a/src/html/template/escape_test.go b/src/html/template/escape_test.go index 12add077c3c38d..3dd212bac94061 100644 --- a/src/html/template/escape_test.go +++ b/src/html/template/escape_test.go @@ -681,35 +681,31 @@ func TestEscape(t *testing.T) { } for _, test := range tests { - tmpl := New(test.name) - tmpl = Must(tmpl.Parse(test.input)) - // Check for bug 6459: Tree field was not set in Parse. - if tmpl.Tree != tmpl.text.Tree { - t.Errorf("%s: tree not set properly", test.name) - continue - } - b := new(strings.Builder) - if err := tmpl.Execute(b, data); err != nil { - t.Errorf("%s: template execution failed: %s", test.name, err) - continue - } - if w, g := test.output, b.String(); w != g { - t.Errorf("%s: escaped output: want\n\t%q\ngot\n\t%q", test.name, w, g) - continue - } - b.Reset() - if err := tmpl.Execute(b, pdata); err != nil { - t.Errorf("%s: template execution failed for pointer: %s", test.name, err) - continue - } - if w, g := test.output, b.String(); w != g { - t.Errorf("%s: escaped output for pointer: want\n\t%q\ngot\n\t%q", test.name, w, g) - continue - } - if tmpl.Tree != tmpl.text.Tree { - t.Errorf("%s: tree mismatch", test.name) - continue - } + t.Run(test.name, func(t *testing.T) { + tmpl := New(test.name) + tmpl = Must(tmpl.Parse(test.input)) + // Check for bug 6459: Tree field was not set in Parse. + if tmpl.Tree != tmpl.text.Tree { + t.Fatalf("%s: tree not set properly", test.name) + } + b := new(strings.Builder) + if err := tmpl.Execute(b, data); err != nil { + t.Fatalf("%s: template execution failed: %s", test.name, err) + } + if w, g := test.output, b.String(); w != g { + t.Fatalf("%s: escaped output: want\n\t%q\ngot\n\t%q", test.name, w, g) + } + b.Reset() + if err := tmpl.Execute(b, pdata); err != nil { + t.Fatalf("%s: template execution failed for pointer: %s", test.name, err) + } + if w, g := test.output, b.String(); w != g { + t.Fatalf("%s: escaped output for pointer: want\n\t%q\ngot\n\t%q", test.name, w, g) + } + if tmpl.Tree != tmpl.text.Tree { + t.Fatalf("%s: tree mismatch", test.name) + } + }) } } @@ -936,6 +932,10 @@ func TestErrors(t *testing.T) { "{{range .Items}}{{if .X}}{{break}}{{end}}{{end}}", "", }, + { + "`", + "", + }, // Error cases. { "{{if .Cond}}var tmpl = `asd {{.}}`;", + `{{.}} appears in a JS template literal`, + }, } for _, test := range tests { buf := new(bytes.Buffer) @@ -1303,6 +1307,10 @@ func TestEscapeText(t *testing.T) { `= state(len(_state_index)-1) { diff --git a/src/html/template/transition.go b/src/html/template/transition.go index 06df679330db2c..92eb3519063c9e 100644 --- a/src/html/template/transition.go +++ b/src/html/template/transition.go @@ -27,6 +27,7 @@ var transitionFunc = [...]func(context, []byte) (context, int){ stateJS: tJS, stateJSDqStr: tJSDelimited, stateJSSqStr: tJSDelimited, + stateJSBqStr: tJSDelimited, stateJSRegexp: tJSDelimited, stateJSBlockCmt: tBlockCmt, stateJSLineCmt: tLineCmt, @@ -262,7 +263,7 @@ func tURL(c context, s []byte) (context, int) { // tJS is the context transition function for the JS state. func tJS(c context, s []byte) (context, int) { - i := bytes.IndexAny(s, `"'/`) + i := bytes.IndexAny(s, "\"`'/") if i == -1 { // Entire input is non string, comment, regexp tokens. c.jsCtx = nextJSCtx(s, c.jsCtx) @@ -274,6 +275,8 @@ func tJS(c context, s []byte) (context, int) { c.state, c.jsCtx = stateJSDqStr, jsCtxRegexp case '\'': c.state, c.jsCtx = stateJSSqStr, jsCtxRegexp + case '`': + c.state, c.jsCtx = stateJSBqStr, jsCtxRegexp case '/': switch { case i+1 < len(s) && s[i+1] == '/': @@ -303,6 +306,8 @@ func tJSDelimited(c context, s []byte) (context, int) { switch c.state { case stateJSSqStr: specials = `\'` + case stateJSBqStr: + specials = "`\\" case stateJSRegexp: specials = `\/[]` } diff --git a/src/runtime/metrics.go b/src/runtime/metrics.go index 55e3d68c2b45ae..d154acce713cb2 100644 --- a/src/runtime/metrics.go +++ b/src/runtime/metrics.go @@ -290,6 +290,7 @@ func initMetrics() { "/godebug/non-default-behavior/http2client:events": {compute: compute0}, "/godebug/non-default-behavior/http2server:events": {compute: compute0}, "/godebug/non-default-behavior/installgoroot:events": {compute: compute0}, + "/godebug/non-default-behavior/jstmpllitinterp:events": {compute: compute0}, "/godebug/non-default-behavior/multipartfiles:events": {compute: compute0}, "/godebug/non-default-behavior/multipartmaxheaders:events": {compute: compute0}, "/godebug/non-default-behavior/multipartmaxparts:events": {compute: compute0}, diff --git a/src/runtime/metrics/description.go b/src/runtime/metrics/description.go index 65288dea614546..a06c017b7f90f3 100644 --- a/src/runtime/metrics/description.go +++ b/src/runtime/metrics/description.go @@ -305,6 +305,13 @@ var allDesc = []Description{ Kind: KindUint64, Cumulative: true, }, + { + Name: "/godebug/non-default-behavior/jstmpllitinterp:events", + Description: "The number of non-default behaviors executed by the html/template" + + "package due to a non-default GODEBUG=jstmpllitinterp=... setting.", + Kind: KindUint64, + Cumulative: true, + }, { Name: "/godebug/non-default-behavior/multipartfiles:events", Description: "The number of non-default behaviors executed by the mime/multipart package " + diff --git a/src/runtime/metrics/doc.go b/src/runtime/metrics/doc.go index 4cf528746b9694..68bbf5a3cea165 100644 --- a/src/runtime/metrics/doc.go +++ b/src/runtime/metrics/doc.go @@ -219,6 +219,11 @@ Below is the full list of supported metrics, ordered lexicographically. The number of non-default behaviors executed by the go/build package due to a non-default GODEBUG=installgoroot=... setting. + /godebug/non-default-behavior/jstmpllitinterp:events + The number of non-default behaviors executed by + the html/templatepackage due to a non-default + GODEBUG=jstmpllitinterp=... setting. + /godebug/non-default-behavior/multipartfiles:events The number of non-default behaviors executed by the mime/multipart package due to a non-default