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: incorrect parsing of script element with CDATA tag since 1.21.1 #63464

Open
niallnsec opened this issue Oct 9, 2023 · 5 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@niallnsec
Copy link

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: bbd043f

And is related to this security issue: #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 seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 9, 2023
@seankhliao
Copy link
Member

cc @rolandshoemaker

@seankhliao seankhliao changed the title html/template: Incorrect parsing of script element with CDATA tag since 1.21.1 html/template: incorrect parsing of script element with CDATA tag since 1.21.1 Dec 8, 2023
@seankhliao seankhliao added this to the Unplanned milestone Jul 13, 2024
@niallnsec
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

3 participants