Skip to content

Commit

Permalink
[release-branch.go1.20] html/template: properly handle special tags w…
Browse files Browse the repository at this point in the history
…ithin the script context

The HTML specification has incredibly complex rules for how to handle
"<!--", "<script", and "</script" when they appear within literals in
the script context. Rather than attempting to apply these restrictions
(which require a significantly more complex state machine) we apply
the workaround suggested in section 4.12.1.3 of the HTML specification [1].

More precisely, when "<!--", "<script", and "</script" appear within
literals (strings and regular expressions, ignoring comments since we
already elide their content) we replace the "<" with "\x3C". This avoids
the unintuitive behavior that using these tags within literals can cause,
by simply preventing the rendered content from triggering it. This may
break some correct usages of these tags, but on balance is more likely
to prevent XSS attacks where users are unknowingly either closing or not
closing the script blocks where they think they are.

Thanks to Takeshi Kaneko (GMO Cybersecurity by Ierae, Inc.) for
reporting this issue.

Fixes #62197
Fixes #62397
Fixes CVE-2023-39319

[1] https://html.spec.whatwg.org/#restrictions-for-contents-of-script-elements

Change-Id: Iab57b0532694827e3eddf57a7497ba1fab1746dc
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1976594
Reviewed-by: Dmitri Shuralyov <[email protected]>
Reviewed-by: Tatiana Bradley <[email protected]>
Reviewed-by: Damien Neil <[email protected]>
Run-TryBot: Roland Shoemaker <[email protected]>
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/2014621
TryBot-Result: Security TryBots <[email protected]>
Reviewed-on: https://go-review.googlesource.com/c/go/+/526099
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Cherry Mui <[email protected]>
  • Loading branch information
