utils: use fdinfo as final fallback for fetch_mnt_id #212
Conversation
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
cyphar
force-pushed
the
fdinfo-mnt-id
branch
2 times, most recently
from
c9b81d6 to
60f3cfb
Compare
cyphar
force-pushed
the
main
branch
2 times, most recently
from
6c9b339 to
07958b3
Compare
cyphar
force-pushed
the
fdinfo-mnt-id
branch
5 times, most recently
from
b8be9d6 to
9b45ce9
Compare
In a future patch we will end up expanding the amount of logs we get from "make test", which will make GitHub Actions unhappy: System.IO.IOException: No space left on device: '/home/runner/actions-runner/cached/_diag/Worker_20250710-090101-utc.log' So we need to be more aggressive about cleaning up unused space... Signed-off-by: Aleksa Sarai <[email protected]>
In a future patch we will need to verify something is the root of a procfs without using ProcfsHandle::try_from_fd(), and so it makes sense to just abstract this into a helper. Signed-off-by: Aleksa Sarai <[email protected]>
FromStr is implemented for String, so we can just use the same str::parse function with String to get back the same string value. In addition to nicely unifying the logic, This feels a bit more ergonomic as well. The only downside is that FromStr::Err is Infallible in that impl (for obvious reasons), so we need to add a dummy entry to ErrorImpl to make the types happy. This variant can never actually be constructed, for the same reason Infallible cannot be constructed. Signed-off-by: Aleksa Sarai <[email protected]>
Rather than passing around Option<BorrowedFd<'_>> between ProcfsHandle helper functions, we should have a separate type to better indicate that this is a bit of a compromise solution. This should also make it harder to accidentally pass the wrong file descriptor as the wrong argument when we extend RawProcfsRoot usage. Signed-off-by: Aleksa Sarai <[email protected]>
/proc/self/fdinfo includes very useful information -- most notably the mnt_id for any file handle. This will allow us to fetch the mnt_id of open files even on kernels without STATX_MNT_ID. The alternative approach would be to parse /proc/self/mountinfo, but /proc/self/fdinfo is harder for an attacker to fake because we also check that the "ino" field matches the actual inode number we get from fstat(2). Thwarting this would require the attacker to be able to correctly and consistently guess the inode number of the file being operated on, as well as the fd being targeted, as well as convincingly creating fake fdinfo files using fairly restrictive gadgets like /proc/self/environ. A sufficiently smart attacker with good knowledge of the procfs access patterns of the victim process might be able to do it, but the attack also needs to be very active because most inode numbers are not very stable in procfs. /proc/self/mountinfo is much easier to attack (if you just set it to a static file without overmounts, the overmounts are invisible). In addition, /proc/self/fdinfo works with detached mount fds (a-la fsopen(2) -- which is relevant for AT_RECURISVE trees due to locked mounts), and is _much_ easier to parse than /proc/self/mountinfo. Signed-off-by: Aleksa Sarai <[email protected]>
Now that we have a hardened /proc/thread-self/fdinfo handler, we can use this as a final fallback when looking for the mount id of a path. Every "struct file" in the system has an associated "mnt_id" in fdinfo, which is a huge benefit. This is not an ideal solution, but it should be quite difficult for an attacker to bypass, and it critically allows us to provide complete security for post-openat2-but-pre-STATX_MNT_ID kernels. Signed-off-by: Aleksa Sarai <[email protected]>
Now that we have the /proc/thread-self/fdinfo fallback, we need to test it by disabling STATX_MNT_ID. Unfortunately, some of our core procfs tests overmount /proc/self/fdinfo, so we need to update those. Note that (unlike openat2), it really isn't necessary to re-run the entire test suite when we disable statx -- all we really care about is the procfs stuff. Signed-off-by: Aleksa Sarai <[email protected]>
Signed-off-by: Aleksa Sarai <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Signed-off-by: Aleksa Sarai [email protected]