Skip to content

jail: harden CLONE_NEWUSER handling, fix dependency RPATH/RUNPATH support - #39

Merged
openwrt-bot merged 14 commits into
openwrt:mainfrom
joshuacov1:ujail-fixes
Aug 15, 2026
Merged

jail: harden CLONE_NEWUSER handling, fix dependency RPATH/RUNPATH support#39
openwrt-bot merged 14 commits into
openwrt:mainfrom
joshuacov1:ujail-fixes

Conversation

@joshuacov1

Copy link
Copy Markdown
Contributor

Running ujail under CLONE_NEWUSER surfaced several independent bugs:

  • /proc,/sys mounting failed on hosts with locked-down mounts; deferred
    userns creation until after mount setup fixes it (1)
  • inherited mounts under /proc,/sys blocked the jail's own mount;
    detached before mounting, and before joining an external userns (3,4)
  • mknod() can't create devices under CLONE_NEWUSER; switched to
    fd-validated bind-mounts (5)
  • uid/gid map handling had reentrancy and error-checking gaps; hardened
    identity resolution throughout (6)
  • /proc,/sys lacked consistent write protection across OCI and plain
    jails; added masking, OCI checks, netns/EPERM safeguards (7-10)
  • cgroups_free() double-free on repeat calls; guarded (2)
  • dependency auto-discovery now also reads a binary's own
    DT_RPATH/DT_RUNPATH, not just the global library search list (11)

Assisted-by: Claude Sonnet 5

This should fix "...mounting /proc currently still fails in the new user namespace
with permission denied for unknown reasons" in commit 6f3dbd283bbdc6f981be5d36d609ee682051f856

@joshuacov1

Copy link
Copy Markdown
Contributor Author

Tested and working on bcm2712 (raspberry pi) with jailed freeradius-3.2.10 and '-f -p -s -N' under separate user.

@joshuacov1

Copy link
Copy Markdown
Contributor Author

Hi everybody,

I know that most of you probably have other full-time jobs and are supporting the OpenWrt project in your free time. However, it's frustrating to wait for weeks without any feedback at all.

Can you look at the PR and point out if something needs to be rearranged or fixed/added? These are just bug fixes. Any help is really appreciated.

cc @dangowrt @Ansuel @nbd168 @dhewg

@dangowrt

Copy link
Copy Markdown
Member

Thank you for contributing and sorry, I was on vacation, I'll take a look at your work now,
I'm anyway working on a bigger rework of procd container support:
https://github.com/dangowrt/procd/tree/procd-ng

Your work hence comes at a good moment and is appreciated, it solves problem I had also been seeing (the O_PATH fd I had also already done, but from what I can see that's the only real overlap).

@dangowrt dangowrt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this review was carried out by a bot; if it got something wrong, please say so.

Thanks, several parts of this series are things we want: the deferred CLONE_NEWUSER creation, the cgroups_free() double-free fix, the getpwnam_r/dprintf/map-write corrections and RPATH/RUNPATH support in the dependency walker.

There is a structural problem: the later patches do not seem to have been revalidated after the first one changed the model. Since 30d262c defers the jail's own userns until after build_jail_fs(), the mount namespace stays owned by the initial user namespace and mount_too_revealing() bails out before any visibility comparison (it is keyed on the mntns owner, not the PID ns as the commit message says). That makes the sysfs/netns check reject a configuration the first patch fixes, lets create_devices() mknod just fine under -f, and leaves no reproduction for what the inherited-mount detach still fixes on the tip. Could you re-test the tip with plain -f -s (no -N), and with a custom device that does not exist on the host?

The OCI /proc,/sys read-only rejection breaks every spec-conformant bundle; see the inline comment.

Two consistency requests: attach validated fds directly via move_mount(MOVE_MOUNT_F_EMPTY_PATH) rather than the /proc/self/fd/%d string, and consolidate the mountinfo parsing into one shared helper. Also, each commit needs to leave the tree fully working on its own, with behaviour changes and their consequences described in the commit message; a couple of the messages describe a pre-series state that no longer exists by the time the commit applies.

Comment thread jail/jail.c Outdated
Comment thread jail/fs.c Outdated
Comment thread jail/jail.c
Comment thread jail/jail.c Outdated
Comment thread jail/jail.c Outdated
Comment thread jail/jail.c
Comment thread jail/fs.c
Comment thread jail/fs.c
Comment thread jail/jail.c Outdated
Comment thread jail/elf.c
Mounting a fresh /proc or /sys from a jail's own CLONE_NEWUSER fails
with EPERM ("VFS: Mount too revealing") whenever the host's reference
/proc has locked-down content anywhere under it. The kernel's mount-
visibility check (fs_fully_visible()/mount_too_revealing()) is keyed
on the new mount's super_block->s_user_ns (the mount namespace's
owning user namespace, set from current_user_ns() at mount() time),
not the PID namespace. Since the jail's own CLONE_NEWUSER is created
together with the PID namespace before /proc,/sys are mounted, the
mount namespace ends up owned by that new, non-init user namespace and
gets rejected. A related mismatch: /proc is mounted with a hardcoded
MS_NOATIME while a host running relatime or strictatime fails the same
visibility check's exact atime-class requirement.

