]> Cypherpunks.ru repositories - gostls13.git/commitdiff
[release-branch.go1.20] html/template: disallow actions in JS template literals
authorRoland Shoemaker <bracewell@google.com>
Mon, 20 Mar 2023 18:01:13 +0000 (11:01 -0700)
committerGopher Robot <gobot@golang.org>
Tue, 4 Apr 2023 16:59:18 +0000 (16:59 +0000)
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>
src/html/template/context.go
src/html/template/error.go
src/html/template/escape.go
src/html/template/escape_test.go
src/html/template/js.go
src/html/template/js_test.go
src/html/template/jsctx_string.go
src/html/template/state_string.go
src/html/template/transition.go

index a97c8be56f958e4894e0dc1d16efb8a16f745e89..c28fb0c5ea8ef30ddbcb4ffec6d0bef34756ad76 100644 (file)
@@ -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 */.
index 5c51f772cbde36f4b08e217d94def0f21a13e896..d7d6f5b3ab589ea1e44f235ba7e2f2716906a8a4 100644 (file)
@@ -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:
+       //     <script>var tmpl = `{{.Interp}`</script>
+       // 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 {
index 54fbcdca333549cedae379361a3e9ac84597bed7..23ece7a72fbacc9e62ea272780f976dda7696417 100644 (file)
@@ -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,15 @@ 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" {
+                       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:
index 12add077c3c38dff247756d4e68e1806cec79b56..3dd212bac94061e87de155269341d9e63a703b6c 100644 (file)
@@ -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}}<a{{if .X}}{{end}}>{{if .X}}{{break}}{{end}}{{end}}",
                        "",
                },
+               {
+                       "<script>var a = `${a+b}`</script>`",
+                       "",
+               },
                // Error cases.
                {
                        "{{if .Cond}}<a{{end}}",
@@ -1082,6 +1082,10 @@ func TestErrors(t *testing.T) {
                        // html is allowed since it is the last command in the pipeline, but urlquery is not.
                        `predefined escaper "urlquery" disallowed in template`,
                },
+               {
+                       "<script>var tmpl = `asd {{.}}`;</script>",
+                       `{{.}} appears in a JS template literal`,
+               },
        }
        for _, test := range tests {
                buf := new(bytes.Buffer)
@@ -1303,6 +1307,10 @@ func TestEscapeText(t *testing.T) {
                        `<a onclick="'foo&quot;`,
                        context{state: stateJSSqStr, delim: delimDoubleQuote, attr: attrScript},
                },
+               {
+                       "<a onclick=\"`foo",
+                       context{state: stateJSBqStr, delim: delimDoubleQuote, attr: attrScript},
+               },
                {
                        `<A ONCLICK="'`,
                        context{state: stateJSSqStr, delim: delimDoubleQuote, attr: attrScript},
index 50523d00f167fd5686ca9ef2ae05d5a88b3d79ec..fe7054efe5cd8c51e760013a128ccff75249d809 100644 (file)
@@ -308,6 +308,7 @@ var jsStrReplacementTable = []string{
        // Encode HTML specials as hex so the output can be embedded
        // in HTML attributes without further encoding.
        '"':  `\u0022`,
+       '`':  `\u0060`,
        '&':  `\u0026`,
        '\'': `\u0027`,
        '+':  `\u002b`,
@@ -331,6 +332,7 @@ var jsStrNormReplacementTable = []string{
        '"':  `\u0022`,
        '&':  `\u0026`,
        '\'': `\u0027`,
+       '`':  `\u0060`,
        '+':  `\u002b`,
        '/':  `\/`,
        '<':  `\u003c`,
index 580cb0a12da0645ff93adbb35b32c8c12f092221..eee7eb2bef7874bbfd849ead452f6d1148c1a14e 100644 (file)
@@ -291,7 +291,7 @@ func TestEscapersOnLower7AndSelectHighCodepoints(t *testing.T) {
                                `0123456789:;\u003c=\u003e?` +
                                `@ABCDEFGHIJKLMNO` +
                                `PQRSTUVWXYZ[\\]^_` +
-                               "`abcdefghijklmno" +
+                               "\\u0060abcdefghijklmno" +
                                "pqrstuvwxyz{|}~\u007f" +
                                "\u00A0\u0100\\u2028\\u2029\ufeff\U0001D11E",
                },
index dd1d87ee454856a14f218c523d2aeda354aba8c6..23948934c950a10fb9e3533ac58f63eeb147bea0 100644 (file)
@@ -4,6 +4,15 @@ package template
 
 import "strconv"
 
+func _() {
+       // An "invalid array index" compiler error signifies that the constant values have changed.
+       // Re-run the stringer command to generate them again.
+       var x [1]struct{}
+       _ = x[jsCtxRegexp-0]
+       _ = x[jsCtxDivOp-1]
+       _ = x[jsCtxUnknown-2]
+}
+
 const _jsCtx_name = "jsCtxRegexpjsCtxDivOpjsCtxUnknown"
 
 var _jsCtx_index = [...]uint8{0, 11, 21, 33}
index 05104be89c25459c6d704722f0e66c9bc5114a38..6fb1a6eeb0e746a4b2257bd6623ab96f28dc7f84 100644 (file)
@@ -4,9 +4,42 @@ package template
 
 import "strconv"
 
-const _state_name = "stateTextstateTagstateAttrNamestateAfterNamestateBeforeValuestateHTMLCmtstateRCDATAstateAttrstateURLstateSrcsetstateJSstateJSDqStrstateJSSqStrstateJSRegexpstateJSBlockCmtstateJSLineCmtstateCSSstateCSSDqStrstateCSSSqStrstateCSSDqURLstateCSSSqURLstateCSSURLstateCSSBlockCmtstateCSSLineCmtstateError"
+func _() {
+       // An "invalid array index" compiler error signifies that the constant values have changed.
+       // Re-run the stringer command to generate them again.
+       var x [1]struct{}
+       _ = x[stateText-0]
+       _ = x[stateTag-1]
+       _ = x[stateAttrName-2]
+       _ = x[stateAfterName-3]
+       _ = x[stateBeforeValue-4]
+       _ = x[stateHTMLCmt-5]
+       _ = x[stateRCDATA-6]
+       _ = x[stateAttr-7]
+       _ = x[stateURL-8]
+       _ = x[stateSrcset-9]
+       _ = x[stateJS-10]
+       _ = x[stateJSDqStr-11]
+       _ = x[stateJSSqStr-12]
+       _ = x[stateJSBqStr-13]
+       _ = x[stateJSRegexp-14]
+       _ = x[stateJSBlockCmt-15]
+       _ = x[stateJSLineCmt-16]
+       _ = x[stateCSS-17]
+       _ = x[stateCSSDqStr-18]
+       _ = x[stateCSSSqStr-19]
+       _ = x[stateCSSDqURL-20]
+       _ = x[stateCSSSqURL-21]
+       _ = x[stateCSSURL-22]
+       _ = x[stateCSSBlockCmt-23]
+       _ = x[stateCSSLineCmt-24]
+       _ = x[stateError-25]
+       _ = x[stateDead-26]
+}
+
+const _state_name = "stateTextstateTagstateAttrNamestateAfterNamestateBeforeValuestateHTMLCmtstateRCDATAstateAttrstateURLstateSrcsetstateJSstateJSDqStrstateJSSqStrstateJSBqStrstateJSRegexpstateJSBlockCmtstateJSLineCmtstateCSSstateCSSDqStrstateCSSSqStrstateCSSDqURLstateCSSSqURLstateCSSURLstateCSSBlockCmtstateCSSLineCmtstateErrorstateDead"
 
-var _state_index = [...]uint16{0, 9, 17, 30, 44, 60, 72, 83, 92, 100, 111, 118, 130, 142, 155, 170, 184, 192, 205, 218, 231, 244, 255, 271, 286, 296}
+var _state_index = [...]uint16{0, 9, 17, 30, 44, 60, 72, 83, 92, 100, 111, 118, 130, 142, 154, 167, 182, 196, 204, 217, 230, 243, 256, 267, 283, 298, 308, 317}
 
 func (i state) String() string {
        if i >= state(len(_state_index)-1) {
index 06df679330db2c49d1bfc7d2369f2bc2a40f164a..92eb3519063c9ea0fd17eb8344939ab4871d8ba8 100644 (file)
@@ -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 = `\/[]`
        }