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

CVE-2020-14040: v0.3.3 golang.org/x/text #1293

Merged
merged 6 commits into from
Nov 28, 2020
Merged

Conversation

henryleduc
Copy link
Member

CVE-2020-14040

The x/text package before 0.3.3 for Go has a vulnerability in encoding/unicode that could lead to the UTF-16 decoder entering an infinite loop, causing the program to crash or run out of memory. An attacker could provide a single byte to a UTF16 decoder instantiated with UseBOM or ExpectBOM to trigger an infinite loop if the String function on the Decoder is called, or the Decoder is passed to golang.org/x/text/transform.String.

Source: https://nvd.nist.gov/vuln/detail/CVE-2020-14040

@boring-cyborg
Copy link

boring-cyborg bot commented Aug 5, 2020

Hey, thank you for opening your first Pull Request !

@CLAassistant
Copy link

CLAassistant commented Aug 5, 2020

CLA assistant check
All committers have signed the CLA.

@denis-tingaikin
Copy link
Member

The patch looks good to me. Can you please sign the commit?

@bombsimon
Copy link
Member

bombsimon commented Aug 6, 2020

What differs a replace from a proper bump with go get -u golang.org/x/text like this?

diff --git a/go.mod b/go.mod
index 6a9bc93..eb68638 100644
--- a/go.mod
+++ b/go.mod
@@ -58,6 +58,7 @@ require (
 	github.com/ultraware/whitespace v0.0.4
 	github.com/uudashr/gocognit v1.0.1
 	github.com/valyala/quicktemplate v1.6.0
+	golang.org/x/text v0.3.3 // indirect
 	golang.org/x/tools v0.0.0-20200724022722-7017fd6b1305
 	gopkg.in/yaml.v2 v2.3.0
 	honnef.co/go/tools v0.0.1-2020.1.5
diff --git a/go.sum b/go.sum
index db3c095..f7d517a 100644
--- a/go.sum
+++ b/go.sum
@@ -494,6 +494,8 @@ golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
 golang.org/x/text v0.3.1-0.20180807135948-17ff2d5776d2/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
 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/text v0.3.3 h1:cokOdA+Jmi5PJGXLlLllQSgYigAEfHXJAERHVMaCc2k=
+golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ=
 golang.org/x/time v0.0.0-20181108054448-85acf8d2951c/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
 golang.org/x/time v0.0.0-20190308202827-9d24e82272b4/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
 golang.org/x/tools v0.0.0-20180221164845-07fd8470d635/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=

Do replace directives affect all dependencies as well or is there another reason for this?

@henryleduc
Copy link
Member Author

henryleduc commented Aug 6, 2020

The patch looks good to me. Can you please sign the commit?

@denis-tingajkin I'm waiting on my company to approve the CLA so I can sign it 😄


What differs a replace from a proper bump with go get -u golang.org/x/text like this?

diff --git a/go.mod b/go.mod
index 6a9bc93..eb68638 100644
--- a/go.mod
+++ b/go.mod
@@ -58,6 +58,7 @@ require (
 	github.com/ultraware/whitespace v0.0.4
 	github.com/uudashr/gocognit v1.0.1
 	github.com/valyala/quicktemplate v1.6.0
+	golang.org/x/text v0.3.3 // indirect
 	golang.org/x/tools v0.0.0-20200724022722-7017fd6b1305
 	gopkg.in/yaml.v2 v2.3.0
 	honnef.co/go/tools v0.0.1-2020.1.5
diff --git a/go.sum b/go.sum
index db3c095..f7d517a 100644
--- a/go.sum
+++ b/go.sum
@@ -494,6 +494,8 @@ golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
 golang.org/x/text v0.3.1-0.20180807135948-17ff2d5776d2/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
 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/text v0.3.3 h1:cokOdA+Jmi5PJGXLlLllQSgYigAEfHXJAERHVMaCc2k=
+golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ=
 golang.org/x/time v0.0.0-20181108054448-85acf8d2951c/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
 golang.org/x/time v0.0.0-20190308202827-9d24e82272b4/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
 golang.org/x/tools v0.0.0-20180221164845-07fd8470d635/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=

Do replace directives affect all dependencies as well or is there another reason for this?

@bombsimon yes, replace directives will affect all dependencies. https://golang.org/ref/mod#go.mod-replace

@bombsimon
Copy link
Member

Thanks! I guess I could've/should've opened the docs before asking but at least I'm extra sure how it's working now. 😃

@ernado
Copy link
Member

ernado commented Aug 7, 2020

AFAIK replace directives can't affect dependencies, that's why we had to fork our dependencies in thee past, so I'm not sure this will bump any dependency.

Also I can't see v0.3.3 in go.sum, which is suspicious and looks like such replace is no-op.

Probably the right way is to

  1. Add golang.org/x/text as explicit dependency
  2. go get golang.org/x/text@latest
  3. commit go.mod and go.sum

@henryleduc
Copy link
Member Author

henryleduc commented Aug 7, 2020

@ernado Do you have any evidence of this behaviour? it's very common practice to use replace statements exactly for this practice.

Also see changes after running go mod download and go mod tidy with the replace directive.

@ernado
Copy link
Member

ernado commented Aug 7, 2020

Now I see changes in go.sum.
I'm not sure that it will work. but looks good to me.
Anyway adding this as explicit dependency looks more reliable.

@sayboras
Copy link
Member

sayboras commented Aug 7, 2020

replace directive is normally causing issue if the project is used as library or dependency. As golangci-lint is CLI tool, I am ok with having replace statement.

But still if it's possible, direct dependency is the best case :)

@sayboras sayboras requested a review from a team August 7, 2020 14:30
go.mod Outdated Show resolved Hide resolved
@henryleduc
Copy link
Member Author

henryleduc commented Aug 7, 2020

replace directive is normally causing issue if the project is used as library or dependency. As golangci-lint is CLI tool, I am ok with having replace statement.

But still if it's possible, direct dependency is the best case :)

@sayboras I'm intrigued I have never come across this and I use them in packages that are pure libraries, so I'm interested what problems you have encountered?

@ernado
Copy link
Member

ernado commented Aug 7, 2020

Please see golang/go#30354

@sayboras
Copy link
Member

sayboras commented Aug 8, 2020

Just curious that we have one GA dependency check with nancy, seems like the vulnerbility db was not up to date :). Keen to see if it will fail in next few day.

@SVilgelm
Copy link
Member

SVilgelm commented Aug 9, 2020

Just curious that we have one GA dependency check with nancy, seems like the vulnerbility db was not up to date :). Keen to see if it will fail in next few day.