rolandshoemaker authored and cherrymui committed Sep 6, 2023
1 parent 023b542 commit 2070531
Show file tree
Hide file tree
Showing 5 changed files with 104 additions and 4 deletions.
6 changes: 3 additions & 3 deletions src/go/build/deps_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -236,15 +236,15 @@ var depsRules = `
< text/template
< internal/lazytemplate;
encoding/json, html, text/template
< html/template;
# regexp
FMT
< regexp/syntax
< regexp
< internal/lazyregexp;
encoding/json, html, text/template, regexp
< html/template;
# suffix array
encoding/binary, regexp
< index/suffixarray;
Expand Down
14 changes: 14 additions & 0 deletions src/html/template/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,20 @@ func isInTag(s state) bool {
return false
}

// isInScriptLiteral returns true if s is one of the literal states within a
// <script> tag, and as such occurances of "<!--", "<script", and "</script"
// need to be treated specially.
func isInScriptLiteral(s state) bool {
// Ignore the comment states (stateJSBlockCmt, stateJSLineCmt,
// stateJSHTMLOpenCmt, stateJSHTMLCloseCmt) because their content is already
// omitted from the output.
switch s {
case stateJSDqStr, stateJSSqStr, stateJSBqStr, stateJSRegexp:
return true
}
return false
}

// delim is the delimiter that will end the current HTML attribute.
type delim uint8

Expand Down
26 changes: 26 additions & 0 deletions src/html/template/escape.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"html"
"internal/godebug"
"io"
"regexp"
"text/template"
"text/template/parse"
)
Expand Down Expand Up @@ -728,6 +729,26 @@ var delimEnds = [...]string{
delimSpaceOrTagEnd: " \t\n\f\r>",
}

var (
// Per WHATWG HTML specification, section 4.12.1.3, there are extremely
// complicated rules for how to handle the set of opening tags <!--,
// <script, and </script when they appear in JS literals (i.e. strings,
// regexs, and comments). The specification suggests a simple solution,
// rather than implementing the arcane ABNF, which involves simply escaping
// the opening bracket with \x3C. We use the below regex for this, since it
// makes doing the case-insensitive find-replace much simpler.
specialScriptTagRE = regexp.MustCompile("(?i)<(script|/script|!--)")
specialScriptTagReplacement = []byte("\\x3C$1")
)

func containsSpecialScriptTag(s []byte) bool {
return specialScriptTagRE.Match(s)
}

func escapeSpecialScriptTags(s []byte) []byte {
return specialScriptTagRE.ReplaceAll(s, specialScriptTagReplacement)
}

var doctypeBytes = []byte("<!DOCTYPE")

// escapeText escapes a text template node.
Expand Down Expand Up @@ -786,6 +807,11 @@ func (e *escaper) escapeText(c context, n *parse.TextNode) context {
b.Write(s[written:cs])
written = i1
}
if isInScriptLiteral(c.state) && containsSpecialScriptTag(s[i:i1]) {
b.Write(s[written:i])
b.Write(escapeSpecialScriptTags(s[i:i1]))
written = i1
}
if i == i1 && c.state == c1.state {
panic(fmt.Sprintf("infinite loop from %v to %v on %q..%q", c, c1, s[:i], s[i:]))
}
Expand Down
47 changes: 46 additions & 1 deletion src/html/template/escape_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,21 @@ func TestEscape(t *testing.T) {
"<script>#! beep\n</script>",
"<script>\n</script>",
},
{
"Special tags in <script> string literals",
`<script>var a = "asd < 123 <!-- 456 < fgh <script jkl < 789 </script"</script>`,
`<script>var a = "asd < 123 \x3C!-- 456 < fgh \x3Cscript jkl < 789 \x3C/script"</script>`,
},
{
"Special tags in <script> string literals (mixed case)",
`<script>var a = "<!-- <ScripT </ScripT"</script>`,
`<script>var a = "\x3C!-- \x3CScripT \x3C/ScripT"</script>`,
},
{
"Special tags in <script> regex literals (mixed case)",
`<script>var a = /<!-- <ScripT </ScripT/</script>`,
`<script>var a = /\x3C!-- \x3CScripT \x3C/ScripT/</script>`,
},
{
"CSS comments",
"<style>p// paragraph\n" +
Expand Down Expand Up @@ -1533,8 +1548,38 @@ func TestEscapeText(t *testing.T) {
context{state: stateJS, element: elementScript},
},
{
// <script and </script tags are escaped, so </script> should not
// cause us to exit the JS state.
`<script>document.write("<script>alert(1)</script>");`,
context{state: stateText},
context{state: stateJS, element: elementScript},
},
{
`<script>document.write("<script>`,
context{state: stateJSDqStr, element: elementScript},
},
{
`<script>document.write("<script>alert(1)</script>`,
context{state: stateJSDqStr, element: elementScript},
},
{
`<script>document.write("<script>alert(1)<!--`,
context{state: stateJSDqStr, element: elementScript},
},
{
`<script>document.write("<script>alert(1)</Script>");`,
context{state: stateJS, element: elementScript},
},
{
`<script>document.write("<!--");`,
context{state: stateJS, element: elementScript},
},
{
`<script>let a = /</script`,
context{state: stateJSRegexp, element: elementScript},
},
{
`<script>let a = /</script/`,
context{state: stateJS, element: elementScript, jsCtx: jsCtxDivOp},
},
{
`<script type="text/template">`,
Expand Down
15 changes: 15 additions & 0 deletions src/html/template/transition.go
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,11 @@ var (
// element states.
func tSpecialTagEnd(c context, s []byte) (context, int) {
if c.element != elementNone {
// script end tags ("</script") within script literals are ignored, so that
// we can properly escape them.
if c.element == elementScript && (isInScriptLiteral(c.state) || isComment(c.state)) {
return c, len(s)
}
if i := indexTagEnd(s, specialTagEndMarkers[c.element]); i != -1 {
return context{}, i
}
Expand Down Expand Up @@ -353,6 +358,16 @@ func tJSDelimited(c context, s []byte) (context, int) {
inCharset = true
case ']':
inCharset = false
case '/':
// If "</script" appears in a regex literal, the '/' should not
// close the regex literal, and it will later be escaped to
// "\x3C/script" in escapeText.
if i > 0 && i+7 <= len(s) && bytes.Compare(bytes.ToLower(s[i-1:i+7]), []byte("</script")) == 0 {
i++
} else if !inCharset {
c.state, c.jsCtx = stateJS, jsCtxDivOp
return c, i + 1
}
default:
// end delimiter
if !inCharset {
Expand Down

0 comments on commit 2070531

Please sign in to comment.