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

Issue in x/text may casue OOM crash when parsing message #95

Closed
teawithsand opened this issue May 14, 2020 · 6 comments
Closed

Issue in x/text may casue OOM crash when parsing message #95

teawithsand opened this issue May 14, 2020 · 6 comments

Comments

@teawithsand
Copy link

teawithsand commented May 14, 2020

Parsing input like

"Content-Type:text/s;" +
"charset=\"=?hz-gb-231" +
"2?q?ncode_packed_for" +
"mat_for_japanese?~?=" +
"\""

May cause OOM crash due to golang/go#35118 (opened at Oct 23, 2019)

Reproduce steps
go version go1.14.2 linux/amd64

  1. Run go mod init github.com/teawithsand/blah
  2. Run go get github.com/emersion/go-message
  3. Following main.go
package main

import (
	"bytes"
	"io"
	"io/ioutil"
	"unicode/utf8"

	"github.com/emersion/go-message"
	_ "github.com/emersion/go-message/charset" // note: looks like removing this line prevents crash as well
	"github.com/emersion/go-message/mail"
)

func main() {
	// trigger bug using fuzzer fn
        // for sake of simplicity
	Fuzz([]byte("Content-Type:text/s;" +
		"charset=\"=?hz-gb-231" +
		"2?q?ncode_packed_for" +
		"mat_for_japanese?~?=" +
		"\""))
}

func Fuzz(data []byte) int {
	if !utf8.Valid(data) {
		return -1
	}

	r := bytes.NewReader(data)
	ent, err := message.Read(r)
	if err == nil {
		return 0
	}

	pr := mail.NewReader(ent)
	for {
		part, err := pr.NextPart()
		if err != nil {
			return 0
		}
		if part == nil {
			break
		}
		_, err = io.Copy(ioutil.Discard, part.Body)
		if err != nil {
			return 0
		}
	}
	return 1
}
  1. go run .
root@55ce18d9c9ab:/workspaces/go/repr# go run .
signal: killed

This code triggers buggy path in golang.org/x/text and causes OOM crash.

Should this library include this fix to prevent users from shooting themselves in a foot and forbid using hz-gb-2312 encoding? Or maybe some notice about such problem would be nice.
IMO this behavior is too much surprising(AKA unexpected) to just leave it as is.

Proposed fix(in charset/charset.go)
Just before return line insert:

if enc == simplifiedchinese.All[2] {
	return nil, errors.New("This encoding has infinite loop bug and is not supported now") // or some other error maybe?
}

I've used replace in go mod to test it and code similar to above and it turns out to workaround issue.

go.mod

module github.com/teawithsand/blah

go 1.14

require github.com/emersion/go-message v0.11.2

go.sum

github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/emersion/go-message v0.11.2 h1:oxO9SQ+3wgBAQRdk07eqfkCJ26Tl8ZHF7CcpGVoE00o=
github.com/emersion/go-message v0.11.2/go.mod h1:C4jnca5HOTo4bGN9YdqNQM9sITuT3Y0K6bSUw9RklvY=
github.com/emersion/go-textwrapper v0.0.0-20160606182133-d0e65e56babe h1:40SWqY0zE3qCi6ZrtTf5OUdNm5lDnGnjRSq9GgmeTrg=
github.com/emersion/go-textwrapper v0.0.0-20160606182133-d0e65e56babe/go.mod h1:aqO8z8wPrjkscevZJFVE1wXJrLpC5LtJG7fqLOsPb2U=
github.com/martinlindhe/base36 v1.0.0 h1:eYsumTah144C0A8P1T/AVSUk5ZoLnhfYFM3OGQxB52A=
github.com/martinlindhe/base36 v1.0.0/go.mod h1:+AtEs8xrBpCeYgSLoY/aJ6Wf37jtBuR0s35750M27+8=
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
golang.org/x/text v0.3.2 h1:tW2bmiBqwgJj/UpqtC8EpXEZVYOwU0yG4iWbprSVAcs=
golang.org/x/text v0.3.2/go.mod h1:bEr9sfX3Q8Zfm5fL9x+3itogRgK3+ptLWKqgva+5dAk=
golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
@emersion
Copy link
Owner

I think this ought to be fixed upstream. I don't want to provide workarounds for Go stdlib bugs.

@foxcpp
Copy link
Collaborator

foxcpp commented May 14, 2020

It should be fixed upstream and then applications that are concerned should be built with fixed Go version. Depending on how hard the upstream bug to fix is we might actually want to disable the affected encoding in meanwhile since the issue is already public.

@foxcpp
Copy link
Collaborator

foxcpp commented May 14, 2020

Footnote: It is possible for application developers to do so by calling charset.RegisterEncoding("hz-gb-2312", nil).

@teawithsand
Copy link
Author

I think that issue can be closed then. I was surprised that issue in x/text causing oom crash is known for more than half of a year and still can be triggered even with go playground.

https://play.golang.org/p/w-Tap1qn21p

@emersion
Copy link
Owner

Thinking about it, since go-message/charset does provide the list of charsets, I think I'd accept a patch temporarily disabling this charset in this package.

brunnre8 added a commit to brunnre8/go-message that referenced this issue May 14, 2020
Upstream go's x/text/encoding package has a bug that makes the
'hz-gb-2312' encoding crash (golang/go#35118)

This was reported to go-message in
emersion#95

This commit removes the offending encoding so that go-message can't crash if
people import the charset package. It should be reverted once the upstream
package is fixed by the go developers.
brunnre8 added a commit to brunnre8/go-message that referenced this issue May 14, 2020
Upstream go's x/text/encoding package has a bug that makes the
'hz-gb-2312' encoding crash (golang/go#35118)

This was reported to go-message in
emersion#95

This commit removes the offending encoding so that go-message can't crash if
people import the charset package. It should be reverted once the upstream
package is fixed by the go developers.
emersion pushed a commit that referenced this issue May 25, 2020
Upstream go's x/text/encoding package has a bug that makes the
'hz-gb-2312' encoding crash (golang/go#35118)

This was reported to go-message in
#95

This commit removes the offending encoding so that go-message can't crash if
people import the charset package. It should be reverted once the upstream
package is fixed by the go developers.
emersion added a commit that referenced this issue Nov 25, 2020
The upstream Go issue has been fixed [1].

[1]: golang/go#35118

References: #95
@emersion
Copy link
Owner

The charset has been re-enabled now that the upstream issue is fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants