html/template: incorrect parsing of script element with CDATA tag since 1.21.1

niallnsec commented 11 months ago

What version of Go are you using (`go version`)?

$ go version
go version go1.21.1 darwin/arm64

Does this issue reproduce with the latest release?

Yes, it does. The script parses correctly using go1.21.0

What operating system and processor architecture are you using (`go env`)?

go env Output

$ go env
GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users//Library/Caches/go-build'
GOENV='/Users//Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users//go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users//go'
GOPRIVATE=''
GOPROXY=''
GOROOT='/opt/homebrew/Cellar/go/1.21.2/libexec'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/opt/homebrew/Cellar/go/1.21.2/libexec/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.21.2'
GCCGO='gccgo'
AR='ar'
CC='cc'
CXX='c++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/7n/55m6c9j15c561rfd89pfj5_r0000gn/T/go-build2874079816=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

I have a Golang service which is parsing templates generated on the fly by a npm/gulp backend for the purposes of development. One of the packages in use in the node side is browsersync, which injects a script into the top of the body of the page. This script is encapsulated in CDATA tags.

A simple reproduction is available here: https://go.dev/play/p/zDDuftHipgG

This issue appears to have been caused by this commit: https://github.com/golang/go/commit/bbd043ff0d6d59f1a9232d31ecd5eacf6507bf6a

And is related to this security issue: https://github.com/golang/go/issues/62197

The commit author does mention that this change will break some legitimate code, but I believe that breaking CDATA tags is a significant enough issue that it should not be ignored.

What did you expect to see?

The template parse and execute correctly.

What did you see instead?

The execution fails with the error:

html/template:temp: ends in a non-text context: {stateJS delimNone urlPartNone jsCtxRegexp attrNone elementScript <nil>}

seankhliao commented 11 months ago

cc @rolandshoemaker

niallnsec commented 2 weeks ago

This is still broken in 1.23.0, considering CDATA comments are used as a security safety feature I think this issue should be fixed.

niallnsec commented 2 weeks ago

Doing some more digging, the issue seems to be more specifically related to having content after a CDATA end tag.

For example, the following would work:

<script id="__bs_script__">//<![CDATA[
//]]>
</script>

But this would not:

<script id="__bs_script__">//<![CDATA[
//]]></script>

If I add in some template stuff like this:

<script id="__bs_script__">//<![CDATA[
//]]>{{ . }}
</script>

If you run that template with any content it will be omitted because the parser is throwing away the whole line because it starts with //

niallnsec commented 2 weeks ago

In the hopes of showing that this is worth fixing, here is an (admittedly contrived) example of how this bug can be used to achieve XSS:

<body><script id="__bs_script__">//<![CDATA[
  (function() {
    console.log("something")
  })()
//]]></script>
alert('you have been pwned')
<script>
console.log("hello")
</script>
</body>

When parsed correctly the code above should render then string "alert('you have been pwned')" to the browser display. But with the behaviour since Go 1.21 this results in execution of the alert statement.

rolandshoemaker commented 2 weeks ago

Hey, sorry this fell through the cracks. I will look into this further. In the future you can contact security@golang.org to report these types of issues to make sure the Security team triages them.

golang / go