Defer creation of the jail's own CLONE_NEWUSER until after
build_jail_fs()/pivot_root have mounted /proc and /sys, using a
handshake between parent and child (enter_userns(), userns_pipe[4]).
This keeps the mount namespace owned by the initial user namespace
throughout mount setup, which the kernel's visibility check
unconditionally exempts. Detect the host's actual atime class from
/proc/self/mountinfo and match it instead of hardcoding one:
strictatime is represented by the absence of both noatime and relatime
in mountinfo, not by a positive token, so that case is treated as
MS_STRICTATIME.

A mount namespace's owning user namespace is fixed at creation and
never changes afterward, so deferring CLONE_NEWUSER this way means
container root permanently loses the ability to call mount() at
runtime, even after entering the container's own user namespace: the
mount namespace was already created while still in the initial one.
Unshare a second, private mount namespace immediately after entering
the container's own user namespace (guarded on the jail actually
having a mount namespace of its own), and again after joining an
external user namespace via -j. The new mount namespace is owned by
the container's own user namespace, so mount(2)/umount(2) work again
for tmpfs, devpts, mqueue, cgroup2, and overlay, matching what
mainstream OCI runtimes provide. The mounts already established in
the jail (proc/sys, masks, read-only binds) are copied into this new
namespace locked, since they originate from a more privileged
namespace, so they stay immune to detachment from inside. A nested
runtime mounting its own further-nested procfs instance still fails
the kernel's mount-visibility check for reasons independent of that
locking, unresolved here.

The userns_pipe handshake retries read()/write() on EINTR and uses
pipe2(O_CLOEXEC), so a signal such as SIGTERM arriving mid-handshake
does not abort the jail without running poststop hooks or cleaning up
the network namespace.

Signed-off-by: Joshua Covington <joshuacov@gmail.com>
cgroups_free() frees cgroup_path but leaves both it and the
initialized flag holding their stale values. A second call to
cgroups_free() without an intervening cgroups_init() frees
cgroup_path again, a double free.

cgroups_free() is called from three separate places in jail.c
(error-path cleanup, normal cgroup teardown, and process exit), so
a path that reaches it twice for the same jail is reachable in
practice, not just a theoretical concern.

Reset cgroup_path to NULL and initialized to false immediately after
the free, so a repeat call is a safe no-op instead of a double free.
This mirrors the guard already at the top of the function, which
checks initialized before doing any work.

Signed-off-by: Joshua Covington <joshuacov@gmail.com>
Mounts inherited from the host mount namespace under /proc,/sys can
be locked, and a jail's own procfs/sysfs mount over one of them is
rejected by the kernel's mount-visibility check, independent of the
CLONE_NEWUSER mount-ordering requirements. mountinfo(5) escapes space,
tab, newline, and backslash in its fields as octal \NNN sequences;
comparing mountinfo fields with a plain strcmp() misses any mountpoint
containing one of those bytes.

Add mountinfo_unescape() and mountinfo_detach_children(), which read
/proc/self/mountinfo with getline() (no line-length limit, needed
since option strings such as Docker overlay2's lowerdir= entries can
exceed any fixed buffer size regardless of whether the line concerns
/proc,/sys at all), unescape the mountpoint field, collect mounts
nested under a given prefix, and lazily detach (MNT_DETACH) each one,
deepest path first, retrying over multiple passes since detaching a
parent can expose further nested children. Call this from
build_jail_fs(), under CLONE_NEWUSER, before mounting the jail's own
/proc and /sys, guarded by opts.procfs/opts.sysfs/opts.ocibundle.

Signed-off-by: Joshua Covington <joshuacov@gmail.com>
Detaching an inherited mount requires CAP_SYS_ADMIN in the user
namespace that owns that mount, established at the mount's creation
time, not the caller's current identity. A freshly joined user
namespace has no such authority over mounts it did not create. A
mount inherited from the host under /proc or /sys - at minimum
/sys/fs/cgroup on virtually any real system - is owned by the initial
user namespace, so mountinfo_detach_children() fails once a process
has joined a different, external user namespace.

exec_jail() calls setns_open(CLONE_NEWUSER) to join an externally
created namespace (opts.setns.user) before build_jail_fs() runs, so
that join drops privilege before mount isolation gets a chance to run.
This is a distinct code path from creating the jail's own new user
namespace, whose creation is deferred until after mount setup and is
unaffected by this ordering.

Move the private-mount-plus-detach step out of build_jail_fs() into
its own isolate_mountns_and_detach_inherited(), and call it in
exec_jail() before setns_open(CLONE_NEWUSER) rather than after, so
mount isolation runs while still holding the privilege of whichever
namespace the process was in when exec_jail() started, before an
external userns join can take it away. Guard the call on needing
either the jail's own CLONE_NEWUSER or an external setns.user join,
not on CLONE_NEWNS alone: applying it to every ordinary CLONE_NEWNS
jail with -p/-s would add mountinfo-parsing overhead and new abort
paths to configurations that don't need it.

Signed-off-by: Joshua Covington <joshuacov@gmail.com>
Creating a custom device with mknod() requires CAP_MKNOD in the
user namespace that will contain it. A process joining an external
user namespace (-j, opts.setns.user) has already lost that capability
by the time devices are created, since setns_open(CLONE_NEWUSER) for
that join runs in exec_jail() before build_jail_fs()/create_devices().
A jail creating its own new user namespace (-f) still holds CAP_MKNOD
at that point, since entering that namespace is deferred until after
build_jail_fs() completes. Bind-mounting a device by path with a
plain stat() check leaves a TOCTOU window between validation and
mount and performs no major:minor check, so a swapped node could be
bind-mounted into the jail unnoticed. devpts using host gid 5 fails to
mount under CLONE_NEWUSER, since that gid is unmapped in the
namespace.

create_devices() tries mknod() first in every case. For the
external-join case (opts.setns.user != -1), where mknod() cannot
succeed, validate each requested device by opening it O_PATH on the
host and fstat()'ing the held descriptor, checking node type and, for
character/block devices, major:minor against the requested values.

Attach the validated device through the kernel's mount API
(open_tree()/mount_setattr()/move_mount(); no glibc wrappers exist for
these yet, so three small syscall() wrappers are added) rather than
bind-mounting the /proc/self/fd/%d magic symlink: open_tree() clones a
detached mount of the held descriptor, mount_setattr() sets
MOUNT_ATTR_RDONLY on that detached tree before it is ever attached
anywhere, and move_mount() attaches it directly by file descriptor.
This does not depend on /proc being mounted or trustworthy at the
point the device is attached - relevant since the whole point of this
jail is running under an unreliable or masked /proc - and there is no
window where the mount is attached but not yet read-only. The mount
queue (struct mount, add_mount()) gains a source_fd field and a
matching add_mount_fd(); entries with a valid fd are attached via
move_mount() in mount_all() through a new do_mount_fd(), instead of
going through the existing path-based do_mount().

struct mount_attr's fields are __u64 in the kernel ABI; since unsigned
long is only 64-bit on some target architectures and a 32-bit
mismatch would pass the wrong size to mount_setattr(). Some of the
new mount-API flag constants (MOVE_MOUNT_F_EMPTY_PATH, AT_EMPTY_PATH,
in addition to the already-guarded OPEN_TREE_* and MOUNT_ATTR_RDONLY)
are missing from musl's headers, so all of them get a local fallback
definition guarded by #ifndef.

A missing mandatory custom device aborts startup; a missing default
device is skipped with a warning. Use gid=0 for devpts under
CLONE_NEWUSER instead of the unmapped host gid 5.

Signed-off-by: Joshua Covington <joshuacov@gmail.com>
@joshuacov1
joshuacov1 force-pushed the ujail-fixes branch 3 times, most recently from ff6429f to 9e794cf Compare August 3, 2026 20:02
getpwnam()/getgrnam() are not reentrant. uid/gid map file
descriptors are opened without O_CLOEXEC. dprintf()'s return value is
checked as a boolean, so a successful write (a positive byte count) is
treated as a failure. Map-write failures are discarded without being
checked. A named user's supplementary groups are never resolved; where
one of them is host gid 0, the gid_map remaps it to a free inner id,
but setgroups() is called with the raw host gid array, so the mapped
identity and the applied supplementary groups disagree. post_start_hook()
re-derives and reapplies identity a second time, which fails under
CLONE_NEWUSER since only namespace uid 0 is mapped into the new
namespace, not the raw host uid. A precheck for -U/-G in main()
resolves identity against the host passwd/group database
unconditionally, but extroot jails resolve identity in
post_start_hook() after pivot_root, against the extroot's own
/etc/passwd, so a user defined only there is rejected before startup
even though that is exactly what -R is for.

Use getpwnam_r()/getgrnam_r(), distinguishing "no such user"/"no such
group" (a NULL result with no error) from a genuine errno-bearing
failure. Add O_CLOEXEC to the map/setgroups file descriptors. Fix the
dprintf() check to test for a negative return. Check every
map-write's return value and abort on failure instead of continuing
with a half-applied identity. Resolve supplementary groups with
getgrouplist() and build the corresponding inner gid map through a
single, deterministic compute_inner_gids() helper, called
independently by the parent (building the gid_map) and the child
(calling setgroups(), across the fork/clone boundary) so both sides
compute the same inner ids from the same inputs without passing state
between them. Skip the redundant identity re-derivation in
post_start_hook() when the uid/gid map already came from
CLONE_NEWUSER. Restrict the -U/-G precheck in main() to non-extroot
jails.

Signed-off-by: Joshua Covington <joshuacov@gmail.com>
A remount used to enforce security flags (ro/nosuid/nodev/noexec)
can return EPERM even when those flags already hold - for example on
a host-owned bind source under CLONE_NEWUSER - and treating that as
fatal breaks configurations that are already correctly locked down.
Mountpoint comparisons for this check use a plain strcmp() against
mountinfo fields; mountinfo(5) escapes space, tab, newline, and
backslash as octal \NNN sequences, so any mountpoint containing one
of those bytes never matches.

Add mountinfo_unescape() and mountinfo_current_flags(), which
unescape and read the kernel-enforced flags for a mountpoint from
/proc/self/mountinfo using getline() (no line-length limit), keeping
the last matching entry: with stacked mounts, the last entry is the
topmost, currently-effective one. On EPERM from a remount, retry with
those actual flags merged in; if that still fails, tolerate the EPERM
only if the security-relevant flags already in effect satisfy the
original request, otherwise fail for a critical mount and warn for a
non-critical one.

Signed-off-by: Joshua Covington <joshuacov@gmail.com>
An OCI bundle can bind-mount a writable host directory over /proc
or /sys, undermining process/kernel-interface isolation regardless of
any other mount hardening in place.

Add is_proc_or_sys_path() and reject an OCI mount request whose
destination is /proc, /sys, or a path under either, when the mount is
a bind mount and not read-only (MS_RDONLY). Scope this to bind mounts
specifically: the OCI runtime-spec default for a container's /proc,
and what runc/Docker/podman emit, is a fresh procfs mount with no "ro"
option and type "proc", not "bind"; maskedPaths/readonlyPaths are the
spec's mechanism for restricting the sensitive parts of that mount,
and ujail's own /proc mount is not MS_RDONLY either.

Detect the bind mount via the parsed MS_BIND flag (set by
parseOCImountopts() whenever "bind"/"rbind" appears in the mount's
"options", independent of its "type"), not by string-comparing "type"
against "bind": a spec using type:"none" with options:["bind","rw"] is
a real-world OCI spelling that a type-string-only check misses
entirely, since parseOCImountopts() sets MS_BIND from "options" alone.
Still also check the type string directly, to keep catching a spec
that sets type:"bind" without "bind" appearing in "options".

Signed-off-by: Joshua Covington <joshuacov@gmail.com>
@joshuacov1
joshuacov1 force-pushed the ujail-fixes branch 2 times, most recently from 79f1958 to 83eb6a9 Compare August 3, 2026 22:35
Plain CLI jails do not mask sensitive /proc,/sys paths such as
/proc/kcore, /proc/sysrq-trigger, /sys/firmware, /proc/sched_debug,
etc.; this masking happens only along the OCI mount-spec path,
leaving every non-OCI jail with unrestricted access to them.

Add proc_mask_critical[], proc_mask_optional[] and sys_mask_critical[]
path lists and mask_default_paths(), which masks each path with a
0-sized tmpfs or bind-mounted empty file via the existing do_mount()
masking mechanism. Call it for non-OCI jails right after mounting
/proc,/sys: failing to mask a critical path aborts startup, failing
to mask an optional path only warns, since not all kernels/configs
expose every optional path. Switch the masking mounts in do_mount()
from hardcoded MS_NOATIME to MS_RELATIME, matching the kernel's
actual atime default and avoiding an atime-class mismatch against the
host's existing /proc,/sys instance.

A jail that defers creating its own CLONE_NEWUSER applies this
masking, and the separate always-on /proc/sys read-only self-bind,
while still in the initial user namespace, so these mounts get copied
into the jail's own mount namespace locked once it regains one of its
own - a locked mount can't be replaced or detached by that same,
now less-privileged process, since doing so needs privilege in the
namespace that owns the mount. A locked mount under /proc also makes
the kernel's mount-visibility check (mount_too_revealing()) refuse a
nested runtime's own /proc mount inside the jail, since it requires
every locked child under a candidate reference /proc to cover only a
permanently-empty directory, which none of these masked paths are.

Defer this masking and the /proc/sys self-bind to a second pass
instead, for jails that defer their own CLONE_NEWUSER: mask_path_now()
and remount_proc_sys_after_unshare() apply the exact same masks and
lock directly, once the jail has regained a mount namespace owned by
its own user namespace. Mounts created there aren't locked, since no
privileged-to-unprivileged copy is involved, so a nested runtime's own
/proc mount succeeds while the masked paths stay exactly as
inaccessible as before. A jail that never creates its own user
namespace has no second pass and no such concern, so it keeps applying
this masking immediately, as before.

UJAIL_NOAFILE is created host-side, pre-pivot_root(), so it is
unreachable from the post-pivot context this second pass always runs
in. A 0-mode empty file gives a stricter, self-diagnosing mask than
/dev/null (open() fails outright, instead of quietly succeeding on
every read/write); build_jail_noafile() creates a second instance at
JAIL_NOAFILE, reachable from inside the jail, before the deferred
CLONE_NEWUSER, and mask_path_now() binds that instead.

