Conversation
A read-only rootfs (root.readonly in the OCI spec, config.Readonlyfs in libcontainer) is implemented by remounting / read-only from finalizeRootfs, which is only called for containers having their own mount namespace (doing it otherwise would remount the host's / read-only). As a result, a container configured with root.readonly: true and no mount namespace was silently started with a writable rootfs -- no error, no warning -- leaving whoever relies on that hardening unprotected. Fail closed instead, the same way we already do for maskedPaths and readonlyPaths, which have the very same requirement. Note this requires amending two tests which use a configuration with a read-only rootfs and no mount namespace (in the same manner as they already amend hostname, maskedPaths and readonlyPaths, for the very same reason). Fixes opencontainers#5371 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Setting the umask has nothing to do with the rootfs setup, yet it was done in finalizeRootfs, which is only called when the container has its own mount namespace. Because of that, a container without a mount namespace ignored process.user.umask from the spec, and did not even set the default 0o022. Move it to standard_init, keeping the original ordering (i.e. after the rootfs is finalized), so that it is applied unconditionally. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Read-only tmpfs mounts, as well as a read-only /dev, are first mounted read-write (as we need to modify their contents afterwards), and are only made read-only later, by remounting them from finalizeRootfs. Same as with a read-only rootfs, this is only done for containers having their own mount namespace, so such mounts were silently left writable otherwise. Refuse those configurations, too. Note that all other read-only mounts get MS_RDONLY set right away, and thus are not affected. To avoid the condition drifting apart between the two (now three) places which need it, add configs.Mount.IsReadonlyDeferred and use it everywhere. Note that filepath.Clean is used instead of pathrs.LexicallyCleanPath, as the two only differ for relative paths (which can never be equal to "/dev" either way), and libcontainer/configs should not depend on internal/pathrs. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin
commented
Sep 17, 2026
| // 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") |
Contributor
Author
There was a problem hiding this comment.
See commit description as to why filepath.Clean and pathrs.LexicallyCleanPath are equivalent in this case. In short, when we expect an absolute path, the input should also be absolute path, and in this case they are equivalent.
kolyshkin
requested review from
AkihiroSuda,
lifubang and
rata
and removed request for
rata
September 17, 2026 19:45
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A read-only rootfs (
root.readonly), as well as read-only tmpfs mounts and aread-only
/dev, are all implemented by remounting them read-only fromfinalizeRootfs, which is only called for containers having their own mountnamespace (doing it otherwise would affect the host mounts).
As a result, such a configuration without a mount namespace was silently
started with those filesystems left writable -- no error, no warning --
leaving whoever relies on that hardening unprotected. Meanwhile,
maskedPathsandreadonlyPaths, which have the very same requirement, areproperly refused.
Fail closed in those cases, too. Note that all other read-only mounts get
MS_RDONLYset right away, and thus are not affected.While at it, fix
process.user.umaskbeing ignored (with not even the defaultumask being set) for a container without a mount namespace -- setting the
umask has nothing to do with the rootfs setup, so it does not belong to
finalizeRootfs.Fixes #5371.