sysroot permit ENOENT and ENOTDIR for containment checks on absent paths#212
sysroot permit ENOENT and ENOTDIR for containment checks on absent paths#212doanbaotrung wants to merge 1 commit into
Conversation
860a5a9 to
33fc1df
Compare
There was a problem hiding this comment.
Review completed against the latest diff
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Looking at the callers, containment is invoked only after sysroot_path_exists(), after The issue’s /etc/sysusers.d/basic.conf example also does not take this path today: when the Please provide a concrete reproducer that reaches this fallback, or clarify that the change only |
When a path component (like a parent directory) does not exist, realpath fails with ENOENT or ENOTDIR. Previously, this failure was mapped to a containment check violation, which then aborted the syscall with ELOOP. We now recursively walk up the directory tree to find the closest existing parent, check its containment, and permit the syscall if the parent is contained. This ensures guest programs receive ENOENT rather than ELOOP on absent parent paths. Fix sysprog21#187
Actions Completed
|
jserv
left a comment
There was a problem hiding this comment.
The new walk-up in sysroot_path_is_contained fires only under the single-window TOCTOU that the module comment (proc-state.c:469) already declares out of scope, so the actionable issue is the fail-open regression noted inline, not the race itself. Two lower-priority items without their own anchor: str_copy_trunc in realpath_existing_parent (:448) silently truncates an over-long path and could authorize a different, shorter contained path — security code should fail closed with ENAMETOOLONG; and the test's rmdir(race_dir) cleanup can leave residue (ENOTEMPTY) if a race iteration created file.txt inside.
| return false; | ||
| if (!realpath(parent, real_path)) { | ||
| if (errno != ENOENT && errno != ENOTDIR) | ||
| return false; |
There was a problem hiding this comment.
When realpath(parent) fails and realpath_existing_parent climbs more than one level (a missing intermediate component, not just the leaf's parent), the snprintf below reattaches only the final basename (slash + 1) onto the shallower ancestor. For ${sr}/a/b/c with b missing it validates ${sr}/a/c, not the ${sr}/a/b/c the syscall runs on — the middle is silently dropped. This authorizes the wrong object even without an adversary, and converts a case that previously failed closed (returned false -> ELOOP) into fail-open: if b is later created as a symlink out of the sysroot, the syscall escapes. Either keep the fail-closed return when realpath(parent) fails, or validate the entire stripped remainder (reject .., confirm the full reconstructed path is prefix-contained) rather than reattaching one component.
| mounted_sysroot_root); | ||
| snprintf(race_file, sizeof(race_file), "/tmp/race_dir/file.txt"); | ||
|
|
||
| pid_t pid = fork(); |
There was a problem hiding this comment.
fork()'s return isn't checked. On -1 the child branch is skipped and kill(pid, SIGKILL) becomes kill(-1, SIGKILL) — SIGKILL to every process the user can signal, then waitpid(-1, ...) reaps an arbitrary child. Add if (pid < 0) FAIL(...) before the split.
| int hit_enoent = 0; | ||
| int hit_eloop = 0; | ||
| int iterations = 0; | ||
| while (hit_enoent == 0 && iterations < 50000) { |
There was a problem hiding this comment.
The test only PASSes if it wins the race and observes ENOENT, so it's flaky on a single core or loaded CI (hit_enoent stays 0 -> spurious FAIL). It also asserts only the errno mapping, never that nothing was created outside the sysroot — so it can't catch the fail-open case in the parent-only branch. Make the ENOENT-mapping check deterministic (open through a statically missing intermediate, assert ENOENT and not ELOOP), keep the racer as a non-fatal stress pass, and add an assertion that no artifact exists outside mounted_sysroot_root.
sysroot permit ENOENT and ENOTDIR for containment checks on absent paths
When a path component (like a parent directory) does not exist,
realpath fails with ENOENT or ENOTDIR. Previously, this failure
was mapped to a containment check violation, which then aborted the
syscall with ELOOP.
We now recursively walk up the directory tree to find the closest
existing parent, check its containment, and permit the syscall if
the parent is contained. This ensures guest programs receive ENOENT
rather than ELOOP on absent parent paths.
Fix #187
Summary by cubic
Fix sysroot containment so missing or non-directory path components no longer trigger ELOOP. We now resolve the nearest existing parent to validate containment and let the syscall return ENOENT or ENOTDIR as expected.
Written for commit e624e3e. Summary will update on new commits.