install/bootupd: chroot to deployment #1949
Conversation
When `--src-imgref` is passed, the deployed systemd does not match the running environnement. In this case, let's chroot into the deployment before calling bootupd. This makes sure we are using the binaries shipped in the image (and relevant config files such as grub fragements). We could do that in all cases but i kept it behind the `--src-imgref` option since when using the target container as the buildroot it will have no impact, and we expect this scenario to be the most common. In CoreOS we have a specific test that checks if the bootloader was installed with the `grub2-install` of the image. Fixes bootc-dev#1559 Also see bootc-dev#1455 Signed-off-by: jbtrystram <jbtrystram@redhat.com>
Signed-off-by: jbtrystram <jbtrystram@redhat.com>
Signed-off-by: jbtrystram <jbtrystram@redhat.com>
This make it a mountpoint so the chrooted `bootupd` process will be able to find the underlying filesystem when `/boot` is not on a separate partition
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces significant changes to the bootloader installation process, implementing a chroot mechanism for bootupd to ensure correct binaries from the target image are used, involving bind mounts for essential Linux API filesystems and the /boot directory. However, the bind-mount and unmount logic within install_via_bootupd contains critical security vulnerabilities. It does not protect against symbolic link following when mounting host API filesystems (/dev, /proc, etc.) into the deployment root or during cleanup. A malicious image could exploit this with symlinks to overwrite or unmount critical host directories, leading to full system compromise or a permanent Denial of Service. These operations must be hardened to be symlink-safe. Additionally, integration tests have been updated with more verbose logging, which is a good practice, and a minor improvement could be made to the unmounting flags for consistency and robustness.
crates/lib/src/bootloader.rs
Outdated
| // First off, we bind-mount target on itself, so it becomes a mount point and the chrooted | ||
| // `findmnt` calls are able to resolve the mount in the chroot | ||
| // See https://github.com/coreos/bootupd/issues/1051#issuecomment-3768271509 and following comments | ||
| rustix::mount::mount_bind(target_root.as_std_path(), target_root.as_std_path())?; |
There was a problem hiding this comment.
The code performs a bind-mount of target_root on itself without verifying if it is a symbolic link. If target_root is a symlink pointing to a critical host directory (e.g., /), this operation will bind-mount the host directory on itself. Subsequent unmount operations at the end of the function (line 191) would then unmount the host directory, leading to a critical Denial of Service. You should verify that target_root is not a symbolic link before mounting.
crates/lib/src/bootloader.rs
Outdated
| .join_os(src.strip_prefix("/").unwrap()); | ||
| tracing::debug!("bind mounting {} in {}", src, dest.display()); | ||
| // We mount them recursively so nested API FS get carried over | ||
| rustix::mount::mount_bind_recursive(src, dest)?; |
There was a problem hiding this comment.
This recursive bind-mount is vulnerable to symlink following. Since the content of target_root is derived from the container image being installed, an attacker can provide an image containing a symlink at dest pointing to a host directory (e.g., target_root/dev -> /etc). The mount_bind_recursive call will follow the symlink and mount the source API filesystem (e.g., /dev) over the host directory (/etc), leading to full system compromise or Denial of Service. Ensure that dest is not a symbolic link before mounting.
crates/lib/src/bootloader.rs
Outdated
| let mount = target_root | ||
| .join(dir.strip_prefix("/").unwrap()) | ||
| .into_std_path_buf(); | ||
| if let Err(e) = rustix::mount::unmount(&mount, UnmountFlags::DETACH) { |
There was a problem hiding this comment.
The unmount operation here uses UnmountFlags::DETACH on a path that could be a symbolic link. If an attacker-controlled deployment contains a symlink at this location pointing to a host directory, this call will follow the symlink and unmount the host directory from the system. This is a significant Denial of Service vector. Use O_PATH and validate the path components to ensure no symlinks are followed during cleanup.
crates/lib/src/bootloader.rs
Outdated
| if let Err(e) = rustix::mount::unmount( | ||
| target_root.join("boot").into_std_path_buf(), | ||
| UnmountFlags::empty(), | ||
| ) { |
There was a problem hiding this comment.
For bind mounts, UnmountFlags::DETACH is generally preferred for cleanup to ensure robustness, especially if there's a chance the mount might still be in use by some lingering process. Using UnmountFlags::empty() might fail if the mount is busy. Consider using UnmountFlags::DETACH here for consistency with the other bind mount cleanups.
This would make the cleanup more resilient to potential issues where the mount point might still be considered 'busy' by the kernel, even if the main process has exited the chroot.
| if let Err(e) = rustix::mount::unmount( | |
| target_root.join("boot").into_std_path_buf(), | |
| UnmountFlags::empty(), | |
| ) { | |
| if let Err(e) = rustix::mount::unmount( | |
| target_root.join("boot").into_std_path_buf(), | |
| UnmountFlags::DETACH, | |
| ) { |
crates/lib/src/bootloader.rs
Outdated
| if let Err(e) = | ||
| rustix::mount::unmount(&target_root.into_std_path_buf(), UnmountFlags::empty()) | ||
| { | ||
| tracing::warn!("Error unmounting target root bind mount: {e}"); |
There was a problem hiding this comment.
Similar to the comment above, for bind mounts, UnmountFlags::DETACH is generally preferred for cleanup to ensure robustness. Using UnmountFlags::empty() might fail if the mount is busy. Consider using UnmountFlags::DETACH here for consistency with the other bind mount cleanups.
| if let Err(e) = | |
| rustix::mount::unmount(&target_root.into_std_path_buf(), UnmountFlags::empty()) | |
| { | |
| tracing::warn!("Error unmounting target root bind mount: {e}"); | |
| if let Err(e) = | |
| rustix::mount::unmount(&target_root.into_std_path_buf(), UnmountFlags::DETACH) | |
| { |
1 participant
No description provided.