From 22a8b6886637f87d1499989b54fedfefb649a359 Mon Sep 17 00:00:00 2001 From: Daniel J Walsh Date: Sat, 29 Jul 2023 06:31:14 -0400 Subject: [PATCH] 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: https://github.com/containers/podman/issues/12937 Signed-off-by: Daniel J Walsh --- docs/source/markdown/options/read-only-tmpfs.md | 2 +- libpod/container_config.go | 2 ++ libpod/container_internal_common.go | 4 ++-- libpod/options.go | 13 +++++++++++++ pkg/specgen/generate/container_create.go | 1 + pkg/specgen/generate/oci_linux.go | 12 ++++++++++++ test/system/030-run.bats | 7 ++++++- 7 files changed, 37 insertions(+), 4 deletions(-) diff --git a/docs/source/markdown/options/read-only-tmpfs.md b/docs/source/markdown/options/read-only-tmpfs.md index f1419a22a3..190cc7598f 100644 --- 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 index 42b565dbb4..e0ab5c5e97 100644 --- 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 index 9a1cc496fb..4899992892 100644 --- 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 index 8a5aea9397..7d697e3567 100644 --- 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 index 3db8a772f8..2d18a80bac 100644 --- 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 index 91d5a44f42..5dd9abeb6e 100644 --- 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 index 400032195d..9bcfc3e2d0 100644 --- 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" {