The /proc/sys/net self-bind dance mirrors phase 1's ordering, so the
closing MS_MOVE has a mountpoint to move from.

An OCI bundle's own maskedPaths/readonlyPaths need the same deferral
as the above whenever the bundle also defers its own CLONE_NEWUSER,
for the same locked-mount reasoning.

build_jail_noafile() anchors JAIL_NOAFILE in a locked, read-only
tmpfs: ownership alone can't survive an explicit 0->0 uidMapping.

Signed-off-by: Joshua Covington <joshuacov@gmail.com>
ujail's construction-time ELF-dependency walker (elf.c) resolves a
scanned binary's DT_NEEDED entries only against a fixed global search
list: /lib, /lib64, /usr/lib, plus /etc/ld.so.conf. It does not read a
scanned object's own DT_RPATH/DT_RUNPATH tags. A binary that
legitimately relies on RPATH/RUNPATH to select its own bundled library
over a system library sharing the same SONAME needs no
/etc/ld.so.conf entry to run outside a jail, but needs one anyway just
so ujail's walker can find and stage the same library.

For each object scanned, read its DT_RPATH/DT_RUNPATH tags in the same
pass already used for DT_STRTAB, and register their directories ahead
of the base search list before processing the object's own DT_NEEDED
entries. Check the object's own DT_RUNPATH scope before the
inherited (parent) DT_RPATH scope, matching runtime-linker precedence.
DT_RUNPATH supersedes DT_RPATH entirely on the same object, per
standard ELF semantics. "$ORIGIN"/"${ORIGIN}" expands to the directory
containing the object that specified it; fs.c keeps each resolved
dependency's real directory available for this rather than discarding
it once the soname lookup completes, so $ORIGIN resolves correctly
for every object in the tree.

RPATH/RUNPATH directories are deduplicated against their own scope
lists, since an object's own RPATH/RUNPATH takes priority over generic
system paths. DT_RUNPATH is non-transitive: it affects resolving only
the direct DT_NEEDED entries of the object that declares it. DT_RPATH
is transitive: it stays visible for the rest of the declaring
object's own subtree, popped again once that subtree finishes, and is
never added to the permanent library search list. Two separate scope
lists, rpath_scope and runpath_scope, are saved and restored around
each object's own DT_NEEDED loop accordingly. This transitive-RPATH
model matches glibc; musl's own ldso/dynlink.c stores DT_RPATH and
DT_RUNPATH into a single field (DT_RUNPATH overwriting DT_RPATH when
both are present) and does not implement a transitivity distinction
between them, resolving both identically per object. The
glibc-compatible behavior is used here, matching what most toolchains
people build against do.

Signed-off-by: Joshua Covington <joshuacov@gmail.com>
Joining an external user namespace (-j, opts.setns.user) always
calls setgroups(0, NULL) to drop supplementary groups before root
inside becomes usable. Writing a gid_map for a user namespace requires
setgroups=deny to have been set for that namespace first - an
unconditional kernel rule, independent of privilege level - and that
denial is permanent for the lifetime of the namespace: no process,
however privileged, can call setgroups() in that namespace again once
a gid_map has been written this way. Any external namespace with a
genuinely working gid_map (needed for setregid()/setreuid() to succeed
at all) therefore has setgroups permanently denied, and the
unconditional setgroups(0, NULL) call here always fails against it
with EPERM, aborting the join.

Tolerate EPERM specifically, with a warning, and continue without
dropping supplementary groups in that case; any other error remains
fatal as before.

Signed-off-by: Joshua Covington <joshuacov@gmail.com>
dangowrt and others added 3 commits August 12, 2026 10:03
ujail masks file paths such as /proc/kcore by bind-mounting a 0-byte
mode-0000 file over them, so any access fails with an error instead of
being silently swallowed the way a /dev/null bind would. Jails which
defer their own user namespace re-apply these masks from inside the
container; a mask source file created by the jail child itself is owned
by the very host uid container root maps to, since the child runs with
euid root_map_uid, and container root can therefore chmod or unlink it.

