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.31] backport fixes for CVE-2024-11218, CVE-2024-9407, CVE-2024-9675 #5965

Open
wants to merge 13 commits into
base: release-1.31
Choose a base branch
from

Conversation

nalind
Copy link
Member

@nalind nalind commented Feb 3, 2025

What type of PR is this?

/kind bug

What this PR does / why we need it:

This cherry picks fixes for CVE-2024-11218, CVE-2024-9407, and CVE-2024-9675.

How to verify it

Some updated integration tests, some new integration tests.

Which issue(s) this PR fixes:

Fixes CVE-2024-11218
Fixes CVE-2024-9407
Fixes CVE-2024-9675

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Writes to locations mounted using `RUN --mount=type=bind` instructions in Containerfiles or during `buildah run --mount type=bind` are now discarded.

mheon and others added 7 commits January 30, 2025 16:44
The `--mount type=cache` argument to the `RUN` instruction in
Dockerfiles was using `filepath.Join` on user input, allowing
crafted paths to be used to gain access to paths on the host,
when the command should normally be limited only to Buildah;s own
cache and context directories. Switch to `filepath.SecureJoin` to
resolve the issue.

Fixes CVE-2024-9675
https://issues.redhat.com/browse/RHEL-61842

Signed-off-by: Matt Heon <[email protected]>
(cherry picked from commit 873bedd)
Signed-off-by: tomsweeneyredhat <[email protected]>
(cherry picked from commit fa341d2)
CVE-2024-9407: validate that the value for the "bind-propagation" flag
when handling "bind" and "cache" mounts in `buildah run` or in RUN
instructions is one of the values that we would accept without the
"bind-propagation=" prefix.

Signed-off-by: Nalin Dahyabhai <[email protected]>
Allow cache mounts (RUN --mount=type=cache) to refer to other stages or
additional build contexts.

Update the build-check-cve-2024-9675 integration test to use different
directories for its main build context and the additional build context
that it uses for its final run.

Signed-off-by: Nalin Dahyabhai <[email protected]>
Add a package that lets us open a directory in a chroot, pass its
descriptor up, and then bind mount that directory to a specified
location.

Signed-off-by: Nalin Dahyabhai <[email protected]>
Add a helper that uses the new internal/open package to bind mount a
location inside of a chroot direct to a new temporary location, for
ensuring that the latter is not bind-mounted from outside of the chroot.

Signed-off-by: Nalin Dahyabhai <[email protected]>
Add a ForceMount flag to pkg/overlay.Options that forces mounting the
overlay filesystem and returning a bind mount to it instead of trying to
leave that for later in cases where we're able to have the kernel do it.

This is mainly for the sake of callers that want to do more things with
the mounted overlay filesystem before passing them to the (presumably)
OCI runtime.

Signed-off-by: Nalin Dahyabhai <[email protected]>
Add a way to pass a "set the SELinux contexts" labels to
MountWithOptions.

Signed-off-by: Nalin Dahyabhai <[email protected]>
@openshift-ci openshift-ci bot added the kind/bug Categorizes issue or PR as related to a bug. label Feb 3, 2025
Copy link
Contributor

openshift-ci bot commented Feb 3, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nalind

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Feb 3, 2025
When handling RUN --mount=type=bind, where the mount is read-write,
instead of a simple bind mount, create an overlay mount with an upper
directory that will be discarded after the overlay mount is unmounted.
This brings us in line with the expected behavior, wherein writes to
bind mounts should be discarded.

Signed-off-by: Nalin Dahyabhai <[email protected]>
Ensure that the temporary directory that we create is never itself the
top-level directory of the content that we're downloading, in case it's
an archive which includes a "." with weird permissions.

Signed-off-by: Nalin Dahyabhai <[email protected]>
Fix a time-of-check/time-of-use error when mounting type=bind and
type=cache directories that use a "src" flag.  A hostile writer could
use a concurrently-running stage or build to replace that "src" location
between the point when we had resolved possible symbolic links and when
runc/crun/whatever actually went to create the bind mount
(CVE-2024-11218).

Stop ignoring the "src" option for cache mounts when there's no "from"
option.

Signed-off-by: Nalin Dahyabhai <[email protected]>
@nalind nalind force-pushed the cve-2024-11218-1.31 branch from 667986c to 8bf15ac Compare February 3, 2025 14:51
For now, take GOOS=windows out of the set of platforms that we require
ourselves to be able to successfully cross-compile for.

Signed-off-by: Nalin Dahyabhai <[email protected]>
instead of centos-stream+epel-next-9-$arch, mirroring a change which was
just made in containers/podman#22432

Signed-off-by: Nalin Dahyabhai <[email protected]>
It's no longer an active release for the rhcontainerbot/podman-next COPR
at https://copr.fedorainfracloud.org/coprs/rhcontainerbot/podman-next/.

Add "adjustments to .packit.yaml" to the list of things we don't require
updated tests for.

Signed-off-by: Nalin Dahyabhai <[email protected]>
@TomSweeneyRedHat
Copy link
Member

LGTM

@TomSweeneyRedHat
Copy link
Member

and Thanks @nalind , this was a beast!

@nalind
Copy link
Member Author

nalind commented Feb 3, 2025

@dashea FYI FWIW

if overlayDir != "" {
overlayDirs = append(overlayDirs, overlayDir)
}
finalMounts = append(finalMounts, *mountSpec)
Copy link
Member

Choose a reason for hiding this comment

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

@nalind in @cevich 's 1.29 review, there's this line:
mountTargets = append(mountTargets, mount.Destination) here, don't we need that here too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's a bug.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wait, maybe not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants