diff --git a/docs/source/markdown/podman-machine-init.1.md.in b/docs/source/markdown/podman-machine-init.1.md.in index 775362a9e9..4dfc337656 100644 --- a/docs/source/markdown/podman-machine-init.1.md.in +++ b/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. diff --git a/pkg/machine/e2e/init_test.go b/pkg/machine/e2e/init_test.go index 8e3bc72b92..06b1024eb1 100644 --- a/pkg/machine/e2e/init_test.go +++ b/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() diff --git a/pkg/machine/shim/host.go b/pkg/machine/shim/host.go index 82e70be8fa..1d08fd643d 100644 --- a/pkg/machine/shim/host.go +++ b/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 +} diff --git a/pkg/machine/shim/host_test.go b/pkg/machine/shim/host_test.go new file mode 100644 index 0000000000..3a5473c156 --- /dev/null +++ b/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") + } + }) + } +}