[go] cmd/go/internal/modcmd: check hashes in go.sum against GOSUMDB
Baokun Lee (Gerrit)
Baokun Lee has uploaded this change for review.
cmd/go/internal/modcmd: check hashes in go.sum against GOSUMDB
For each module in the build list not matched by GOPRIVATE or
GONOSUMDB (that is, each publicly available module), go mod verify
should check the hash of the downloaded module and the hash in
go.sum against the checksum database in GOSUMDB, assuming it is not
disabled.
Fixes #47752
Change-Id: I04503e76147bccfbb1da4ae6f5d363bd57725f92
---
M src/cmd/go/internal/modcmd/verify.go
M src/cmd/go/internal/modfetch/fetch.go
2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/src/cmd/go/internal/modcmd/verify.go b/src/cmd/go/internal/modcmd/verify.go
index 5a6eca3..cf8cbf8 100644
--- a/src/cmd/go/internal/modcmd/verify.go
+++ b/src/cmd/go/internal/modcmd/verify.go
@@ -12,8 +12,10 @@
"io/fs"
"os"
"runtime"
+ "strings"
"cmd/go/internal/base"
+ "cmd/go/internal/cfg"
"cmd/go/internal/modfetch"
"cmd/go/internal/modload"
@@ -100,6 +102,15 @@
return errs
}
h := string(bytes.TrimSpace(data))
+ if !strings.HasPrefix(h, "h1:") {
+ base.Fatalf("verifying %v", module.VersionError(mod, fmt.Errorf("unexpected ziphash: %q", h)))
+ }
+
+ if cfg.GOSUMDB != "off" && !module.MatchPrefixPatterns(cfg.GONOSUMDB, mod.Path) {
+ if err := modfetch.CheckSumDB(mod, h); err != nil {
+ base.Fatalf("%s", err)
+ }
+ }
if zipErr != nil && errors.Is(zipErr, fs.ErrNotExist) {
// ok
diff --git a/src/cmd/go/internal/modfetch/fetch.go b/src/cmd/go/internal/modfetch/fetch.go
index d3d30d9..438f358 100644
--- a/src/cmd/go/internal/modfetch/fetch.go
+++ b/src/cmd/go/internal/modfetch/fetch.go
@@ -581,7 +581,7 @@
// Consult checksum database if appropriate.
if useSumDB(mod) {
// Calls base.Fatalf if mismatch detected.
- if err := checkSumDB(mod, h); err != nil {
+ if err := CheckSumDB(mod, h); err != nil {
return err
}
}
@@ -625,9 +625,9 @@
goSum.m[mod] = append(goSum.m[mod], h)
}
-// checkSumDB checks the mod, h pair against the Go checksum database.
+// CheckSumDB checks the mod, h pair against the Go checksum database.
// It calls base.Fatalf if the hash is to be rejected.
-func checkSumDB(mod module.Version, h string) error {
+func CheckSumDB(mod module.Version, h string) error {
modWithoutSuffix := mod
noun := "module"
if strings.HasSuffix(mod.Version, "/go.mod") {
To view, visit change 345189. To unsubscribe, or for help writing mail filters, visit settings.
Baokun Lee (Gerrit)
Patch set 1:Run-TryBot +1
Jay Conrod (Gerrit)
Attention is currently required from: Bryan C. Mills, Baokun Lee.
4 comments:
Patchset:
Thanks for working on this.
This change needs a test in cmd/go/testdata/script, probably something like mod_verify_sumdb.txt. It should check that:
- 'go mod verify' detects a .mod file whose sum matches go.sum but not the sumdb.
- 'go mod verify' detects a .zip file whose sum matches go.sum and the .ziphash file but not the sumdb.
- 'go mod verify' does not detect either of these errors when GOSUMDB=off or when GONOSUMDB matches the module.
In general, we need to test almost any change to cmd/go more significant than typos and doc fixes. That's doubly true for anything security related.
testdata/script/README has info on the test format. Note that the script tests use a local instance of the sumdb based on the test proxy, which serves modules in testdata/mod.
File src/cmd/go/internal/modcmd/verify.go:
Patch Set #1, Line 88:
var errs []errorWe actually need to verify two sums: one for the .mod file (mod.Version will end with "/go.mod") and one for the .zip file.
The .mod sum is checked against go.sum as part of loading the module graph, before verifyMod is called, but we should still call CheckSumDB here.
Patch Set #1, Line 94:
data, err := os.ReadFile(zip + "hash")Let's use modfetch.Sum instead of reading and trying to verify the ziphash file ourselves. modfetch.Sum should probably be modified to return an error if it can't load the sum.
Patch Set #1, Line 109:
if cfg.GOSUMDB != "off" && !module.MatchPrefixPatterns(cfg.GONOSUMDB, mod.Path) {Let's export and call modfetch.useSumDB instead of copying the code here.
Let's also make modfetch.CheckSumDB panic if UseSumDB returns false for that module path.
To view, visit change 345189. To unsubscribe, or for help writing mail filters, visit settings.
Baokun Lee (Gerrit)
Attention is currently required from: Bryan C. Mills, Baokun Lee.
Baokun Lee uploaded patch set #2 to this change.
cmd/go/internal/modcmd: check hashes in go.sum against GOSUMDB
For each module in the build list not matched by GOPRIVATE or
GONOSUMDB (that is, each publicly available module), go mod verify
should check the hash of the downloaded module and the hash in
go.sum against the checksum database in GOSUMDB, assuming it is not
disabled.
Fixes #47752
Change-Id: I04503e76147bccfbb1da4ae6f5d363bd57725f92
---
M src/cmd/go/internal/modcmd/download.go
M src/cmd/go/internal/modcmd/verify.go
M src/cmd/go/internal/modfetch/bootstrap.go
M src/cmd/go/internal/modfetch/fetch.go
M src/cmd/go/internal/modfetch/sumdb.go
M src/cmd/go/internal/modfetch/zip_sum_test/zip_sum_test.go
M src/cmd/go/internal/modload/build.go
A src/cmd/go/testdata/script/mod_verify_sumdb.txt
8 files changed, 100 insertions(+), 26 deletions(-)
To view, visit change 345189. To unsubscribe, or for help writing mail filters, visit settings.
Baokun Lee (Gerrit)
Attention is currently required from: Bryan C. Mills, Jay Conrod.
Patch set 2:Run-TryBot +1
4 comments:
Patchset:
Thanks for working on this. […]
Done
File src/cmd/go/internal/modcmd/verify.go:
Patch Set #1, Line 88:
var errs []error
We actually need to verify two sums: one for the .mod file (mod.Version will end with "/go. […]
Done
Patch Set #1, Line 94:
data, err := os.ReadFile(zip + "hash")
Let's use modfetch.Sum instead of reading and trying to verify the ziphash file ourselves. modfetch. […]
Done
Patch Set #1, Line 109:
if cfg.GOSUMDB != "off" && !module.MatchPrefixPatterns(cfg.GONOSUMDB, mod.Path) {
Let's export and call modfetch.useSumDB instead of copying the code here. […]
Done
To view, visit change 345189. To unsubscribe, or for help writing mail filters, visit settings.
Jay Conrod (Gerrit)
Attention is currently required from: Bryan C. Mills, Baokun Lee.
12 comments:
File src/cmd/go/internal/modcmd/verify.go:
Patch Set #2, Line 125:
if modErr != nil && errors.Is(modErr, fs.ErrNotExist) {Nit: move all the code to check .mod files up to L113.
Then we'll have three logical blocks: one that checks .zip, one that checks the extracted directory, and one that checks .mod.
Patch Set #2, Line 128:
data, err := os.ReadFile(modfile)Use modfetch.GoModSum to load the sum for the .mod file.
That should be in the block at L140-145, since we only need it if we're using the sumdb.
modload.LoadModGraph already downloads all needed .mod files, computes the sums, and checks against go.sum, so we don't need to do that here. L140-145 is almost all we need.
Patch Set #2, Line 162:
if modfetch.UseSumDB(mod) {Nit: move this to L124 so the code that verifies the .zip file is all together.
File src/cmd/go/internal/modfetch/fetch.go:
Patch Set #2, Line 660:
// if present in the download cache.Let's add: Sum returns an error equivalent to os.ErrNotExist (according to errors.Is) if the module has not been downloaded or if the ziphash file is not present.
Patch Set #2, Line 664:
not set GOMODCACHERephrase: "loading sum: GOMODCACHE is not set"
Patch Set #2, Line 677:
errors.New("invalid contents of a zip hash file: " + mod.Path + "@" + mod.Version)fmt.Errorf("%s: file does not contain a valid sum")
File src/cmd/go/internal/modfetch/zip_sum_test/zip_sum_test.go:
Patch Set #2, Line 134:
sum, _ := modfetch.Sum(test.m)Let's report errors here, including those equivalent to os.ErrNotExist. This test checks the sums for a wide range of downloaded modules. We must have sums for all of them.
File src/cmd/go/testdata/script/mod_verify_sumdb.txt:
Patch Set #2, Line 4:
env GOPROXY GONOPROXY GOSUMDB GONOSUMDBLet's delete the parts here that aren't essential.
GO111MODULE defaults to on, so we don't need to set that explicitly.
We're not changing GOPROXY so no need to set proxy on L3.
L4 just prints those four variables, so let's leave that out, too.
Patch Set #2, Line 5:
[short] skipDon't skip this test in short mode. It doesn't involve accessing the external network or compiling / linking anything, so it should be pretty fast.
Patch Set #2, Line 8:
env GOSUMDB=$sumdb' '$proxy/sumdb-wrongI'd like to find a way to do this without using sumdb-wrong. The sumdb is the source of truth here; the local module cache and go.sum files are less reliable, so let's change those instead.
You can use 'go mod download' or 'go mod download mod@version' to populate the module cache, either with everything in the build list or with a specific version.
You can "corrupt" something in the module cache by overwriting it. For .mod files, that's pretty straightforward. For .zips, you'll need to overwrite $GOMODCACHE/cache/download/$modpath/@v/$version.zip and .ziphash, as well as the extracted directory $GOMODCACHE/$modpath@$version/. A convenient way to do that in this test would be to download one version of a module, then copy it as a different version.
go mod download rsc.io/qu...@v1.5.1
cp $GOMODCACHE/rsc.io/qu...@v1.5.1 $GOMODCACHE/rsc.io/qu...@v1.5.2
cp $GOMODCACHE/cache/download/rsc.io/quote/@v/v1.5.1.zip $GOMODCACHE/cache/download/rsc.io/quote/@v/v1.5.2.zip
cp $GOMODCACHE/cache/download/rsc.io/quote/@v/v1.5.1.ziphash $GOMODCACHE/cache/download/rsc.io/quote/@v/v1.5.2.ziphashPatch Set #2, Line 15:
go get rsc.io/qu...@v1.1.0Why 'go get'? I don't think the version should change here. Maybe 'go mod download' if you just want to ensure all the files are cached?
Need to test GONOSUMDB when GOSUMDB is enabled, too.
To view, visit change 345189. To unsubscribe, or for help writing mail filters, visit settings.
Baokun Lee (Gerrit)
Attention is currently required from: Bryan C. Mills, Baokun Lee.
Baokun Lee uploaded patch set #3 to this change.
cmd/go/internal/modcmd: check hashes in go.sum against GOSUMDB
For each module in the build list not matched by GOPRIVATE or
GONOSUMDB (that is, each publicly available module), go mod verify
should check the hash of the downloaded module and the hash in
go.sum against the checksum database in GOSUMDB, assuming it is not
disabled.
Fixes #47752
Change-Id: I04503e76147bccfbb1da4ae6f5d363bd57725f92
---
M src/cmd/go/internal/modcmd/download.go
M src/cmd/go/internal/modcmd/verify.go
M src/cmd/go/internal/modfetch/bootstrap.go
M src/cmd/go/internal/modfetch/fetch.go
M src/cmd/go/internal/modfetch/sumdb.go
M src/cmd/go/internal/modfetch/zip_sum_test/zip_sum_test.go
M src/cmd/go/internal/modload/build.go
A src/cmd/go/testdata/script/mod_verify_sumdb.txt
8 files changed, 124 insertions(+), 28 deletions(-)
To view, visit change 345189. To unsubscribe, or for help writing mail filters, visit settings.
Baokun Lee (Gerrit)
Attention is currently required from: Bryan C. Mills, Jay Conrod.
Patch set 3:Run-TryBot +1
12 comments:
File src/cmd/go/internal/modcmd/verify.go:
Patch Set #2, Line 125:
if modErr != nil && errors.Is(modErr, fs.ErrNotExist) {
Nit: move all the code to check .mod files up to L113. […]
Done
Patch Set #2, Line 128:
data, err := os.ReadFile(modfile)
Use modfetch.GoModSum to load the sum for the .mod file. […]
Done
Patch Set #2, Line 162:
if modfetch.UseSumDB(mod) {Nit: move this to L124 so the code that verifies the .zip file is all together.
Done
File src/cmd/go/internal/modfetch/fetch.go:
Patch Set #2, Line 660:
// if present in the download cache.
Let's add: Sum returns an error equivalent to os.ErrNotExist (according to errors. […]
Done
Patch Set #2, Line 664:
not set GOMODCACHERephrase: "loading sum: GOMODCACHE is not set"
Done
Patch Set #2, Line 677:
errors.New("invalid contents of a zip hash file: " + mod.Path + "@" + mod.Version)
fmt. […]
Done
File src/cmd/go/internal/modfetch/zip_sum_test/zip_sum_test.go:
Patch Set #2, Line 134:
sum, _ := modfetch.Sum(test.m)
Let's report errors here, including those equivalent to os.ErrNotExist. […]
Done
File src/cmd/go/testdata/script/mod_verify_sumdb.txt:
Patch Set #2, Line 4:
env GOPROXY GONOPROXY GOSUMDB GONOSUMDB
Let's delete the parts here that aren't essential. […]
Done
Patch Set #2, Line 5:
[short] skip
Don't skip this test in short mode. […]
Done
Patch Set #2, Line 8:
env GOSUMDB=$sumdb' '$proxy/sumdb-wrong
I'd like to find a way to do this without using sumdb-wrong. […]
Done
Why 'go get'? I don't think the version should change here. […]
Done
Need to test GONOSUMDB when GOSUMDB is enabled, too.
Done
To view, visit change 345189. To unsubscribe, or for help writing mail filters, visit settings.
Jay Conrod (Gerrit)
Attention is currently required from: Bryan C. Mills, Baokun Lee.
Patch set 3:Code-Review +2Trust +1
1 comment:
Patchset:
Looks good. Thanks again for working on this.
To view, visit change 345189. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: Bryan C. Mills, Baokun Lee.
Patch set 3:Trust +1
Bryan C. Mills (Gerrit)
Attention is currently required from: Baokun Lee.
3 comments:
File src/cmd/go/internal/modcmd/verify.go:
Patch Set #3, Line 91:
if errors.Is(err, fs.ErrNotExist) {It looks like there are three items in the module cache that we verify:
1. The .ziphash file.
2. The .zip file.
3. The unzipped module contents.If I'm reading the code correctly, previously we would skip a module if all three were missing, emit an error if we had a .zip or unzipped directory without a .ziphash, and then use the ziphash to verify whichever of (2) and/or (3) is present.
To fix #47752, we want to also verify the .ziphash file against the checksum database. However, if the .ziphash file is missing and the contents are still present, I think we should still do one of three options:
a. Error out (as we did before).
b. Reconstruct the .ziphash file from whatever is on disk and then check it.
c. Check the .zip file and unzipped contents only against the sumDB, ignoring the missing ziphash.From what I can tell, this function currently (in PS3) does none of those things — instead, it skips the check entirely if the .ziphash file is not present.
If something higher up the stack always recreates the zipfile from the other two sources, a comment or perhaps some restructuring the code would help to make that clearer.
Patch Set #3, Line 109:
dir, dirErr := modfetch.DownloadDir(mod)It's not clear to me why this call to DownloadDir is up here before the modfetch error checks — AFAICT we now don't even check it until line 145.
If there is a specific reason for it (say, some interaction with the calls to modfetch.CheckSumDB below), a comment would be helpful.
File src/cmd/go/testdata/script/mod_verify_sumdb.txt:
(nit) trailing space
To view, visit change 345189. To unsubscribe, or for help writing mail filters, visit settings.
Baokun Lee (Gerrit)
Attention is currently required from: Baokun Lee.
Baokun Lee uploaded patch set #4 to this change.
cmd/go/internal/modcmd: check hashes in go.sum against GOSUMDB
For each module in the build list not matched by GOPRIVATE or
GONOSUMDB (that is, each publicly available module), go mod verify
should check the hash of the downloaded module and the hash in
go.sum against the checksum database in GOSUMDB, assuming it is not
disabled.
Fixes #47752
Change-Id: I04503e76147bccfbb1da4ae6f5d363bd57725f92
---
M src/cmd/go/internal/modcmd/download.go
M src/cmd/go/internal/modcmd/verify.go
M src/cmd/go/internal/modfetch/bootstrap.go
M src/cmd/go/internal/modfetch/fetch.go
M src/cmd/go/internal/modfetch/sumdb.go
M src/cmd/go/internal/modfetch/zip_sum_test/zip_sum_test.go
M src/cmd/go/internal/modload/build.go
A src/cmd/go/testdata/script/mod_verify_sumdb.txt
8 files changed, 122 insertions(+), 26 deletions(-)
To view, visit change 345189. To unsubscribe, or for help writing mail filters, visit settings.
Baokun Lee (Gerrit)
Attention is currently required from: Bryan C. Mills.
Patch set 4:Run-TryBot +1
3 comments:
File src/cmd/go/internal/modcmd/verify.go:
Patch Set #3, Line 91:
if errors.Is(err, fs.ErrNotExist) {
It looks like there are three items in the module cache that we verify: […]
Done
Patch Set #3, Line 109:
dir, dirErr := modfetch.DownloadDir(mod)
It's not clear to me why this call to DownloadDir is up here before the modfetch error checks — AFAI […]
Done
File src/cmd/go/testdata/script/mod_verify_sumdb.txt:
(nit) trailing space
Done
To view, visit change 345189. To unsubscribe, or for help writing mail filters, visit settings.
Jay Conrod (Gerrit)
Attention is currently required from: Bryan C. Mills, Baokun Lee.
Patch set 4:-Code-ReviewTrust +1
1 comment:
File src/cmd/go/internal/modcmd/verify.go:
Patch Set #4, Line 87:
func verifyMod(mod module.Version) []error {I apologize for the meandering review: this is subtle code relevant for security, and it's important we get it right.
Reading through again, it looks like some important things are missing. Here's what I'm looking for:
- [✓] .mod matches go.sum: I think this should happen when we load the build list on L60, so no need to do it here.
- [ ] .mod matches sumdb. This is checked on L127, but that check is skipped if we return early because the zip and ziphash files are absent on L95. We should check this first.
- [✓] .zip file is not downloaded if absent. DownloadDir only checks if the extracted zip directory is present and complete. It doesn't download or extract the zip file.
- [ ] If .zip file is present but the directory or ziphash files are absent, that indicates something went wrong. I think this code still ignores the error. At L92, dirErr and err would both be equivalent to fs.ErrNotExist. We should only skip checking the ziphash if the .zip file is also missing (zipErr is fs.ErrNotExist on L103). Bryan suggested: (a) report an error if the zip file is present, (b) recreate ziphash and download dir, or (c) only check the zip and download dir. I think either (a) or (b) is the right choice. You can implement either by calling modfetch.Download. That will ensure the zip, ziphash, and extracted directory exist, and it will check the ziphash against go.sum. Make sure to stat the zip file first to prevent Download from fetching anything new.
- [ ] .ziphash must match go.sum. modfetch.Download will check this (but see above).
- [✓] .ziphash must match sumdb.
- [✓] zip directory must match ziphash.
Sorry, I realize this is a lot. If you'd prefer, we can submit this CL with everything except the changes in verify.go (just what's needed to compile against the other changes), and I can work on the rest.
To view, visit change 345189. To unsubscribe, or for help writing mail filters, visit settings.
Baokun Lee (Gerrit)
Attention is currently required from: Bryan C. Mills, Jay Conrod.
Patch set 4:-Run-TryBot
1 comment:
File src/cmd/go/internal/modcmd/verify.go:
Patch Set #4, Line 87:
func verifyMod(mod module.Version) []error {
I apologize for the meandering review: this is subtle code relevant for security, and it's important […]
OK, So I roll back the changes of `verify,go` first, right?
To view, visit change 345189. To unsubscribe, or for help writing mail filters, visit settings.
Lee Baokun (Gerrit)
Attention is currently required from: Bryan Mills, Jay Conrod.
Lee Baokun uploaded patch set #5 to this change.
cmd/go/internal/modcmd: check hashes in go.sum against GOSUMDB
For each module in the build list not matched by GOPRIVATE or
GONOSUMDB (that is, each publicly available module), go mod verify
should check the hash of the downloaded module and the hash in
go.sum against the checksum database in GOSUMDB, assuming it is not
disabled.
Fixes #47752
Change-Id: I04503e76147bccfbb1da4ae6f5d363bd57725f92
---
M src/cmd/go/internal/modcmd/download.go
M src/cmd/go/internal/modcmd/verify.go
M src/cmd/go/internal/modfetch/bootstrap.go
M src/cmd/go/internal/modfetch/fetch.go
M src/cmd/go/internal/modfetch/sumdb.go
M src/cmd/go/internal/modfetch/zip_sum_test/zip_sum_test.go
M src/cmd/go/internal/modload/build.go
A src/cmd/go/testdata/script/mod_verify_sumdb.txt
8 files changed, 185 insertions(+), 24 deletions(-)
To view, visit change 345189. To unsubscribe, or for help writing mail filters, visit settings.
Lee Baokun (Gerrit)
Attention is currently required from: Bryan Mills, Jay Conrod.
Patch set 5:Run-TryBot +1
Lee Baokun (Gerrit)
Attention is currently required from: Bryan Mills, Jay Conrod, Lee Baokun.
Lee Baokun uploaded patch set #6 to this change.
cmd/go/internal/modcmd: check hashes in go.sum against GOSUMDB
For each module in the build list not matched by GOPRIVATE or
GONOSUMDB (that is, each publicly available module), go mod verify
should check the hash of the downloaded module and the hash in
go.sum against the checksum database in GOSUMDB, assuming it is not
disabled.
Fixes #47752
Change-Id: I04503e76147bccfbb1da4ae6f5d363bd57725f92
---
M src/cmd/go/internal/modcmd/download.go
M src/cmd/go/internal/modcmd/verify.go
M src/cmd/go/internal/modfetch/bootstrap.go
M src/cmd/go/internal/modfetch/fetch.go
M src/cmd/go/internal/modfetch/sumdb.go
M src/cmd/go/internal/modfetch/zip_sum_test/zip_sum_test.go
A src/cmd/go/testdata/script/mod_verify_sumdb.txt
7 files changed, 135 insertions(+), 24 deletions(-)
To view, visit change 345189. To unsubscribe, or for help writing mail filters, visit settings.
Lee Baokun (Gerrit)
Attention is currently required from: Bryan Mills, Jay Conrod.
Patch set 6:Run-TryBot +1
Bryan Mills (Gerrit)
Attention is currently required from: Bryan Mills, Lee Baokun.
Bryan Mills removed Jay Conrod from this change.
To view, visit change 345189. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Attention is currently required from: Bryan Mills, Lee Baokun.
Bryan Mills removed Than McIntosh from this change.
To view, visit change 345189. To unsubscribe, or for help writing mail filters, visit settings.