Skip to content

Conversation

@chrisd8088
Copy link
Member

@chrisd8088 chrisd8088 commented Apr 25, 2022

As our official policy is to support the latest version of Go, we upgrade our CI and release workflows to version 1.18, which was recently released.

We also drop support for versions older than 1.17 as they are not supported by upstream Go, and because the mitigations for CVE-2022-27191 are only available in Go 1.17 and later; see below for details.

After upgrading to Go version 1.18 the go get command no longer builds or installs packages by default; it only manages dependencies in go.mod and go.sum. We therefore switch to using go install to build and install goimports and goversioninfo. We then also no longer need to reset our working tree because go install does not modify go.mod or go.sum.

We update the vendored golang.org/x/crypto and golang.org/x/text modules to the latest versions, which in turn updates the vendored copy of the golang.org/x/net and golang.org/x/sys modules.

Updating these modules' entries in vendor/modules.txt and go.{mod,sum} means we will not be flagged by security scanners regarding either CVE-2021-38561 or CVE-2022-27191, neither of which should actually affect Git LFS.

The Git LFS client should not be affected by CVE-2021-38561 as it pertains the Go x/text/language package and specifically the BCP 47 tag functions, which Git LFS does not use.

The Git LFS client should not be affected by CVE-2022-27191 as it pertains to the Go x/crypto/ssh package and specifically a crash vulnerability in the SSH server functions, which Git LFS does not use.

The specific commands run to perform this update were:

  go get golang.org/x/crypto@latest &&
  go get golang.org/x/text@latest &&
  go mod tidy && go mod vendor

(As an aside, this is another example of why PR #4903 is a nice idea!)

Per the Go language announcement, we need version v0.0.0-20220314234659-1baeb1ce4c0b or higher of golang.org/x/crypto to include the fixes for CVE-2022-27191 (which, again, we don't technically require as we don't use the x/crypto/ssh package). That version of golang.org/x/crypto [depends] (https://github.com/golang/crypto/blob/1baeb1ce4c0b006eff0f294c47cb7617598dfb3d/go.mod#L6) on golang.org/x/[email protected], which includes the backwards-compatibility breaking change from golang/net@540bb53 that introduces the use of the os.ErrDeadlineExceeded while addressing golang/go#48830.

/cc @ChriFo as reporter in #4961.

@chrisd8088 chrisd8088 requested a review from a team as a code owner April 25, 2022 17:28
@chrisd8088
Copy link
Member Author

Ah, I see the undefined: os.ErrDeadlineExceeded error is still popping up for older Go versions. Sigh.

@chrisd8088
Copy link
Member Author

chrisd8088 commented Apr 25, 2022

I'll take a further look at this later, but I suspect we may have to go the route proposed in #4903 and just get rid of vendored Go modules entirely.

UPDATE: I actually don't think this would resolve the incompatibility; it would just make it slightly easier to update modules. Sigh.

Update the vendored golang.org/x/crypto and golang.org/x/text
modules to the latest versions, which in turn updates the vendored
copy of the golang.org/x/net and golang.org/x/sys modules.

Updating these modules' entries in vendor/modules.txt and go.{mod,sum}
means we will not be flagged by security scanners regarding either
CVE-2021-38561 or CVE-2022-27191, neither of which should actually
affect Git LFS.

The Git LFS client should not be affected by CVE-2021-38561 as it
pertains the Go x/text/language package and specifically the BCP 47
tag functions, which Git LFS does not use.

The Git LFS client should not be affected by CVE-2022-27191 as it
pertains to the Go x/crypto/ssh package and specifically a crash
vulnerability in the SSH server functions, which Git LFS does not use.

The specific commands run to perform this update were:

  go get golang.org/x/crypto@latest &&
  go get golang.org/x/text@latest &&
  go mod tidy && go mod vendor
@chrisd8088 chrisd8088 force-pushed the update-vendor-x-modules branch from c2922a8 to c175fdb Compare April 26, 2022 00:19
@chrisd8088
Copy link
Member Author

chrisd8088 commented Apr 26, 2022

On taking a closer look, I don't see a way around the incompatibility of the newer x/crypto package's dependency on a newer x/net and our desire to compile with older Go versions, other than to drop support for Go versions older than 1.17 or just declare this an issue we can't resolve and which doesn't technically affect us, despite what security scanners might say.

Dropping our vendored modules would not help, as I first thought, since we'd still specify an x/crypto module version in go.mod.

Specifically, per the Go language announcement, we need version v0.0.0-20220314234659-1baeb1ce4c0b or higher of golang.org/x/crypto to "mitigate" CVE-2022-27191. I put "mitigate" in quotes because Git LFS doesn't actually use any SSH server functionality and we don't even vendor the x/crypto/ssh code for that reason.

That version of golang.org/x/crypto depends on golang.org/x/[email protected], which includes the backwards-compatibility breaking change from golang/net@540bb531 that introduces the use of the os.ErrDeadlineExceeded while addressing golang/go#48830.

At least, that's as far as I've gotten today; I hope that's all correct.

(One other data point is that Go only supports two major releases at a time, so anything older than 1.17 is unsupported.)

Footnotes

  1. https://cs.opensource.google/go/x/net/+/540bb53d3b2ef1be1c9bd6bffb0281fc6c7038ca

@bk2204
Copy link
Member

bk2204 commented Apr 26, 2022

I think in that case, we may need to just drop support for versions older than 1.17. I don't think we have much of choice, to be honest.

@chrisd8088
Copy link
Member Author

I think in that case, we may need to just drop support for versions older than 1.17. I don't think we have much of choice, to be honest.

That's fine with me; my only question would be whether there were specific reasons to continue supporting older versions (e.g., building in older Linux distributions rather than using a pre-compiled binary?)

I also note that the same announcement mentions that version v0.0.0-20220315160706-3147a52a75dd:

[I]mplements client authentication support for signature algorithms based on SHA-2 for use with existing RSA keys.
Previously, a client would fail to authenticate with RSA keys to servers that reject signature algorithms based on SHA-1.

I don't know if the new pure SSH transfer adaptor, for instance, would be affected by that.

@bk2204
Copy link
Member

bk2204 commented Apr 27, 2022

That's fine with me; my only question would be whether there were specific reasons to continue supporting older versions (e.g., building in older Linux distributions rather than using a pre-compiled binary?)

Yeah, we try to make things a little easier on older distros when possible. In this case, I don't think it's possible, though.

[I]mplements client authentication support for signature algorithms based on SHA-2 for use with existing RSA keys.
Previously, a client would fail to authenticate with RSA keys to servers that reject signature algorithms based on SHA-1.

I don't know if the new pure SSH transfer adaptor, for instance, would be affected by that.

Nope, we use OpenSSH regardless.

@chrisd8088
Copy link
Member Author

Nope, we use OpenSSH regardless.

Yeah, that makes sense, especially since I just noted that we don't vendor the x/crypto/ssh code at all. 🤦 :-)

As our official policy is to support the latest version of Go, we
upgrade our CI and release workflows to version 1.18, which was
recently released.

We also drop support for versions older than 1.17 as they are
not supported by upstream Go, and because the mitigations for
CVE-2022-27191 are only available in Go 1.17 and later.

Specifically, the updates to the x/crypto/ssh package (which Git LFS
does not use, so this security issue does not strictly affect Git LFS)
from version v0.0.0-20220314234659-1baeb1ce4c0b depend on changes in
the x/net package which are not available in any versions of that
package prior to v0.0.0-20211112202133-69e39bad7dc2.  See for reference:

https://groups.google.com/g/golang-announce/c/-cp44ypCT5s
After upgrading to Go version 1.18 the "go get" command no longer
builds or installs packages by default; it only manages dependencies
in go.mod and go.sum.

We therefore switch to using "go install" to build and install
the goimports and goversioninfo.  We then also no longer need to
reset our working tree because "go install" does not modify
go.mod or go.sum.
@chrisd8088 chrisd8088 changed the title Update vendored x/crypto and x/text Go modules and dependencies Update to Go 1.18, drop older Go version support, and update modules and dependencies Apr 27, 2022
Correct a typo from commit 1bd8630
in this PR and use "go install" when building goversioninfo in
our release workflow.

h/t bk2204 on PR review
@chrisd8088 chrisd8088 requested a review from a team April 27, 2022 20:02
@chrisd8088 chrisd8088 merged commit 9f91ab6 into git-lfs:main Apr 27, 2022
@chrisd8088 chrisd8088 deleted the update-vendor-x-modules branch April 27, 2022 21:23
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.

2 participants