I checked their OSS index and did not find anything about CVE-2020-14040, I don't know how they update the index DB, but the CVE is pretty old and it should be in their DB.

@SVilgelm
Copy link
Member

SVilgelm commented Aug 9, 2020

I opened an issue for OSS Index: OSSIndex/vulns#115

@SVilgelm
Copy link
Member

SVilgelm commented Aug 9, 2020

According to the documentation:

The go command allows a build to include at most one version of any particular module path, meaning at most one of each major version: one rsc.io/quote, one rsc.io/quote/v2, one rsc.io/quote/v3, and so on. This gives module authors a clear rule about possible duplication of a single module path: it is impossible for a program to build with both rsc.io/quote v1.5.2 and rsc.io/quote v1.6.0. At the same time, allowing different major versions of a module (because they have different paths) gives module consumers the ability to upgrade to a new major version incrementally. In this example, we wanted to use quote.Concurrency from rsc/quote/v3 v3.1.0 but are not yet ready to migrate our uses of rsc.io/quote v1.5.2. The ability to migrate incrementally is especially important in a large program or codebase.

And since they didn't change a major version, we can use direct dependency instead of replace directive.
So I vote for direct dependency

go get golang.org/x/text
rm go.sum
go mod tidy

gives this diff:

diff --git a/go.mod b/go.mod
index 6a9bc93..eb68638 100644
--- a/go.mod
+++ b/go.mod
@@ -58,6 +58,7 @@ require (
        github.com/ultraware/whitespace v0.0.4
        github.com/uudashr/gocognit v1.0.1
        github.com/valyala/quicktemplate v1.6.0
+       golang.org/x/text v0.3.3 // indirect
        golang.org/x/tools v0.0.0-20200724022722-7017fd6b1305
        gopkg.in/yaml.v2 v2.3.0
        honnef.co/go/tools v0.0.1-2020.1.5
diff --git a/go.sum b/go.sum
index db3c095..b87256f 100644
--- a/go.sum
+++ b/go.sum
@@ -492,8 +492,9 @@ golang.org/x/sys v0.0.0-20200519105757-fe76b779f299 h1:DYfZAGf2WMFjMxbgTjaC+2HC7
 golang.org/x/sys v0.0.0-20200519105757-fe76b779f299/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
 golang.org/x/text v0.3.1-0.20180807135948-17ff2d5776d2/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
-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/text v0.3.3 h1:cokOdA+Jmi5PJGXLlLllQSgYigAEfHXJAERHVMaCc2k=
+golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ=
 golang.org/x/time v0.0.0-20181108054448-85acf8d2951c/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
 golang.org/x/time v0.0.0-20190308202827-9d24e82272b4/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
 golang.org/x/tools v0.0.0-20180221164845-07fd8470d635/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=

@SVilgelm
Copy link
Member

SVilgelm commented Aug 9, 2020

And one more, the dependabot does not update the dependencies in replace, so we will need to manually track the updates.

@SVilgelm
Copy link
Member

Here is the answer:

The free OSS Index service is provided on a best effort basis with its own dedicated research team. Due to the complexity of tracking all ecosystems and GitHub issues that our researchers trawl through to find new vulnerabilities, occasionally things fall through the cracks, particularly when things like NVD do not necessarily provide precise identifiers. We are continually working to improve the automation of the system, but we depend on our users of the free service to help us keep track of issues that may otherwise have been missed. Thanks helping make OSS Index better!

So in such cases like we have, if Nancy does not fail, feel free to report the issues here: https://github.com/OSSIndex/vulns/issues

@sayboras
Copy link
Member

@henryleduc any update on this? Feel free to shout out if you need any help.

@Nivl Nivl mentioned this pull request Sep 21, 2020
@sayboras
Copy link
Member

mark this one as draft

@sayboras sayboras marked this pull request as draft October 18, 2020 06:06
@henryleduc
Copy link
Member Author

@sayboras Apologies for the delay on this, it's been super busy lately so haven't had time to have a look at this.

I've addressed all the comments, please let me know if there is anything else.

Seems like a couple indirect or unused dependencies were removed in the go mod tidy from the go.sum.

@henryleduc henryleduc marked this pull request as ready for review November 22, 2020 14:30
@sayboras
Copy link
Member

Seems like a couple indirect or unused dependencies were removed in the go mod tidy from the go.sum.

yes, it's kind of expected behavior with go.sum. I occasionally remove go.sum and re-generate it with go mod tidy.

The changes look good to me, I checked manually if 0.3.4 is used as part of the build, seems like it it.

# Remove all local pkg/mod
$ sudo rm -rf *
$ pwd
/home/tammach/go/pkg/mod/golang.org/x
$ ls -lrt
total 0

# Run the `make build` from this branch
$ make build
go build -o golangci-lint ./cmd/golangci-lint

# Verify which version is used (go build -v didn't print the version)
$ ls -lrt
total 24
dr-x------  7 tammach tammach 4096 Nov 28 18:36 sys@v0.0.0-20201009025420-dfb3f7c4e634
dr-x------ 15 tammach tammach 4096 Nov 28 18:36 tools@v0.0.0-20201013201025-64a9e34f3752
dr-x------  3 tammach tammach 4096 Nov 28 18:36 xerrors@v0.0.0-20200804184101-5ec99f83aff1
dr-x------  9 tammach tammach 4096 Nov 28 18:36 mod@v0.3.0
dr-x------ 24 tammach tammach 4096 Nov 28 18:36 net@v0.0.0-20200822124328-c89045814202
dr-x------ 19 tammach tammach 4096 Nov 28 18:36 text@v0.3.4
$ pwd
/home/tammach/go/pkg/mod/golang.org/x

@ernado ernado merged commit 91e7331 into golangci:master Nov 28, 2020
@golangci-automator
Copy link

Hey, @henryleduc — we just merged your PR to golangci-lint! 🔥🚀

golangci-lint is built by awesome people like you. Let us say “thanks”: we just invited you to join the GolangCI organization on GitHub.
This will add you to our team of maintainers. Accept the invite by visiting this link.

By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.
More information about contributing is here.

Thanks again!

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

Successfully merging this pull request may close these issues.

None yet

9 participants