imaginator: Fix symlink resolution for copy and append to stay within image rootfs#254
Conversation
…inting and formatting" This reverts commit a17ccda.
…tive and absolute targets
rgooch
left a comment
There was a problem hiding this comment.
This adds a lot of code and more complexity. Consider a simpler approach, such as walking the source and destination trees and failing if there is a symlink in the way.
| // AppendFile is not safe to call concurrently for the same file. | ||
| func AppendFile(destFilename, sourceFilename string) error { | ||
| return appendFile(destFilename, sourceFilename) | ||
| func AppendFile(destDir, destFilename, sourceFilename string) error { |
There was a problem hiding this comment.
This changes the library API in a non-backwards compatible way, which we can't do because this API has already been published (merged).
6c158b7 to
29a9ffb
Compare
29a9ffb to
f90c09e
Compare
Thanks @rgooch for the review. I agree the custom resolver adds too much complexity. I looked into failing on destination symlinks, just like we do for source paths. But that makes the builder too restrictive. We do not control the source image file structure. Failing on destination symlinks breaks standard Linux setups like:
To support these safely without letting writes escape to the host, we need to do two things:
Custom path resolution in userspace takes a lot of code and testing. Instead of the custom Go resolver, we can use the openat2 syscall with the RESOLVE_IN_ROOT flag. The Linux kernel enforces the boundary natively. This drastically reduces our code complexity. It only requires Do you want to pivot to openat2 (https://man7.org/linux/man-pages/man2/openat2.2.html) to keep the code simple? Or should we stick with the userspace resolver to avoid the kernel version limit? Let me know. |
Description & Motivation
This PR enhances a critical symlink resolution edgecase in the merge paths used by
copy(files,post-install-files) andappend(files.append,post-install-files.append).Previously, symlinks were not safely resolved with image-root semantics. This allowed malicious or malformed symlinks within image layers to escape the image rootfs (
destDir) and inadvertently overwrite or modify files on the host machine running the build.This PR introduces a userspace chroot path resolver that makes resolution consistently rootfs-relative for both final targets and intermediate directory components.
The Problem
The curent implementation suffered from two main escape vectors:
..or an absolute path starting with/would resolve against the host filesystem instead of the virtual image rootfs./var/lib/dockerwhere/var/libis a symlink to/tmp), the underlying Goosoperations would follow it out of thedestDirand operate on the host's/tmpdirectory.I successfully reproduced both vectors on the current binary: copy and append operations were successfully tricking the builder into creating/modifying files on the host's
/tmpdirectory instead of the<imageroot-fs>/tmpdirectory.The Approach
Symlink resolution now treats
destDiras a strict virtual root boundary throughout the entire traversal of the destination path.In practice:
destDir...segments are clamped to the image root (matching Linuxchrootsemantics).Why not Go 1.24
os.Root?I strongly considered using Go 1.24's native
os.Root, but it is fundamentally incompatible with standard Linux base images.os.Rootis designed for strict security sandboxing (like static web servers). If it encounters an absolute symlink, it strictly rejects it as an escape violation and returns a permission error. However, container image builders require absolute symlinks to function (e.g., modern distros where/bin -> /usr/bin).A custom userspace chroot resolver was required to safely clamp absolute symlinks to the root boundary without throwing errors. The resolution logic implemented here is heavily inspired by cyphar/filepath-securejoin, which was built to solve this exact same container escape vulnerability for
runc.Related to: #239
Type of Change
How Has This Been Tested?
files.appendandpost-install-files.appenddirectories. After successful creation, created a VM and verified the files natively.fsutilpath resolver.Testing Approach
Reproduced the buggy behavior with the existing binary, then verified the new implementation protects against all edge cases.
Tested successfully for Final-Target Symlinks:
....(Clamping verified)MAXSYMLINKS)Tested successfully for Intermediate Destination Directory Symlinks:
/var/run -> /runstyle parent symlink behaviorChecklist: