Fix and test the safe_openat fallback used without openat2 - #2155
Open
kolyshkin wants to merge 5 commits into
Open
Fix and test the safe_openat fallback used without openat2#2155kolyshkin wants to merge 5 commits into
kolyshkin wants to merge 5 commits into
Conversation
When the last resolved component is a relative symlink, the code backs up over it with while (*(--new_path) != '/'); leaving new_path pointing at the separator itself, so the expanded symlink target overwrites it. "<root>/dir/link" with "link" pointing to "file" resolves to "<root>/dirfile" instead of "<root>/dir/file", and "<root>/link" resolves to "<root>link". Top level symlinks happen to survive since safe_openat_fallback() strips the rootfs prefix and then consumes leading slashes, but any relative symlink below the first level resolves to a path that does not exist, and the open fails with ENOENT. Keep the separator after backing up over the component. This is only reachable through the safe_openat() fallback, i.e. on kernels without openat2(2) or when a seccomp filter blocks it. Add unit tests for symlink expansion, which was not covered at all. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
chroot_realpath() resolves the last component as well, so the fallback used when openat2(2) is not available silently follows a symlink even when the caller passed O_NOFOLLOW. A "dest-nofollow" bind mount whose destination is a symlink is then created on the symlink target instead of on the symlink itself, and a dangling destination symlink makes the container fail to start. When O_NOFOLLOW is set, resolve only the parent directory and let openat(2) deal with the last component. A trailing '/' still forces the symlink to be resolved, as the kernel does. While at it, drop the rootfs prefix only when it is actually present: chroot_realpath() returns the path unchanged when rootfs is "/", and the unconditional "path_in_chroot += rootfs_len" removed the first character of the path. With the parent directory now possibly being the empty string, that also read past the end of the buffer. Document that dirfd must be a file descriptor for rootfs itself. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The safe_openat() fallback is only used on kernels older than 5.6 or where a seccomp filter blocks openat2(2), so it is never exercised by CI, and it silently diverged from the openat2 path. Add tests/no_openat2, a small helper installing a seccomp filter that makes openat2 fail with ENOSYS and then executing its arguments. The filter is inherited across exec and by every child, so pointing TESTS_ENVIRONMENT at it runs the whole suite through the fallback. Add a "make check-no-openat2" target using it, and run it in CI. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
check_fd_under_path() requires a '/' right after the rootfs prefix, so when the rootfs is "/" it rejects every path: for "/proc" the character after the prefix is 'p', and the open fails with "target `/proc` not under the directory `/`". This is reached through the safe_openat() fallback, i.e. on kernels without openat2(2), for containers using the host root, as done for the mounts set up by libcrun_container_enter_cgroup_ns() and by the krun handler. Return early when the rootfs is "/" or not set: every path is under it, so there is nothing to verify. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The krun handler passes a NULL rootfs together with AT_FDCWD for containers without a rootfs of their own, and libcrun_create_dev() and libkrun_read_vm_config() then call safe_openat() with it. The fallback used when openat2(2) is not available calls strlen() on it and crashes, and the empty path case passes it to open() directly. Treat a NULL or empty rootfs as "/", which is what chroot_realpath() already does. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Ephemeral COPR build failed. @containers/packit-build please check. |
1 similar comment
|
Ephemeral COPR build failed. @containers/packit-build please check. |
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
2 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.
The
safe_openat()fallback, used on kernels withoutopenat2(2)(pre 5.6)or where a seccomp filter blocks it, has silently diverged from the
openat2path: nothing in CI ever runs it. This series fixes four bugsfound there and adds a way to keep the path tested.
The last commit adds
tests/no_openat2, a small helper installing a seccompfilter that makes
openat2fail withENOSYSand then executing itsarguments. The filter is inherited across exec and by every child, so
pointing
TESTS_ENVIRONMENTat it runs the whole suite through thefallback:
make check-no-openat2. This is what found the first two bugs.It is run as root only, since without
CAP_SYS_ADMINthe helper has to setNO_NEW_PRIVSto install the filter and the test containers inherit it,which breaks the
noNewPrivilegestests.The fixes, in order:
chroot_realpathdrops the separator when it expands a relativesymlink, so
<root>/dir/linkwithlink -> fileresolves to<root>/dirfile. Top level symlinks survive by accident, anythingbelow the first level resolves to a path that does not exist.
Symlink expansion had no test coverage at all.
The fallback follows the last component even when
O_NOFOLLOWwasrequested, since
chroot_realpathresolves it. Adest-nofollowbindmount onto a symlink is created on the symlink target, and a dangling
destination symlink makes the container fail to start. This is what
mount-bind-mount-symlink-nofollowcatches onceopenat2is blocked.check_fd_under_path()expects a/right after the rootfs prefix, sowith rootfs
/it rejects every path.safe_openat()callsstrlen()on the rootfs, which the krun handlerpasses as NULL for containers without a rootfs of their own.
Fixes 3 and 4 are only reachable through the fallback as well.
Tested with
make checkand with the suite run through the fallback; bothgive identical results now.
This may be the underlying cause of #2154, which reports device creation
failing on a 4.14 kernel. The mechanism described in that issue does not
hold up (
chroot_realpathreturns the partially resolved path onENOENTrather than NULL, and stripping the rootfs prefix from an unresolvable
relative path gives back the original path), but bugs 1 and 2 above produce
exactly this class of failure on a kernel without
openat2. I have notbeen able to reproduce the reported error message itself.