Disallow Dockerfile symlinks escaping build context#6885
Disallow Dockerfile symlinks escaping build context#6885mahendrarathore1742 wants to merge 3 commits into
Conversation
2ba89e0 to
f935293
Compare
nalind
left a comment
There was a problem hiding this comment.
This looks broadly correct to me. I tend to run conformance tests under podman unshare if I need to run them locally, but they're passing in CI. There are a couple of tweaks I'd like to see, though.
| assert.NoError(t, os.Mkdir(contextDir, 0o755)) | ||
| dockerfile := filepath.Join(tmpDir, "Dockerfile") | ||
| assert.NoError(t, os.WriteFile(dockerfile, []byte("FROM scratch"), 0o644)) | ||
| assert.NoError(t, os.Symlink("../Dockerfile", filepath.Join(contextDir, "Dockerfile"))) |
There was a problem hiding this comment.
Might want to add a second case where the target is an absolute path (dockerfile instead of "../Dockerfile") to be sure.
| return "", nil, fmt.Errorf("resolving containerfile %q: %w", dfile, err) | ||
| } | ||
| resolvedRel, err := filepath.Rel(contextAbs, resolved) | ||
| if err != nil || resolvedRel == ".." || strings.HasPrefix(resolvedRel, ".."+string(filepath.Separator)) { |
There was a problem hiding this comment.
Can we split this block into two blocks, so that for cases where err != nil the error we return can wrap the error that filepath.Rel() returned?
There was a problem hiding this comment.
Thanks for the review! I split the filepath.Rel() error handling to preserve the wrapped error and added an absolute-path symlink case to the test.
| return "", nil, fmt.Errorf("resolving containerfile %q: %w", dfile, err) | ||
| } | ||
| if rel, err := filepath.Rel(contextAbs, dfileAbs); err == nil && rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) { | ||
| resolved, err := filepath.EvalSymlinks(dfileAbs) |
There was a problem hiding this comment.
Could this dereference symlinks outside of the context directory?
(EvalSymlinks() on line 158)
There was a problem hiding this comment.
Would rel not be "..", and also not have "../" as a prefix if dfileAbs was outside of contextAbs?
There was a problem hiding this comment.
Symlinks might first take us out of the context directory and then back in again.
It could also be single symlink that contains something like ../context/Dockerfile as in the
the example #6861 (comment) where docker fails but podman succeeds.
There was a problem hiding this comment.
Thanks for raising this. The current check only verifies dfileAbs is under the context, so a symlink could still resolve outside (or escape then re-enter). I’ll add a second guard to ensure the resolved path is also within contextAbs (via filepath.Rel on the resolved path) and push an update.
There was a problem hiding this comment.
I added another docker behavior example #6861 (comment)
There was a problem hiding this comment.
Thanks for adding the additional Docker behavior example! I've tested both scenarios and the fix now correctly handles:
context/Dockerfile -> ../context/file- Now correctly fails (matches Docker)context/Dockerfile -> ../file- Now correctly succeeds (matches Docker)
The implementation has been updated and tested to ensure both the security fix and Docker behavior alignment are working correctly. The changes are pushed to the fix/dockerfile-symlink-context branch and ready for review.
|
LGTM |
|
Some unhappy tests which I restarted. |
|
You're going to want to rebase to pick up missing packages that are causing those "pivot_root: command not found" test failures, added to the VM images that we updated to in #6911. |
Signed-off-by: Mahendra Singh Rathore <49229348+mahendrarathore1742@users.noreply.github.com>
Signed-off-by: Mahendra Singh Rathore <49229348+mahendrarathore1742@users.noreply.github.com>
- Prevent Dockerfile symlinks from escaping build context (security fix) - Match Docker behavior for symlink resolution: * context/Dockerfile -> ../context/file: fail (both Docker and Buildah) * context/Dockerfile -> ../file: succeed (Docker behavior, now Buildah matches) - Add comprehensive tests for main issue and Docker behavior examples - Resolve symlink validation to check both original and resolved paths Closes podman-container-tools#6861 Signed-off-by: Mahendra Singh Rathore <49229348+mahendrarathore1742@users.noreply.github.com>
ed06de3 to
09e6b2c
Compare
Summary
This change aligns Buildah with Docker/BuildKit behavior by rejecting Dockerfile paths
inside the build context that resolve (via symlink) to a target outside the context.
What Changed
imagebuildah.BuildDockerfilesto ensure any symlinkresolves within
ContextDirectory.Why
Builds should not be able to read files outside the declared build context.
Docker/BuildKit fails in this case; Buildah previously allowed it. This fixes the
inconsistency and closes the security gap.
Testing
Closes
Closes #6861