From 5283f956a5d4d0d053f01311fd8c455345b9da2f Mon Sep 17 00:00:00 2001 From: Brent Baude Date: Tue, 4 Mar 2025 15:23:32 -0600 Subject: [PATCH] Disallow mounting to certain destination /dir paths When certain directories, like /tmp, get mounted over, FCOS/Linux can act in unexpected ways. Added a sanity check for a list of directories think might be impacted by this. Also, moved the volume parsing earlier in the init process so we can catch problems before the expensive decompression of machine images. The following destinations are forbidden for volumes: `/bin`, `/boot`, `/dev`, `/etc`, `/home`, `/proc`, `/root`, `/run`, `/sbin`, `/sys`, `/tmp`, `/usr`, and `/var`. Subdirectories Fixes: #18230 Signed-off-by: Brent Baude --- .../markdown/podman-machine-init.1.md.in | 6 ++ pkg/machine/e2e/init_test.go | 17 ++++++ pkg/machine/shim/host.go | 42 +++++++++++-- pkg/machine/shim/host_test.go | 61 +++++++++++++++++++ 4 files changed, 121 insertions(+), 5 deletions(-) create mode 100644 pkg/machine/shim/host_test.go 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") + } + }) + } +}