Create a single canonical mask source at boot instead, right after
mounting /tmp: a 0-byte mode-0000 file on a dedicated 4k tmpfs at
/tmp/.ujail, remounted read-only at superblock level. The superblock
belongs to the initial user namespace, so no user namespace can ever
remount it read-write; chmod, unlink and write fail with EROFS through
every path, including fresh bind mounts created from inside a
container, and that holds even when host root is mapped into the
container. Creating the file once during early boot also means jails
starting in parallel never need a concurrent create-if-missing dance.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
Every deferred-userns jail currently constructs its own mask source in
build_jail_noafile(): a directory under its /dev, a dedicated tmpfs
mount, a setfsuid() dance to create the file as host root and a
read-only remount to lock the superblock, repeated by every such jail
and leaving each one an extra tmpfs superblock just to hold a single
immutable empty file.

Drop build_jail_noafile() and instead bind procd's canonical noafile,
created once at boot on a read-only tmpfs superblock owned by the
initial user namespace, to JAIL_NOAFILE inside the jail. The bind is
queued through the regular mount list, so it is set up while still
privileged, survives pivot_root, and arrives in the container's own
mount namespace locked. Every modification attempt from inside then
fails: chmod, unlink and write with EROFS on the read-only superblock
no user namespace can remount, umount with EINVAL on the locked mount,
and this holds for any uid mapping. The queued mount uses a fatal error
level, so a jail which will re-apply masks in phase 2 refuses to start
when the noafile is missing instead of silently degrading to unmasked
files.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
The final MS_REMOUNT | MS_RDONLY in early_noafile() was unchecked. A
failure there leaves /tmp/.ujail silently read-write, so every jail
then masks against a weaker, uid-mapping-dependent noafile with
nothing logged anywhere.

Signed-off-by: Joshua Covington <joshuacov@gmail.com>
@joshuacov1

Copy link
Copy Markdown
Contributor Author

@dangowrt
Thank you for your input.

Your fix now obsoletes build_jail_noafile() from patch 9. This is cleaner than what I had and queuing the bind in phase 1 avoids a directory-permission workaround my phase-2 version needed.

One small gap in early_noafile(): the final MS_REMOUNT | MS_RDONLY mount's return value isn't checked at the end. If that specific remount fails, then /tmp/.ujail silently stays read-write, so early_noafile() returns normally either way and every jail on the system then masks against a weaker (uid-mapping-dependent) noafile with nothing logged anywhere.

I pushed a small fix for that on top that checks the return value and logs via ERROR() on failure, which matches the existing check on the first mount call earlier in that function.

@joshuacov1

Copy link
Copy Markdown
Contributor Author

There is something I think should be pointed out not as an objection, but for the record:

In the first version with build_jail_noafile() any jail invocation could construct its own noafile from nothing and was thus self-contained. The adjustment now requires procd's own early_mounts() to have already run this boot. This is generally a low-risk for real OpenWrt because procd genuinely is PID 1. However if /tmp/.ujail ever gets corrupted or removed at the host level, it would also take down masking for every jail on the system simultaneously. In the previous version it would have cost one jail per failure.

On the win-side now we are reusing the existing locked-mount part instead of a previous function and the bind gets the same automatic MNT_LOCKED-on-copy treatment every other phase-1 mount gets. This is less duplicated work across many jails.

@openwrt-bot
openwrt-bot merged commit debe838 into openwrt:main Aug 15, 2026
1 check failed
@dangowrt

Copy link
Copy Markdown
Member

I've merged it as is now, follow-up changes are welcome, of course -- in any case, it's an improvement already. Thank you!

@joshuacov1
joshuacov1 deleted the ujail-fixes branch August 19, 2026 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants