[release-1.9] Bump ocicrypt and go-jose CVE-2024-28180 by TomSweeneyRedHat · Pull Request #2293 · podman-container-tools/skopeo

TomSweeneyRedHat · 2024-04-11T21:41:43Z

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-30788

mtrmac

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.

mtrmac · 2024-04-11T22:14:42Z

At a first glance, the test failure probably requires backporting also #2286 .

TomSweeneyRedHat · 2024-04-18T23:07:46Z

I've reworked this. I skipped the ocicrypt bump, and just went with the sigstore bump like we decided upon in the release-1.11 branch #2292

mtrmac · 2024-04-19T15:25:59Z

(That’s a lot of updates for an old branch. I assume we are fine with that level of risk, and that it builds correctly on the relevant old target platforms — I haven’t checked.)
Does this intentionally avoid updating the square/go-jose version? I’d like to clearly differentiate between “we have checked and know that the vulnerability is unreachable” and “we didn’t check”. (Per earlier analysis, I’m fine with the letsencrypt/boulder caller calling an old version. I didn’t check ocicrypt in detail.)
The test failures require at least [release-1.11] Backport #2280 #2286 , and doing something about the macOS test runner.

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-30788 Signed-off-by: tomsweeneyredhat <tsweeney@redhat.com>

TomSweeneyRedHat · 2024-04-25T22:15:50Z

I've done a cherry pick and repushed. We'll see where that gets it.

mtrmac · 2024-04-26T15:33:08Z

Linux looks good.

I think the macOS builds can just be disabled: They don’t matter for the backports. (Alternatively, #1813 , but we would be building with a recent Go, and that’s probably eventually going to break anyway.)

TomSweeneyRedHat · 2024-05-01T19:21:46Z

I repushed with the .cirrus.yaml change. @mtrmac, I added them to the cherry pick without thinking. If you want me to move it to the first commit or a new commit, let me know.

@cevich, could you eyeball the .cirrus.yaml change, please? My first official change to a CI yaml file.

cevich

The .cirrus.yml changes mostly LGTM. All my comments are non-blocking, just suggestions. I'm happy to take a peek @TomSweeneyRedHat you did a fine job 😄

cevich · 2024-05-01T20:06:26Z

+#######
+# Removed the osx task in the Skopeo release-1.9 branch on May 1, 2024.
+# This release going forward is likely to not be delivered to anything 
+# but RHEL for bug fixes.  AS the CI is failing out right, we'll just 
+# comment it out for now.  TODO: Remove at some later time.
+#######
+#osx_task:
+#    # Run for regular PRs and those with [CI:BUILD] but not [CI:DOCS]
+#    only_if: &not_docs_multiarch >-
+#        $CIRRUS_CHANGE_TITLE !=~ '.*CI:DOCS.*' &&
+#        $CIRRUS_CRON != 'multiarch'
+#    depends_on:
+#        - validate
+#    macos_instance:
+#        image: catalina-xcode
+#    setup_script: |
+#        # /usr/local/opt/go@1.18 will be populated by (brew install go@1.18) below
+#        export PATH=$GOPATH/bin:/usr/local/opt/go@1.18/bin:$PATH
+#        brew update
+#        brew install gpgme go@1.18 go-md2man
+#        go install golang.org/x/lint/golint@latest
+#    test_script: |
+#        export PATH=$GOPATH/bin:/usr/local/opt/go@1.18/bin:$PATH
+#        go version
+#        go env
+#        make validate-local test-unit-local bin/skopeo
+#        sudo make install
+#        /usr/local/bin/skopeo -v
+#######


It's not a big deal, but just a gentle reminder: This is version control, you can simply delete the lines and they will live on in the history 😁
(Though it helps the archaeologists if this is done in a dedicated commit)

cevich · 2024-05-01T20:09:56Z

 cross_task:
    alias: cross
-    only_if: *not_docs_multiarch
+#    only_if: *not_docs_multiarch


Hrmm, this jumped out at me. I checked and see it has been removed from main as well. As above, I'd suggest just deleting the line instead of commenting it out.

cevich · 2024-05-01T20:10:34Z

        - validate
        - doccheck
-        - osx
+#        - osx


If you clobber the commented-out task, make sure to remove this line also.

I was on the fence for the clobber, I'll go do so and will move the yaml change to a new commit.

... because the tests are assuming a v2s2 image, but as of Fedora 39, the image uses the OCI format. Signed-off-by: Miloslav Trmač <mitr@redhat.com> Signed-off-by: tomsweeneyredhat <tsweeney@redhat.com>

Remove the osx build which was always failing on this branch. As it's an older branch, we won't be pushing out to anything but RHEL, so osx isn't a concern here. Signed-off-by: tomsweeneyredhat <tsweeney@redhat.com>

cevich

LGTM. Thanks Tom.

mtrmac

Thanks again!

Merging as is.

@TomSweeneyRedHat just to make sure:

The vulnerability is in JSONWebEncryption.Decrypt and JSONWebEncryption.DecryptMulti.

ocicrypt does actually call DecryptMulti, and on this brach, it is calling the square version of the library which remains at unpatched 2.6.0. OTOH the call is in jweKeyWrapper.UnwrapKey, processing a private key of the invoking user. So there should be no privilege escalation.

I’m comfortable with the technical decision to leave the unfixed square…2.6.0 around, and invoked; I just want to highlight this, in case that were insufficient.

TomSweeneyRedHat · 2024-05-02T20:00:38Z

Thanks Miloslav for the merge and detailed analysis. These multiple versions of go-jose are killing me. I missed the square bit. Let me go poke at that. The squre/go-jose only went up to 2.6.0, so we can't bump that. Let me go see if I can remove that dependency on the square one. If not, we'll leave it as is.

mtrmac · 2024-05-02T20:03:12Z

IIRC a small (not full) bump of ocicrypt could do the trick. (A full bump would go all the way to v3, which is not used on this branch yet.)

TomSweeneyRedHat · 2024-05-02T20:52:12Z

ocicrypt is dragging in the square version, which disappeared in ocicrypt v1.1.8. However, that's when ocicrypt started pulling in go-jose:v3, and there are a lot of files that are added. Decisions, decisions.

mtrmac · 2024-05-02T21:30:32Z

So there isn’t an intermediate version of ocicrypt, I’m sorry.

Even an update to go-jose/v3 removes the square/v2 one. It’s not a net addition; even git mostly shows that as renamed files.
The difference between square/v2.6.0 and go-jose/v3.0.0 is very small (mostly the package name change, in errors), and we would migrating along with ocicrypt, the only user, and removing an unmaintained dependency. I rather like that part.
The update of ocicrypt to 1.1.8 is mostly lint updates and removal of pkg/errors, perhaps unwanted churn but mostly manageable.
The difference between JOSE v3.0.0 and v3.0.3 does update a lot of the golang.org/x dependencies, that’s more uncomfortable.

I am fine with either decision.

TomSweeneyRedHat · 2024-05-02T22:10:09Z

@mtrmac I just went through the code and concur with your analysis, the only spot we have that's a problem is the one you spotted in the keywrapper_Jwe.go file. But not being a person that wins bets often, I think we better move forward with #2314 just to be ultra safe.

mtrmac reviewed Apr 11, 2024

View reviewed changes

TomSweeneyRedHat force-pushed the dev/tsweeney/cve-jose-1.9 branch from 54d6795 to 25f38c5 Compare April 18, 2024 23:02

TomSweeneyRedHat force-pushed the dev/tsweeney/cve-jose-1.9 branch from 25f38c5 to db0f387 Compare April 25, 2024 22:15

TomSweeneyRedHat force-pushed the dev/tsweeney/cve-jose-1.9 branch 3 times, most recently from dbfd64e to bf1df09 Compare May 1, 2024 19:18

cevich approved these changes May 1, 2024

View reviewed changes

TomSweeneyRedHat force-pushed the dev/tsweeney/cve-jose-1.9 branch from bf1df09 to b37db26 Compare May 2, 2024 00:05

mtrmac and others added 2 commits May 1, 2024 20:20

[release-1.9] Freeze the fedora-minimal image reference at Fedora 38

336c13e

... because the tests are assuming a v2s2 image, but as of Fedora 39, the image uses the OCI format. Signed-off-by: Miloslav Trmač <mitr@redhat.com> Signed-off-by: tomsweeneyredhat <tsweeney@redhat.com>

[release-1.9] remove osx build from the CI

6fc5bb8

Remove the osx build which was always failing on this branch. As it's an older branch, we won't be pushing out to anything but RHEL, so osx isn't a concern here. Signed-off-by: tomsweeneyredhat <tsweeney@redhat.com>

TomSweeneyRedHat force-pushed the dev/tsweeney/cve-jose-1.9 branch from b37db26 to 6fc5bb8 Compare May 2, 2024 00:21

cevich reviewed May 2, 2024

View reviewed changes

mtrmac approved these changes May 2, 2024

View reviewed changes

mtrmac merged commit 76adb50 into podman-container-tools:release-1.9 May 2, 2024

TomSweeneyRedHat deleted the dev/tsweeney/cve-jose-1.9 branch May 2, 2024 20:14

TomSweeneyRedHat mentioned this pull request May 2, 2024

[release-1.9] remove square/jose, and bump to ocicrypt v1.1.8 #2314

Merged

mtrmac mentioned this pull request May 10, 2024

[release-1.3] Freeze the fedora-minimal image reference at Fedora 38 #2326

Merged

stale-locking-app Bot added the locked - please file new issue/PR label Aug 1, 2024

stale-locking-app Bot locked as resolved and limited conversation to collaborators Aug 1, 2024

Uh oh!

Conversation

TomSweeneyRedHat commented Apr 11, 2024

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

mtrmac commented Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TomSweeneyRedHat commented Apr 18, 2024

Uh oh!

mtrmac commented Apr 19, 2024

Uh oh!

TomSweeneyRedHat commented Apr 25, 2024

Uh oh!

mtrmac commented Apr 26, 2024

Uh oh!

TomSweeneyRedHat commented May 1, 2024

Uh oh!

cevich left a comment

Choose a reason for hiding this comment

Uh oh!

cevich May 1, 2024

Choose a reason for hiding this comment

Uh oh!

cevich May 1, 2024

Choose a reason for hiding this comment

Uh oh!

cevich May 1, 2024

Choose a reason for hiding this comment

Uh oh!

TomSweeneyRedHat May 1, 2024

Choose a reason for hiding this comment

Uh oh!

cevich left a comment

Choose a reason for hiding this comment

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

TomSweeneyRedHat commented May 2, 2024

Uh oh!

mtrmac commented May 2, 2024

Uh oh!

TomSweeneyRedHat commented May 2, 2024

Uh oh!

mtrmac commented May 2, 2024

Uh oh!

TomSweeneyRedHat commented May 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mtrmac commented Apr 11, 2024 •

edited

Loading