jail: fix -j user namespace join, deferred userns, masks and sysfs - #46
Merged
Merged
Conversation
joshuacov1
force-pushed
the
jail-fixes
branch
from
September 26, 2026 10:28
8400ff0 to
de6f8d3
Compare
Contributor
Author
|
Since main HEAD moved since this PR was posted I rebased it on the current HEAD a6f9ffd. The series is unchanged in intent but three patches needed a rework.
Everything else still applies clean and is unchanged. |
dangowrt
requested changes
Sep 27, 2026
Member
There was a problem hiding this comment.
Most of the patches can go in as-is, they fix real issues in a straight forward way.
Patch 4/8 and 8/8 need to be discussed. If you prefer I can pick all the others first, but I'd prefer the whole PR fixed.
Nit: patch 7 (d69f17b) is a long-standing bug from 6f3dbd2 rather than 6aa23a8 fallout, and its commit message does not say so.
joshuacov1
force-pushed
the
jail-fixes
branch
from
September 27, 2026 21:23
de6f8d3 to
8d5fac3
Compare
Contributor
Author
I've reworked the series into 10 patches. The body of 7/10 now names 6f3dbd2 (unconditional CLONE_NEWPID/IPC), with -j from c482c5d making it reachable. Your points on patches 4 and 8 should now be addressed. The details are in the inline threads (old 8 is now 8/10 and 9/10). Or did you have anything else in mind? |
userns_wait_idmaps() remounts / with mountns_propagation() once the user namespace exists. On the deferred path the mount namespace is still owned by the initial user namespace at that point, so the call returns EPERM and the jail dies with "mount propagation failed". isolate_mountns() applies the same propagation to every CLONE_NEWNS jail before any user namespace handling, so drop the second call. Fixes: 6aa23a8 ("jail: give the container's namespaces to its own user namespace") Signed-off-by: Joshua Covington <joshuacov@gmail.com> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
remount_proc_sys_after_unshare() and remount_readonly_now() run in a
mount namespace that does not own the mounts copied into it, where
MNT_LOCK_{NOSUID,NODEV,NOEXEC} are set. A remount passing only
MS_RDONLY asks to clear them and fails with EPERM, so /proc/sys and
any OCI readonlyPath stay writable. Add bind_remount_readonly(),
which ORs in the flags read back from mountinfo. The atime class is
left out of that: path_mount() preserves it on a remount that names
no atime flag, which needs no mountinfo lookup and cannot pick the
wrong class when the path is not found.
Fixes: d381289 ("jail: mask default sensitive /proc,/sys paths for plain jails")
Signed-off-by: Joshua Covington <joshuacov@gmail.com>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
The file masks for /proc/kcore, /proc/sysrq-trigger and the OCI maskedPaths bind the noafile and remount it with a hard-coded MS_RELATIME. procd mounts /tmp with MS_NOATIME and a user namespace from clone() locks the atime class of everything inherited into the jail's mount namespace, so the remount is EPERM: a critical mask fails mount_all() and an optional one is left read-write. Split the remount half of bind_remount_readonly() into remount_readonly() and use it for both noafile mask sites, naming no atime flag so the kernel keeps the class the bind inherited. Fixes: 6aa23a8 ("jail: give the container's namespaces to its own user namespace") Signed-off-by: Joshua Covington <joshuacov@gmail.com> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
The euid taken before clone(), the owner of the staged device nodes, the overlay upper chown, the console hand-over and the devpts gid all key on CLONE_NEWUSER, which -j does not set, and root_map_uid is never derived for a joined namespace. The jail's root then finds the files prepared for it owned by nobody. Derive the mapping by reading /proc/<pid>/uid_map of the pid -j named: uid_m_show() renders the outside column through the reader's user namespace, so reading it from here yields a host uid at any nesting depth. The namespace fd pins the namespace but not that pid, so the map counts only while /proc/<pid>/ns/user still names the namespace ujail holds. Those checks key on jail_has_userns(), false for a namespace given as a path, where the map cannot be read and the ownership decisions would act on a guess. The securebits restore instead keys on the namespace being present, so one invocation always gets one securebits set. Fixes: acf36f2 ("jail: seteuid before clone(CLONE_NEWUSER)") Signed-off-by: Joshua Covington <joshuacov@gmail.com> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
Mounting procfs needs CAP_SYS_ADMIN in the user namespace owning the pid namespace. A jail joining a user namespace by -j entered it at the top of exec_jail(), before any mount, while its pid namespace comes from clone() in the parent and belongs to the initial user namespace, so such a jail with -p failed with EPERM on its own /proc. Build the jail fs privileged and join in enter_userns(), where a deferred user namespace is created, so the join takes the same phase-2 path. That path applied its masks without looking at the result, so make remask_after_unshare() report them and keep the critical ones fatal, as mask_default_paths() does in phase 1. The /proc/sys read-only self-bind follows it into phase 2 on the same terms: fatal, and left alone when the bundle defines /proc/sys itself. Fixes: c482c5d ("jail: add support for referencing existing namespaces") Signed-off-by: Joshua Covington <joshuacov@gmail.com> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
jail_join_ns() reports a stale pid or an unknown namespace type, but the return value is discarded, so the jail starts with none of the requested namespaces and one that asked for a user namespace runs as host root in the initial one. Report the error and refuse to start. Two of the names it accepts never resolved: "mount" and "network" were used verbatim in /proc/<pid>/ns/<name>, where the links are "mnt" and "net", and "mnt" was not accepted at all, so the mount namespace could not be joined under any spelling. Keep the accepted names and their links in one table. Fixes: c482c5d ("jail: add support for referencing existing namespaces") Signed-off-by: Joshua Covington <joshuacov@gmail.com> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
Every non-OCI jail has CLONE_NEWPID and CLONE_NEWIPC added to its namespace set, and a join refused any namespace that set already named. procd appends -j after the flags that populate it, so a jail combining procfs, sysfs or netns with a -j entry for the same namespace was rejected, and clone(CLONE_NEWPID) after setns(CLONE_NEWPID) is EINVAL, so "-j <pid>:pid" never got past clone() either. Clear the create bit of every joined namespace once both the options and the bundle have been read, and fail when the pid namespace cannot be entered. A jail root has to be built in a mount namespace ujail owns, so an entry naming one is refused where a jail filesystem was asked for, and linux.namespaces entries keep their per-type uniqueness. Fixes: c482c5d ("jail: add support for referencing existing namespaces") Signed-off-by: Joshua Covington <joshuacov@gmail.com> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
Move the mount flag translation of idmap_tree_fd() into mountflags_to_attr(). No functional change: MOUNT_ATTR_RELATIME is 0 and __ATIME is only cleared when an atime flag is requested, so a clone with MNT_LOCK_ATIME keeps its atime mode. Signed-off-by: Joshua Covington <joshuacov@gmail.com>
Mounting sysfs needs CAP_SYS_ADMIN in the userns owning the netns. A jail with a userns from clone() and no netns of its own gets EPERM for -s or an OCI sysfs mount and does not start. fsmount() sysfs in the parent before clone() with the queued flags and leave the fd for do_mount_fd() to move_mount(). The mount is not MNT_LOCK_READONLY: only lock_mnt_tree() on a copy into a less privileged userns sets it, and a clone-time userns makes no such copy. A jail keeping CAP_SYS_ADMIN can remount /sys read-write. Fixes: 6aa23a8 ("jail: give the container's namespaces to its own user namespace") Signed-off-by: Joshua Covington <joshuacov@gmail.com> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
The mask branch of do_mount() returns the caller's error code without a message, so a failure to mask a critical path such as /proc/kcore or /sys/firmware surfaces only as "mount_all() failed", and an optional one leaves no trace at all even though the path it was meant to hide stays visible. Report both, with the path and the errno. Signed-off-by: Joshua Covington <joshuacov@gmail.com> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
dangowrt
force-pushed
the
jail-fixes
branch
from
September 28, 2026 11:35
8d5fac3 to
4021843
Compare
Member
|
I've reworked the whole PR because testing showed several regressions introduced, incl. security-relevant ones. I hope it's ok, but I thought to make things move fast, I'll just fix everything and let you re-test if you use-case is now still satisfied. Please report back, then we can get this merged asap. |
Contributor
Author
|
I'm absolutely fine with your approach and it definitely got things moving much faster. Thank you. I re-tested the 10 patches on my router: freeradius (procfs + userns, own user/group, capabilities, no_new_privs, /tmp noatime) starts cleanly again and authentication works. Masks and /proc/sys are read-only as expected. I also ran the -j cases in a test VM (nested user namespace, pid+user, stale pid, mnt/net/network, -N/-p combined with -j), and they all behave as your commit messages describe. I see that a user namespace given as a path no longer gets a root mapping, so the staged device nodes stay nobody-owned inside. That's fine for me. From my side this is good to merge. |
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
3 participants
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.
This is a follow up to #39: the
-j /procEPERM left open there, plus what testing surfaced beyond it. All nine patches are on c230ad8. In details:-f -pjail failsmount_all()silently, every OCI container with maskedPaths has them rw. Keep the flags in effect (3)-j <pid>:pidfailed with clone3 EINVAL (7)Clone-time userns path (uidMappings, uxc/runc/crun parity) is unchanged except in 3 and 8, which turn EPERM into a working mount; namespace ownership is left untouched, the container mounts its own procfs, nested runtimes are ok. The following behaviour changes worth knowing of: on -j user the cgroupns is now host-owned like on the deferred-create path; a netifd-triggered restart whose -j target has exited now fails instead of starting unconfined. Previously masks that were silently left rw, now become ro.
A side note:
The error I hit is more-or-less /tmp's noatime being remapped as relatime by ujail's hard-coded flag, and this only becomes a kernel-enforced failure once a userns is involved. Own, deferred, or joined, doesn't matter which but what matters is that the process is no longer the owner of the original /tmp mount. That's exactly why in my case /etc/init.d/radiusd's -f -p (own userns) hits it: -f is precisely the condition that turns the pre-existing flag mismatch into an enforced EPERM.
Assisted-by: Claude Fable 5.1
@dangowrt since you wrote 6aa23a8 ("jail: give the container's namespaces to its own user namespace"), can you look at this?