Merge pull request #28450 from TomSweeneyRedHat/dev/tsweeney/cve-2025-52881-follow-4.4.1-rhel

[v4.4.1-rhel] do not pass volume-opt as bind mounts options to runtime

Author: lsm5

libpod/container_internal_common.go

@@ -312,7 +312,13 @@ func (c *Container) generateSpec(ctx context.Context) (*spec.Spec, error) {
 				if err := c.relabel(m.Source, c.MountLabel(), label.IsShared(o)); err != nil {
 					return nil, err
 				}
-
+			case "no-dereference":
+				// crun calls the option `copy-symlink`.
+				// Podman decided for --no-dereference as many
+				// bin-utils tools (e..g, touch, chown, cp) do.
+				options = append(options, "copy-symlink")
+			case "copy", "nocopy":
+				// no real OCI runtime bind mount options, these should already be handled by the named volume mount above
 			default:
 				options = append(options, o)
 			}

libpod/runtime_ctr.go

@@ -479,6 +479,15 @@ func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (_ *Contai
 			_, err := r.state.Volume(vol.Name)
 			if err == nil {
 				// The volume exists, we're good
+				// Make sure to drop all volume-opt options as they only apply to
+				// the volume create which we don't do again.
+				var volOpts []string
+				for _, opts := range vol.Options {
+					if !strings.HasPrefix(opts, "volume-opt") {
+						volOpts = append(volOpts, opts)
+					}
+				}
+				vol.Options = volOpts
 				continue
 			} else if !errors.Is(err, define.ErrNoSuchVolume) {
 				return nil, fmt.Errorf("retrieving named volume %s for new container: %w", vol.Name, err)
@@ -504,6 +513,7 @@ func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (_ *Contai
 		if len(vol.Options) > 0 {
 			isDriverOpts := false
 			driverOpts := make(map[string]string)
+			var volOpts []string
 			for _, opts := range vol.Options {
 				if opts == "idmap" {
 					needsChown = false
@@ -515,8 +525,11 @@ func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (_ *Contai
 						return nil, err
 					}
 					driverOpts[driverOptKey] = driverOptValue
+				} else {
+					volOpts = append(volOpts, opts)
 				}
 			}
+			vol.Options = volOpts
 			if isDriverOpts {
 				parsedOptions := []VolumeCreateOption{WithVolumeOptions(driverOpts)}
 				volOptions = append(volOptions, parsedOptions...)

test/e2e/common_test.go

@@ -421,6 +421,15 @@ func (s *PodmanSessionIntegration) InspectImageJSON() []inspect.ImageData {
 	return i
 }
 
+// PodmanExitCleanly runs a podman command with args, and expects it to ExitCleanly within the default timeout.
+// It returns the session (to allow consuming output if desired).
+func (p *PodmanTestIntegration) PodmanExitCleanly(args ...string) *PodmanSessionIntegration {
+	session := p.Podman(args)
+	session.WaitWithDefaultTimeout()
+	Expect(session).Should(ExitCleanly())
+	return session
+}
+
 // InspectContainer returns a container's inspect data in JSON format
 func (p *PodmanTestIntegration) InspectContainer(name string) []define.InspectContainerData {
 	cmd := []string{"inspect", name}
@@ -475,8 +484,15 @@ func (p *PodmanTestIntegration) RunTopContainerWithArgs(name string, args []stri
 		podmanArgs = append(podmanArgs, "--name", name)
 	}
 	podmanArgs = append(podmanArgs, args...)
-	podmanArgs = append(podmanArgs, "-d", ALPINE, "top")
-	return p.Podman(podmanArgs)
+	podmanArgs = append(podmanArgs, "-d", ALPINE, "top", "-b")
+	session := p.PodmanExitCleanly(podmanArgs...)
+	cid := session.OutputToString()
+	// Output indicates that top is running, which means it's safe
+	// for our caller to invoke `podman stop`
+	if !WaitContainerReady(p, cid, "Mem:", 20, 1) {
+		Fail("Could not start a top container")
+	}
+	return session
 }
 
 // RunLsContainer runs a simple container in the background that

test/e2e/run_volume_test.go

@@ -1,17 +1,20 @@
 package integration
 
 import (
+	"encoding/json"
 	"fmt"
 	"os"
 	"os/exec"
 	"os/user"
 	"path/filepath"
+	"strconv"
 	"strings"
 
 	. "github.com/containers/podman/v4/test/utils"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	. "github.com/onsi/gomega/gexec"
+	"github.com/opencontainers/runtime-spec/specs-go"
 )
 
 // in-container mount point: using a path that is definitely not present
@@ -451,9 +454,27 @@ var _ = Describe("Podman run with volumes", func() {
 		Expect(separateVolumeSession).Should(Exit(0))
 		Expect(separateVolumeSession.OutputToString()).To(Equal(baselineOutput))
 
-		copySession := podmanTest.Podman([]string{"run", "--rm", "-v", "testvol3:/etc/apk:copy", ALPINE, "stat", "-c", "%h", "/etc/apk/arch"})
-		copySession.WaitWithDefaultTimeout()
-		Expect(copySession).Should(Exit(0))
+		podmanTest.PodmanExitCleanly("run", "--name", "testctr", "-v", "testvol3:/etc/apk:copy", ALPINE, "stat", "-c", "%h", "/etc/apk/arch")
+
+		inspect := podmanTest.PodmanExitCleanly("container", "inspect", "testctr", "--format", "{{.OCIConfigPath}}")
+
+		// Make extra check that the OCI config does not contain the copy opt, runc 1.3.0 fails on that while crun does not.
+		// We only test crun upstream so make sure the spec is sane: https://github.com/containers/podman/issues/26938
+		f, err := os.Open(inspect.OutputToString())
+		Expect(err).ToNot(HaveOccurred())
+		defer f.Close()
+		var spec specs.Spec
+		err = json.NewDecoder(f).Decode(&spec)
+		Expect(err).ToNot(HaveOccurred())
+
+		found := false
+		for _, m := range spec.Mounts {
+			if m.Destination == "/etc/apk" {
+				found = true
+				Expect(m.Options).To(Equal([]string{"rprivate", "nosuid", "nodev", "rbind"}))
+			}
+		}
+		Expect(found).To(BeTrue(), "OCI spec contains /etc/apk mount")
 
 		noCopySession := podmanTest.Podman([]string{"run", "--rm", "-v", "testvol4:/etc/apk:nocopy", ALPINE, "stat", "-c", "%h", "/etc/apk/arch"})
 		noCopySession.WaitWithDefaultTimeout()
@@ -875,14 +896,18 @@ VOLUME /test/`, ALPINE)
 	It("podman run with --mount and named volume with driver-opts", func() {
 		// anonymous volume mount with driver opts
 		vol := "type=volume,source=test_vol,dst=/test,volume-opt=type=tmpfs,volume-opt=device=tmpfs,volume-opt=o=nodev"
-		session := podmanTest.Podman([]string{"run", "--rm", "--mount", vol, ALPINE, "echo", "hello"})
-		session.WaitWithDefaultTimeout()
-		Expect(session).Should(Exit(0))
 
-		inspectVol := podmanTest.Podman([]string{"volume", "inspect", "test_vol"})
-		inspectVol.WaitWithDefaultTimeout()
-		Expect(inspectVol).Should(Exit(0))
-		Expect(inspectVol.OutputToString()).To(ContainSubstring("nodev"))
+		// Loop twice to cover both the initial code path that creates the volume and the ones which reuses it.
+		for i := range 2 {
+			name := "testctr" + strconv.Itoa(i)
+			podmanTest.PodmanExitCleanly("run", "--name", name, "--mount", vol, ALPINE, "echo", "hello")
+
+			inspectVol := podmanTest.PodmanExitCleanly("volume", "inspect", "test_vol")
+			Expect(inspectVol.OutputToString()).To(ContainSubstring("nodev"))
+
+			inspect := podmanTest.PodmanExitCleanly("container", "inspect", name, "--format", "{{range .Mounts}}{{.Options}}{{end}}")
+			Expect(inspect.OutputToString()).To(ContainSubstring("[nosuid nodev rbind]"))
+		}
 	})
 
 	It("volume permissions after run", func() {

test/utils/matchers.go

@@ -168,6 +168,58 @@ func (matcher *ExitMatcher) MatchMayChangeInTheFuture(actual interface{}) bool {
 	return true
 }
 
+// ExitCleanly asserts that a PodmanSession exits 0 and with no stderr
+// Consider using PodmanTestIntegration.PodmanExitCleanly instead of directly using this matcher.
+func ExitCleanly() types.GomegaMatcher {
+	return &exitCleanlyMatcher{}
+}
+
+type exitCleanlyMatcher struct {
+	msg string
+}
+
+type podmanSession interface {
+	ExitCode() int
+	ErrorToString() string
+}
+
+func (matcher *exitCleanlyMatcher) Match(actual interface{}) (success bool, err error) {
+	session, ok := actual.(podmanSession)
+	if !ok {
+		return false, fmt.Errorf("ExitCleanly must be passed a PodmanSession; Got:\n %+v\n%q", actual, format.Object(actual, 1))
+	}
+
+	exitcode := session.ExitCode()
+	stderr := session.ErrorToString()
+	if exitcode != 0 {
+		matcher.msg = fmt.Sprintf("Command failed with exit status %d", exitcode)
+		if stderr != "" {
+			matcher.msg += ". See above for error message."
+		}
+		return false, nil
+	}
+
+	// FIXME: #19809, "failed to connect to syslog" warnings on f38
+	// FIXME: so, until that is fixed, don't check stderr if containerized
+	if !Containerized() {
+		if stderr != "" {
+			matcher.msg = fmt.Sprintf("Unexpected warnings seen on stderr: %q", stderr)
+			return false, nil
+		}
+	}
+
+	return true, nil
+}
+
+func (matcher *exitCleanlyMatcher) FailureMessage(_ interface{}) (message string) {
+	return matcher.msg
+}
+
+func (matcher *exitCleanlyMatcher) NegatedFailureMessage(_ interface{}) (message string) {
+	// FIXME - I see no situation in which we could ever want this?
+	return matcher.msg + " (NOT!)"
+}
+
 type ValidJSONMatcher struct {
 	types.GomegaMatcher
 }