Merge pull request #25466 from baude/issue18230

Do not allow mounting to machine dir /tmp

Author: openshift-merge-bot[bot]

docs/source/markdown/podman-machine-init.1.md.in

@@ -149,6 +149,12 @@ options are:
 * **rw**: mount volume read/write (default)
 * **security_model=[model]**: specify 9p security model (see below)
 
+Note: The following destinations are forbidden for volumes: `/bin`, `/boot`, `/dev`, `/etc`,
+`/home`, `/proc`, `/root`, `/run`, `/sbin`, `/sys`, `/tmp`, `/usr`, and `/var`. Subdirectories
+of these destinations are allowed but users should be careful to not mount to important directories
+as unexpected results may occur.
+
+
 The 9p security model [determines] https://wiki.qemu.org/Documentation/9psetup#Starting_the_Guest_directly
 if and how the 9p filesystem translates some filesystem operations before
 actual storage on the host.

pkg/machine/e2e/init_test.go

@@ -79,6 +79,23 @@ var _ = Describe("podman machine init", func() {
 		Expect(badMemSession).To(Exit(125))
 	})
 
+	It("init volume check", func() {
+		skipIfWSL("WSL does not use volume mounts")
+		// Check that mounting to certain target directories like /tmp at the / level is NOT ok
+		tmpVol := initMachine{}
+		targetMount := "/tmp"
+		tmpVolSession, err := mb.setCmd(tmpVol.withVolume(fmt.Sprintf("/whatever:%s", targetMount))).run()
+		Expect(err).ToNot(HaveOccurred())
+		Expect(tmpVolSession).To(Exit(125))
+		Expect(tmpVolSession.errorToString()).To(ContainSubstring(fmt.Sprintf("Error: machine mount destination cannot be %q: consider another location or a subdirectory of an existing location", targetMount)))
+
+		// Mounting to /tmp/foo (subdirectory) is OK
+		tmpSubdir := initMachine{}
+		tmpSubDirSession, err := mb.setCmd(tmpSubdir.withVolume("/whatever:/tmp/foo")).run()
+		Expect(err).ToNot(HaveOccurred())
+		Expect(tmpSubDirSession).To(Exit(0))
+	})
+
 	It("simple init", func() {
 		i := new(initMachine)
 		session, err := mb.setCmd(i.withImage(mb.imagePath)).run()

pkg/machine/shim/host.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"path"
 	"path/filepath"
 	"runtime"
 	"strings"
@@ -118,6 +119,18 @@ func Init(opts machineDefine.InitOptions, mp vmconfigs.VMProvider) error {
 		createOpts.UserModeNetworking = *umn
 	}
 
+	// Mounts
+	if mp.VMType() != machineDefine.WSLVirt {
+		mc.Mounts = CmdLineVolumesToMounts(opts.Volumes, mp.MountType())
+	}
+
+	// Issue #18230 ... do not mount over important directories at the / level (subdirs are fine)
+	for _, mnt := range mc.Mounts {
+		if err := validateDestinationPaths(mnt.Target); err != nil {
+			return err
+		}
+	}
+
 	// Get Image
 	// TODO This needs rework bigtime; my preference is most of below of not living in here.
 	// ideally we could get a func back that pulls the image, and only do so IF everything works because
@@ -251,11 +264,6 @@ func Init(opts machineDefine.InitOptions, mp vmconfigs.VMProvider) error {
 	}
 	ignBuilder.WithUnit(readyUnit)
 
-	// Mounts
-	if mp.VMType() != machineDefine.WSLVirt {
-		mc.Mounts = CmdLineVolumesToMounts(opts.Volumes, mp.MountType())
-	}
-
 	// TODO AddSSHConnectionToPodmanSocket could take an machineconfig instead
 	if err := connection.AddSSHConnectionsToPodmanSocket(mc.HostUser.UID, mc.SSH.Port, mc.SSH.IdentityPath, mc.Name, mc.SSH.RemoteUsername, opts); err != nil {
 		return err
@@ -774,3 +782,27 @@ func Reset(mps []vmconfigs.VMProvider, opts machine.ResetOptions) error {
 	}
 	return resetErrors.ErrorOrNil()
 }
+
+func validateDestinationPaths(dest string) error {
+	// illegalMounts are locations at the / level of the podman machine where we do want users mounting directly over
+	illegalMounts := map[string]struct{}{
+		"/bin":  {},
+		"/boot": {},
+		"/dev":  {},
+		"/etc":  {},
+		"/home": {},
+		"/proc": {},
+		"/root": {},
+		"/run":  {},
+		"/sbin": {},
+		"/sys":  {},
+		"/tmp":  {},
+		"/usr":  {},
+		"/var":  {},
+	}
+	mountTarget := path.Clean(dest)
+	if _, ok := illegalMounts[mountTarget]; ok {
+		return fmt.Errorf("machine mount destination cannot be %q: consider another location or a subdirectory of an existing location", mountTarget)
+	}
+	return nil
+}

pkg/machine/shim/host_test.go

@@ -0,0 +1,61 @@
+package shim
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func Test_validateDestinationPaths(t *testing.T) {
+	tests := []struct {
+		name    string
+		dest    string
+		wantErr bool
+	}{
+		{
+			name:    "Expect fail - /tmp",
+			dest:    "/tmp",
+			wantErr: true,
+		},
+		{
+			name:    "Expect fail trailing /",
+			dest:    "/tmp/",
+			wantErr: true,
+		},
+		{
+			name:    "Expect fail double /",
+			dest:    "//tmp",
+			wantErr: true,
+		},
+		{
+			name:    "/var should fail",
+			dest:    "/var",
+			wantErr: true,
+		},
+		{
+			name:    "/etc should fail",
+			dest:    "/etc",
+			wantErr: true,
+		},
+		{
+			name:    "/tmp subdir OK",
+			dest:    "/tmp/foobar",
+			wantErr: false,
+		},
+		{
+			name:    "/foobar OK",
+			dest:    "/foobar",
+			wantErr: false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := validateDestinationPaths(tt.dest)
+			if tt.wantErr {
+				assert.ErrorContainsf(t, err, "onsider another location or a subdirectory of an existing location", "illegal mount target")
+			} else {
+				assert.NoError(t, err, "mounts to subdirs or non-critical dirs should succeed")
+			}
+		})
+	}
+}