make /dev & /dev/shm read/only when --read-only --read-only-tmpfs=false

The intention of --read-only-tmpfs=fals when in --read-only mode was to not allow any processes inside of the container to write content anywhere, unless the caller also specified a volume or a tmpfs. Having /dev and /dev/shm writable breaks this assumption. Fixes: #12937 Signed-off-by: Daniel J Walsh <[email protected]>
containers · Jul 30, 2023 · 22a8b68 · 22a8b68
1 parent b6a52f1
commit 22a8b68
Show file tree

Hide file tree

Showing 7 changed files with 37 additions and 4 deletions.
diff --git a/docs/source/markdown/options/read-only-tmpfs.md b/docs/source/markdown/options/read-only-tmpfs.md
@@ -4,4 +4,4 @@
 ####> are applicable to all of those.
 #### **--read-only-tmpfs**
 
-If container is running in **--read-only** mode, then mount a read-write tmpfs on _/run_, _/tmp_, and _/var/tmp_. The default is **true**.
+If container is running in **--read-only** mode, then mount a read-write tmpfs on _/dev_, _/dev/shm_, _/run_, _/tmp_, and _/var/tmp_. The default is **true**.
diff --git a/libpod/container_config.go b/libpod/container_config.go
@@ -434,6 +434,8 @@ type ContainerMiscConfig struct {
 	// MountAllDevices is an option to indicate whether a privileged container
 	// will mount all the host's devices
 	MountAllDevices bool `json:"mountAllDevices"`
+	// ReadWriteTmpfs indicates whether all tmpfs should be mounted readonly when in ReadOnly mode
+	ReadWriteTmpfs bool `json:"readWriteTmpfs"`
 }
 
 // InfraInherit contains the compatible options inheritable from the infra container

diff --git a/libpod/container_internal_common.go b/libpod/container_internal_common.go
@@ -384,7 +384,7 @@ func (c *Container) generateSpec(ctx context.Context) (s *spec.Spec, cleanupFunc
 			Destination: dstPath,
 			Options:     bindOptions,
 		}
-		if c.IsReadOnly() && dstPath != "/dev/shm" {
+		if c.IsReadOnly() && (dstPath != "/dev/shm" || !c.config.ReadWriteTmpfs) {
 			newMount.Options = append(newMount.Options, "ro", "nosuid", "noexec", "nodev")
 		}
 		if dstPath == "/dev/shm" && c.state.BindMounts["/dev/shm"] == c.config.ShmDir {
@@ -1603,7 +1603,7 @@ func (c *Container) restore(ctx context.Context, options ContainerCheckpointOpti
 				Destination: dstPath,
 				Options:     []string{define.TypeBind, "private"},
 			}
-			if c.IsReadOnly() && dstPath != "/dev/shm" {
+			if c.IsReadOnly() && (dstPath != "/dev/shm" || !c.config.ReadWriteTmpfs) {
 				newMount.Options = append(newMount.Options, "ro", "nosuid", "noexec", "nodev")
 			}
 			if dstPath == "/dev/shm" && c.state.BindMounts["/dev/shm"] == c.config.ShmDir {

diff --git a/libpod/options.go b/libpod/options.go
@@ -723,6 +723,19 @@ func WithPrivileged(privileged bool) CtrCreateOption {
 	}
 }
 
+// WithReadWriteTmpfs sets up read-write tmpfs flag in the container runtime.
+// Only Used if containers are run in ReadOnly mode.
+func WithReadWriteTmpfs(readWriteTmpfs bool) CtrCreateOption {
+	return func(ctr *Container) error {
+		if ctr.valid {
+			return define.ErrCtrFinalized
+		}
+
+		ctr.config.ReadWriteTmpfs = readWriteTmpfs
+		return nil
+	}
+}
+
 // WithSecLabels sets the labels for SELinux.
 func WithSecLabels(labelOpts []string) CtrCreateOption {
 	return func(ctr *Container) error {

diff --git a/pkg/specgen/generate/container_create.go b/pkg/specgen/generate/container_create.go
@@ -559,6 +559,7 @@ func createContainerOptions(rt *libpod.Runtime, s *specgen.SpecGenerator, pod *l
 		}
 	}
 	options = append(options, libpod.WithPrivileged(s.Privileged))
+	options = append(options, libpod.WithReadWriteTmpfs(s.ReadWriteTmpfs))
 
 	// Get namespace related options
 	namespaceOpts, err := namespaceOptions(s, rt, pod, imageData)

diff --git a/pkg/specgen/generate/oci_linux.go b/pkg/specgen/generate/oci_linux.go
@@ -31,6 +31,15 @@ func setProcOpts(s *specgen.SpecGenerator, g *generate.Generator) {
 	}
 }
 
+func setDevOptsReadOnly(g *generate.Generator) {
+	for i := range g.Config.Mounts {
+		if g.Config.Mounts[i].Destination == "/dev" {
+			g.Config.Mounts[i].Options = append(g.Config.Mounts[i].Options, "ro")
+			return
+		}
+	}
+}
+
 // canMountSys is a best-effort heuristic to detect whether mounting a new sysfs is permitted in the container
 func canMountSys(isRootless, isNewUserns bool, s *specgen.SpecGenerator) bool {
 	if s.NetNS.IsHost() && (isRootless || isNewUserns) {
@@ -315,6 +324,9 @@ func SpecGenToOCI(ctx context.Context, s *specgen.SpecGenerator, rt *libpod.Runt
 		g.SetProcessOOMScoreAdj(*s.OOMScoreAdj)
 	}
 	setProcOpts(s, &g)
+	if s.ReadOnlyFilesystem && !s.ReadWriteTmpfs {
+		setDevOptsReadOnly(&g)
+	}
 
 	return configSpec, nil
 }

diff --git a/test/system/030-run.bats b/test/system/030-run.bats
@@ -1073,7 +1073,12 @@ EOF
     CONTAINERS_CONF_OVERRIDE="$containersconf" run_podman 1 run --rm $IMAGE touch /testro
     CONTAINERS_CONF_OVERRIDE="$containersconf" run_podman run --rm --read-only=false $IMAGE touch /testrw
     CONTAINERS_CONF_OVERRIDE="$containersconf" run_podman run --rm $IMAGE touch /tmp/testrw
-    CONTAINERS_CONF_OVERRIDE="$containersconf" run_podman 1 run --rm --read-only-tmpfs=false $IMAGE touch /tmp/testro
+    for dir in /tmp /var/tmp /dev /dev/shm /run; do
+        CONTAINERS_CONF_OVERRIDE="$containersconf" run_podman 1 run --rm --read-only-tmpfs=false $IMAGE touch $dir/testro
+        assert "$output" =~ "touch: $dir/testro: Read-only file system"
+        CONTAINERS_CONF_OVERRIDE="$containersconf" run_podman run --rm --read-only-tmpfs=true $IMAGE touch $dir/testro
+        CONTAINERS_CONF_OVERRIDE="$containersconf" run_podman run --rm --read-only=false $IMAGE touch $dir/testro
+    done
 }
 
 @test "podman run ulimit from containers.conf" {