Repository navigation
Conversation
Adds rejectsDockerArchiveConfigPathEscapingImageDirectory covering the case where manifest.json has Config: "../config.json". The new test asserts convertIfNeeded throws with the expected escapes-image-directory message. Extends makeDockerArchive() with an optional config parameter so the canonical tests are unchanged. Per @kiwigitops review request on apple#1598. Signed-off-by: Matt Van Horn <mvanhorn@gmail.com>
|
@kiwigitops added the regression test in 841bd1f. |
Adds rejectsDockerArchiveLayerPathEscapingImageDirectory, which builds a
docker-archive fixture whose manifest.json declares a Layers entry with
an absolute path (/tmp/layer.tar). The test asserts the archiveMemberURL
guard in DockerArchiveConverter throws the 'docker archive member escapes
image directory' error rather than reading outside the tarball root.
Generalizes the makeDockerArchive fixture helper to accept a custom
layers list while preserving the existing default ('layer/layer.tar') so
the pre-existing tests are unchanged.
Addresses @kiwigitops review feedback on apple#1598.
Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
|
Added the absolute-path regression in 65b9bfe. Builds a docker-archive fixture whose manifest.json declares Generalized the The relative-path traversal case for |
jglogan
left a comment
jglogan
left a comment
There was a problem hiding this comment.
@mvanhorn Thanks for the contribution! I've assigned you to the issue, but I think there are a few problems with the PR. Unfortunately, we've been slammed with internal work and haven't had enough time to go through all of the PRs that have landed in the past months.
I'll follow up with more specific comments but the main thing is that for anything involving file traversal like this, we need to:
- Traverse the filesystem using file descriptor syscalls as opposed to the older path-based equivalents. The syscalls should default to use NOFOLLOW and CLOEXEC to prevent symlink attacks and avoid exposing unnecessary descriptors in subprocesses.
- When working with paths (prior to doing file descriptor traversal of a hierarchy), use System.FilePath and not URL or String. When building paths, make sure to use FilePath.Component which will prevent an adversary from injecting multi-component paths.
apple/containerization#957 was just merged and it expands FileDescriptorOps with all the utility functions that should be needed to implement this feature securely. I'm going to tag containerization and update container early next week.
This branch has not been deployed
3 participants
Type of Change
Motivation and Context
container image load -i <file>currently expects an OCI-layout tarball. The two most common toolchains in the wild that emit container tarballs (Jib for JVM projects,docker savefor Docker workflows) emit the docker-archive layout instead, so users hit a hard wall trying to load images they already have.I raised this in #1426 on April 29 and proposed the conversion-on-load approach there. This PR is that implementation: detect docker-archive in
ImagesService.load()and convert it to OCI in-process so the existing load path continues unchanged for OCI inputs and now also accepts docker-archive inputs without an extraskopeo/cranestep.Closes #1426
Testing
docker savetarball; verified both materialize correctly and existing OCI-tarball load is unchanged)Tests/ImagesServiceTests/DockerArchiveConverterTests.swiftcovers the converter on canonical Jib anddocker savefixtures plus the malformed-input paths)AI was used for assistance with the implementation. I read the AI contribution guidelines in CONTRIBUTING.md — I've reviewed the diff myself and use the change locally. I can walk through the file line by line if useful, but my read is that wouldn't add much beyond the diff and tests. Happy to work with whatever review style you prefer.