Repository navigation
cp: keep host-to-container writes inside the container mount - #5336
Open
danielcho02 wants to merge 1 commit into
Open
danielcho02 wants to merge 1 commit into
danielcho02 wants to merge 1 commit into
Conversation
The destination was resolved in userspace and then passed as a host path string to os.Mkdir and to `tar -x -C`, which resolve it again. A process in the container could replace a component with a symlink in between; absolute symlinks under /proc/<pid>/root resolve against the host root, so the write could land on the host. Record the mount holding the destination and the path relative to it. openExtractDir opens the mount with os.OpenRoot, creates a missing destination with Root.Mkdir and opens the directory tar extracts into with O_PATH; tarExtractCmd hands that descriptor to tar (ExtraFiles, -C /proc/self/fd/3) instead of a path. A single-file bind mount (e.g. /etc/hosts) is opened on its parent directory, which is where tar already wrote. A mount whose source is not a path (e.g. /dev, source "tmpfs") is refused; its source used to be resolved against the working directory, so copying to /dev/foo from a directory containing tmpfs/ wrote ./tmpfs/foo on the host. The container-to-host direction is unchanged. Errors from creating or opening the destination now name the path relative to the mount, and tar errors show -C /proc/self/fd/3. os.Root needs read as well as search permission on the directories it walks, which matters only to a caller without CAP_DAC_OVERRIDE (e.g. a 0333 intermediate directory now fails with permission denied). Signed-off-by: Minkyu Cho <chominkyu3290@gmail.com>
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
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.
nerdctl cp <host> <container>:<path>resolves the destination in userspace (resolvePath), then hands the resulting host path string (/proc/<pid>/root/..., or a volume's host path) toos.Mkdirand totar -x -C. Both resolve that string again in the kernel. If a process in the running container replaces a path component with a symlink in between, the write follows it, and absolute symlinks under/proc/<pid>/rootresolve against the host root, so the file lands on the host.This keeps host-to-container writes inside the mount that holds the destination:
getPathSpecFromContainerrecords the mount's host path and the destination's path relative to it.openExtractDiropens that mount withos.OpenRoot, creates a missing destination withRoot.Mkdir, and opens the directory tar extracts into withRoot.OpenFile(..., O_PATH|O_DIRECTORY).tarExtractCmd, used byCopyFiles, hands that handle to tar throughExtraFilesas-C /proc/self/fd/3, so no path is left for tar to resolve again.No new dependency (
os.OpenInRootis already used inpkg/cmd/container/run_mount.go). The container-to-host direction is unchanged, and so is the documented warning thatnerdctl cp"is designed only for use with trusted, cooperating containers"; this only narrows the host-to-container case.Details for copies into the container:
ErrFilesystem, but now come fromos.Rootand name the path relative to the mount, e.g.mkdirat data/x/proof: no such file or directoryrather thanmkdir /proc/<pid>/root/data/x/proof: .... Likewise, tar errors and the debug log showtar -x -C /proc/self/fd/3 -f -instead of the host path.os.Rootopens each directory on the way to the destination, so it needs read as well as search permission on them, where path strings needed search only. This only matters for a caller withoutCAP_DAC_OVERRIDEover those directories: as a non-root user, with an intermediate directory of mode0333,mkdirandtar -Cby path succeed whileRoot.MkdirandRoot.OpenFilefail withpermission denied. A directory that cannot be searched is not a new case: resolving the destination already fails there withErrFilesystem(lstat ...: permission denied), before and after this change, so the destination is never created or opened./etc/hostsis opened on its parent directory on the host, which is where tar already wrote./devhas sourcetmpfs,/dev/shmhasshm), the source name used to be resolved against nerdctl's working directory. Usually that fails withdestination parent does not exist, before and after this change. But run from a directory that containstmpfs/,nerdctl cp f c:/dev/foosucceeded and wrote./tmpfs/fooon the host.openExtractDirnow refuses such a mount withcannot copy into mount with non-path source "tmpfs".Testing
pkg/containerutil/cp_linux_test.go, on a directory standing in for/proc/<pid>/rootwith a volume:getPathSpecFromContainerrecords the mount;openExtractDiropens the right directory, creates a missing destination inside the mount, and opens a file bind mount on its parent.openExtractDirfails and nothing is created outside, for an existing destination, a created one, and a file copied to a new name.tarExtractCmdextracts into the handle after the directory has been swapped for a symlink, and without a handle builds the same-C <dir> -f -command as before.mkdiror the open to path strings, dropping the handle from the tar command, or giving tar the path, makes these tests fail. They pass as root and as a non-root user, also withTMPDIRbehind a symlink. The calls inCopyFilesare not unit-tested: the integration tests show copies still work through them, and the live race below that they keep writes inside.go test ./cmd/nerdctl/container -run TestCopypasses: 201 passed (3 tests, 198 subtests), 0 failed, 1 skipped (rootful, Ubuntu on WSL2, GNU tar 1.35)./data/xbetween a directory and a symlink to a host-only directory, 300 ×nerdctl cp /tmp/payload cpf:/data/x/proof. Across six runs on successive revisions of this branch, the unpatched binary wrote 7, 1, 1, 2, 1 and 2 files on the host and the patched one none; the last run is on this revision. Escapes are rare per attempt, so the unit tests carry the weight. While the container keeps flipping, the patched copies all fail, mostly withdestination parent does not exist, otherwise with tar exiting with status 2 or themkdirfinding no parent; copies into containers that do not do this succeed as before.Scope: this pins the directory tar extracts into and the
mkdir. Paths below it are resolved by tar itself. With an archive membersub/file.txtandsuba symlink in the pinned directory, GNU tar 1.35 refuses (Cannot open: Invalid cross-device link) when the symlink is absolute or leads out of the directory, and follows it when it is relative and stays inside. Other tar implementations were not checked.Checked by hand outside nerdctl: an
O_PATH|O_DIRECTORYdescriptor passed as fd 3 throughnsenter -t <pid> -U --preserve-credentials -- tar -x -C /proc/self/fd/3, into a user namespace created withunshare -U -r, extracted into the pinned directory after it had been swapped for a symlink.Not tested: a rootless nerdctl run end to end, and tar 1.30-13 on AlmaLinux 8 (the
getcwdfailure in #5237, worked around by #5238); tar still opens the-Cdirectory itself here.