diff --git a/CHANGELOG.md b/CHANGELOG.md index 42e41113177..6667f6079c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 device rules. (#5403) - Some long-standing file-descriptor leaks on the eBPF devices cgroups were fixed. (#5403, #5487) +- `process.user.umask` is now honored for a container which does not have its + own mount namespace. Previously it was silently ignored, and not even the + default umask of 022 was set. (#5479) ### Changed ### - runc now requires Go 1.26+ to build. (#5413) @@ -38,6 +41,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 the runc binary shrunk by about 1 MiB (7.5%) on amd64. (#5403) - The `cpuAffinity` and NUMA `memoryPolicy` settings are no longer limited to 1024 CPUs/nodes, as runc now uses a dynamically-sized CPU mask. (#5343) +- A container configuration which asks for a read-only rootfs + (`root.readonly`), a read-only tmpfs mount, or a read-only `/dev`, but does + not have its own mount namespace, is now refused. Previously such a + container was silently started with those filesystems left writable, since + making them read-only requires a remount, which is only possible in a + private mount namespace. This is consistent with how `maskedPaths` and + `readonlyPaths`, which have the same requirement, were already treated. + (#5371, #5479) ## [1.5.0] - 2026-06-19 diff --git a/libcontainer/configs/mount_linux.go b/libcontainer/configs/mount_linux.go index 6db1b3918fc..77f76f8eb83 100644 --- a/libcontainer/configs/mount_linux.go +++ b/libcontainer/configs/mount_linux.go @@ -1,6 +1,10 @@ package configs -import "golang.org/x/sys/unix" +import ( + "path/filepath" + + "golang.org/x/sys/unix" +) type MountIDMapping struct { // Recursive indicates if the mapping needs to be recursive. @@ -66,3 +70,15 @@ func (m *Mount) IsBind() bool { func (m *Mount) IsIDMapped() bool { return m.IDMapping != nil } + +// IsReadonlyDeferred tells whether the mount is to be mounted read-write +// first, and remounted read-only afterwards (by finalizeRootfs), rather than +// being mounted read-only right away. This is needed for mounts which require +// further modifications after being mounted (setting up files in "/dev", +// chmod-ing tmpfs mounts). +// +// Note that such a remount is only possible in a private mount namespace. +func (m *Mount) IsReadonlyDeferred() bool { + return m.Flags&unix.MS_RDONLY == unix.MS_RDONLY && + (m.Device == "tmpfs" || filepath.Clean(m.Destination) == "/dev") +} diff --git a/libcontainer/configs/validate/validator.go b/libcontainer/configs/validate/validator.go index 6375b65fbc2..7674d3e6cc0 100644 --- a/libcontainer/configs/validate/validator.go +++ b/libcontainer/configs/validate/validator.go @@ -133,6 +133,20 @@ func security(config *configs.Config) error { !config.Namespaces.Contains(configs.NEWNS) { return errors.New("unable to restrict sys entries without a private MNT namespace") } + // A read-only rootfs is implemented by remounting / read-only, which can + // only be done in a private mount namespace (otherwise it would affect + // the host). + if config.Readonlyfs && !config.Namespaces.Contains(configs.NEWNS) { + return errors.New("unable to make rootfs read-only without a private MNT namespace") + } + // Same for mounts which can only be made read-only by a remount. + if !config.Namespaces.Contains(configs.NEWNS) { + for _, m := range config.Mounts { + if m.IsReadonlyDeferred() { + return fmt.Errorf("unable to make %s read-only without a private MNT namespace", m.Destination) + } + } + } if config.ProcessLabel != "" && !selinux.GetEnabled() { return errors.New("selinux label is specified in config, but selinux is disabled or not supported") } diff --git a/libcontainer/configs/validate/validator_test.go b/libcontainer/configs/validate/validator_test.go index dee9e5e0614..fd7a2633746 100644 --- a/libcontainer/configs/validate/validator_test.go +++ b/libcontainer/configs/validate/validator_test.go @@ -172,6 +172,97 @@ func TestValidateSecurityWithoutNEWNS(t *testing.T) { } } +func TestValidateSecurityWithReadonlyfs(t *testing.T) { + config := &configs.Config{ + Rootfs: "/var", + Readonlyfs: true, + Namespaces: configs.Namespaces( + []configs.Namespace{ + {Type: configs.NEWNS}, + }, + ), + } + + err := Validate(config) + if err != nil { + t.Errorf("Expected error to not occur: %+v", err) + } +} + +func TestValidateSecurityReadonlyfsWithoutNEWNS(t *testing.T) { + config := &configs.Config{ + Rootfs: "/var", + Readonlyfs: true, + } + + err := Validate(config) + if err == nil { + t.Error("Expected error to occur but it was nil") + } +} + +func TestValidateSecurityWithReadonlyTmpfs(t *testing.T) { + config := &configs.Config{ + Rootfs: "/var", + Mounts: []*configs.Mount{ + { + Destination: "/dev/shm", + Device: "tmpfs", + Flags: unix.MS_RDONLY, + }, + }, + Namespaces: configs.Namespaces( + []configs.Namespace{ + {Type: configs.NEWNS}, + }, + ), + } + + err := Validate(config) + if err != nil { + t.Errorf("Expected error to not occur: %+v", err) + } +} + +func TestValidateSecurityReadonlyTmpfsWithoutNEWNS(t *testing.T) { + config := &configs.Config{ + Rootfs: "/var", + Mounts: []*configs.Mount{ + { + Destination: "/dev/shm", + Device: "tmpfs", + Flags: unix.MS_RDONLY, + }, + }, + } + + err := Validate(config) + if err == nil { + t.Error("Expected error to occur but it was nil") + } +} + +// A read-only mount which does not need a remount to be made read-only is +// fine without a mount namespace. +func TestValidateSecurityReadonlyBindWithoutNEWNS(t *testing.T) { + config := &configs.Config{ + Rootfs: "/var", + Mounts: []*configs.Mount{ + { + Source: "/etc", + Destination: "/etc", + Device: "bind", + Flags: unix.MS_BIND | unix.MS_RDONLY, + }, + }, + } + + err := Validate(config) + if err != nil { + t.Errorf("Expected error to not occur: %+v", err) + } +} + func TestValidateUserNamespace(t *testing.T) { if _, err := os.Stat("/proc/self/ns/user"); errors.Is(err, os.ErrNotExist) { t.Skip("Test requires userns.") diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go index 35571a29819..446fd8c5803 100644 --- a/libcontainer/rootfs_linux.go +++ b/libcontainer/rootfs_linux.go @@ -278,10 +278,7 @@ func finalizeRootfs(config *configs.Config) (err error) { // All tmpfs mounts and /dev were previously mounted as rw // by mountPropagate. Remount them read-only as requested. for _, m := range config.Mounts { - if m.Flags&unix.MS_RDONLY != unix.MS_RDONLY { - continue - } - if m.Device == "tmpfs" || pathrs.LexicallyCleanPath(m.Destination) == "/dev" { + if m.IsReadonlyDeferred() { if err := remountReadonly(m); err != nil { return err } @@ -295,11 +292,6 @@ func finalizeRootfs(config *configs.Config) (err error) { } } - if config.Umask != nil { - unix.Umask(int(*config.Umask)) - } else { - unix.Umask(0o022) - } return nil } @@ -1496,7 +1488,7 @@ func (m *mountEntry) mountPropagate(rootFd *os.File, mountLabel string) error { // operations on it. We need to set up files in "/dev", and other tmpfs // mounts may need to be chmod-ed after mounting. These mounts will be // remounted ro later in finalizeRootfs(), if necessary. - if m.Device == "tmpfs" || pathrs.LexicallyCleanPath(m.Destination) == "/dev" { + if m.IsReadonlyDeferred() { flags &= ^unix.MS_RDONLY } diff --git a/libcontainer/specconv/spec_linux_test.go b/libcontainer/specconv/spec_linux_test.go index 4578f01158e..02271a19cb6 100644 --- a/libcontainer/specconv/spec_linux_test.go +++ b/libcontainer/specconv/spec_linux_test.go @@ -564,6 +564,7 @@ func TestSpecconvExampleValidate(t *testing.T) { func TestSpecconvNoLinuxSection(t *testing.T) { spec := Example() spec.Root.Path = "/" + spec.Root.Readonly = false // Requires a mount namespace. spec.Linux = nil spec.Hostname = "" diff --git a/libcontainer/standard_init_linux.go b/libcontainer/standard_init_linux.go index afc4cbb09ef..43ed700f2f7 100644 --- a/libcontainer/standard_init_linux.go +++ b/libcontainer/standard_init_linux.go @@ -118,6 +118,14 @@ func (l *linuxStandardInit) Init() error { } } + // Set the umask. This is not related to the rootfs setup, so it has to be + // done even when the container does not have its own mount namespace. + if l.config.Config.Umask != nil { + unix.Umask(int(*l.config.Config.Umask)) + } else { + unix.Umask(0o022) + } + if hostname := l.config.Config.Hostname; hostname != "" { if err := unix.Sethostname([]byte(hostname)); err != nil { return &os.SyscallError{Syscall: "sethostname", Err: err} diff --git a/tests/integration/host-mntns.bats b/tests/integration/host-mntns.bats index 7907a5b9e47..4c488b2b322 100644 --- a/tests/integration/host-mntns.bats +++ b/tests/integration/host-mntns.bats @@ -22,7 +22,8 @@ function teardown() { | .hooks |= . + {"createRuntime": [{"path": "/bin/sh", "args": ["/bin/sh", "-c", "touch createRuntimeHook.$$"]}]} | .linux.namespaces -= [{"type": "mount"}] | .linux.maskedPaths = [] - | .linux.readonlyPaths = []' + | .linux.readonlyPaths = [] + | .root.readonly = false' runc run test_host_mntns [ "$status" -eq 0 ] runc delete -f test_host_mntns