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

[release-1.11] Bump ocicrypt and go-jose CVE-2024-28180 #2292

Merged

Conversation

TomSweeneyRedHat
Copy link
Member

Bump github.com/go-jose/go-jose to v3.0.0 and
github.com/containers/ocicrypt to v1.1.10

Addresses: CVE-2024-28180
https://issues.redhat.com/browse/OCPBUGS-30789
https://issues.redhat.com/browse/OCPBUGS-30790
https://issues.redhat.com/browse/OCPBUGS-30791

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

  • Shouldn’t this also include an update to gopkg.in/go-jose/go-jose.v2?
  • I don’t mind the ocicrypt update, but it seems not to be directly related to the vulnerability. (One major effect it does have is updating from go-jose v2 to v3. That might be a good enough reason to include it.) The downside of this is that it rather extends the scope of the update, so I just wanted to double-check that it is intentional.

Feel free to merge as is if all of this is intentional.

@TomSweeneyRedHat
Copy link
Member Author

@mtrmac I've found a fun vendoring puzzle and would like your opinion. In this branch, as you noted, we are using: gopkg.in/square/go-jose.v2. I went to update that, and there are no updates, as that branch was deprecated a while back. That particular version of go-jose seems to be dragged in via: letsencrypt/boulder, which is pulled in from sigstore v1.5.1, which is pulled in via c/image v5.24.

The go-jose issue happens with the Decrypt() call in go-jose. It looks to me that if we bump c/image up to v5.26.latest, then we'll at least get the right go-jose that we can bump from there. Do you think that would work, or do you have other thoughts? Perhaps just bump sigstore to 1.7.1 and see if that cures things. Actually, that might be better (thinking and typing).

Anyway, I'm assuming we may have the same issue in other projects, too, so any thoughts will be gratefully accepted.

@mtrmac
Copy link
Collaborator

mtrmac commented Apr 16, 2024

Oh, I had happily forgotten everything about this situation. To remind myself, there are three locations:

  • github.com/go-jose/go-jose
  • gopkg.in/go-jose/go-jose.v2
  • gopkg.in/square/go-jose.v2

For letsencrypt/boulder, we only call one utility in letsencrypt/boulder/goodkey. That can refer to JOSE types but does not actually call any code. If that were the only one, I’d lean towards just doing the paperwork and not updating.

The user in vendor/github.com/sigstore/sigstore/pkg/oauthflow (jose.ParseSigned) is really called. From a very quick skim it does not seem to call Decrypt, but I’m not immediately 100% sure, and anyway that would mean even more paperwork, and accepting more risk.

Assuming the vulnerability is fixed in go-jose/go-jose.v2, I’d recommend go mod edit -replace gopkg.in/square/go.jose.v2=gopkg.in/go-jose/go-jose.v2@version (or whatever the syntax is); that’s upgrading only the vulnerable project, and it would be far less disruptive than upgrading the various callers of the jose package.

@TomSweeneyRedHat
Copy link
Member Author

@mtrmac
I'm having no luck with the replace. I keep running into this when I do go mod tidy:
go: gopkg.in/go-jose/go-jose.v2@v2.6.3 used for two different module paths (gopkg.in/go-jose/go-jose.v2 and gopkg.in/square/go-jose.v2)

This StackOverflow entry,
https://stackoverflow.com/questions/63918238/golang-module-name-change-causes-local-tests-to-fail plus other similar ones, seem to indicate that you need to replace all the imports too, along with the go mod replace and this issue golang/go#26904 seems to indicate that replace without doing a mass sed is still a WIP until Go 1.22 or later (and it's been bumped a couple of versions from when it was first reported).

It looks to me that the replace command is good for replacing the same module at the same location with a different version, or to replace the module with one that is stored locally on the system. NOT to replace a module with a "moved" external module like in this case.

@Luap99 or @vrothberg I know you two are go.mod wizards, any thoughts?

@Luap99
Copy link
Member

Luap99 commented Apr 17, 2024

It looks to me that the replace command is good for replacing the same module at the same location with a different version, or to replace the module with one that is stored locally on the system. NOT to replace a module with a "moved" external module like in this case.

No that works fine normally, i.e. we do it all the time when we vendor test commits in podman from our forks.

go mod edit -replace gopkg.in/square/go.jose.v2=gopkg.in/go-jose/go-jose.v2@v2.6.3 followed by make vendor should work fine, it doesn't however change any vendored code which is weird as the upstream tag clearly contains changes so something doesn't add up here.

@Luap99
Copy link
Member

Luap99 commented Apr 17, 2024

go mod edit -replace gopkg.in/square/go.jose.v2=gopkg.in/go-jose/go-jose.v2@v2.6.3

Nevermind copied pasted the wrong source here, with go mod edit -replace gopkg.in/square/go-jose.v2=gopkg.in/go-jose/go-jose.v2@v2.6.3 I get the same vendor issue as you @TomSweeneyRedHat

I think the new vendor system is simply not compatible with the old version suffix vendor model used by go-jose v2

@Luap99
Copy link
Member

Luap99 commented Apr 17, 2024

I think the new vendor system is simply not compatible with the old version suffix vendor model used by go-jose v2

Ah no it is because the new version uses the new import location while the other packages uses the old one and go only allows one location per module so yes the linked go issue seems to apply here.

Not sure how to get unstucked here, I guess bumping all the users of gopkg.in/square/go-jose.v2 to newer version that use the new v3 module instead but yes this will likely bring in a lot of unrelated changes.

@Luap99
Copy link
Member

Luap99 commented Apr 17, 2024

Ok I did a check in the repos the pull in the v2, you could bump sigstore to v1.5.2 and letsencrypt/boulder to release-2023-01-30. Then make sure to change gopkg.in/go-jose/go-jose.v2 to v2.6.3 in the indirect list.

@mtrmac
Copy link
Collaborator

mtrmac commented Apr 17, 2024

For the record, per https://go-review.googlesource.com/c/go/+/126156 rejecting the replace is intentional. I was not aware of that.

@mtrmac
Copy link
Collaborator

mtrmac commented Apr 17, 2024

Ok I did a check in the repos the pull in the v2, you could bump sigstore to v1.5.2 and letsencrypt/boulder to release-2023-01-30. Then make sure to change gopkg.in/go-jose/go-jose.v2 to v2.6.3 in the indirect list.

I agree, and this seems reasonably low-risk at least for the dependencies we call directly. (The boulder GoodKey function that is called from sigstore enforces a key size check in the updated version, but AFAICS all of that code is dead in Skopeo.)

Note that this would bump the required Go version to 1.18 due to use of any.


The worst-case solution would be to “fork”, of sorts, the square/go-jose repo at 2.6.3 in another location, and use go replace to replace the square version only.

@TomSweeneyRedHat
Copy link
Member Author

@Luap99 and @mtrmac TYVM for the investigation and suggestions. I'll give that a whirl.

Bump github.com/go-jose/go-jose to v3.0.0 and
github.com/containers/ocicrypt to v1.1.10

Addresses: CVE-2024-28180
https://issues.redhat.com/browse/OCPBUGS-30789
https://issues.redhat.com/browse/OCPBUGS-30790
https://issues.redhat.com/browse/OCPBUGS-30791

Signed-off-by: tomsweeneyredhat <tsweeney@redhat.com>
@TomSweeneyRedHat
Copy link
Member Author

Repushed, that tango of a vendor dance seemed to have turned the trick!

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mtrmac mtrmac merged commit 0518984 into containers:release-1.11 Apr 17, 2024
8 checks passed
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Jul 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants