Reject symlinked Containerfile and ignore files escaping build context by Honny1 · Pull Request #6886 · podman-container-tools/buildah

Honny1 · 2026-06-02T14:19:48Z

Fixes: #6861
Fixes: podman-container-tools/podman#28749

What type of PR is this?

/kind api-change

/kind bug

/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake
/kind other

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Buildah no longer follows symlinked Containerfile/Dockerfile or ignore files (e.g. Dockerfile.dockerignore) that point outside the build context directory, matching docker build behavior.

Honny1 · 2026-06-05T13:03:21Z

PTAL @podman-container-tools/buildah-maintainers @podman-container-tools/buildah-reviewers

mtrmac · 2026-06-05T15:17:41Z

-	return foundCtrFile, nil
+// isRegularFileInContext returns true if path is a regular file (or a symlink
+// to one) whose real target is inside contextDir.
+func isRegularFileInContext(contextDir, path string) bool {


(A drive-by, I have really only reviewed this part:) I think this is fine — works on Unix systems, and on Windows filepath.Rel rejects inputs which differ in the volume letter.

In principle, I think building an explicit os.Root and working within that would be structurally much safer, but looking at just this diff, that would probably be an invasive change and affect public API. (I didn’t investigate how difficult that would be.)

nalind

A subtle bug in how the symlinks get resolved, and I don't think I understand why some of these tests are using a subshell to run ln -s.

nalind · 2026-06-05T15:11:08Z

+	}
+	if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
+		return false
+	}


This appears to run into the same issue @eriksjolund pointed out in #6885 (comment).

If you mean "../../correctly-guessed/directory/name/Containerfile" pointing us back at "./Containerfile": I think to reject that we would truly need os.Root, or an equivalent manual use of openat2 RESOLVE_BENEATH (+ some implementation on non-Linux?) — or an entirely new manual symlink resolving code.

(filepath.SecureJoin is an ~equivalent of RESOLVE_IN_ROOT, it would not reject such links but interpret them differently.)

nalind · 2026-06-05T15:27:37Z

+RUN echo traversal-back-inside-works
+_EOF
+  # Symlink path goes ../context/file -- traverses outside but real target resolves inside.
+  (cd ${contextdir} && ln -s ../context/file Dockerfile)


Suggested change

(cd ${contextdir} && ln -s ../context/file Dockerfile)

ln -s ../context/file ${contextdir}/Dockerfile

nalind · 2026-06-05T15:28:05Z

+  (cd ${contextdir} && ln -s ../context/file Dockerfile)
+
+  run_buildah build $WITH_POLICY_JSON ${contextdir}
+  assert "$output" =~ "traversal-back-inside-works"


I think this is expected to fail while attempting to read ${contextdir}/context/file.

nalind · 2026-06-05T15:30:30Z

+RUN test -f /dir/file
+_EOF
+  touch ${contextdir}/file
+  (cd ${contextdir} && ln -s ../ign Dockerfile.dockerignore)


Suggested change

(cd ${contextdir} && ln -s ../ign Dockerfile.dockerignore)

ln -s ../ign ${contextdir}/Dockerfile.dockerignore

mtrmac

Just a quick look: This is getting increasingly convoluted.

If the goal is to solve https://github.com/podman-container-tools/buildah/pull/6886/changes#r3363568131, I think refactoring to build around os.Root would ultimately be safer and much easier to prove correct. But it might be very invasive, I didn’t check – I’ll let Buildah experts make the decision on whether this should happen.

mtrmac · 2026-06-08T16:11:45Z

+			return false
+		}
+		clean := filepath.Clean(target)
+		if !filepath.IsAbs(target) && (clean == ".." || strings.HasPrefix(clean, ".."+string(filepath.Separator))) {


Shouldn’t absolute symlinks be treated as a clear attempt to break out?

mtrmac · 2026-06-08T16:13:24Z

+	if err != nil {
+		return false
+	}
+	info, err := os.Lstat(path)


Couldn’t path have symlinks pointing outside of the target directory in the middle? ./symlink-to-etc/password. where symlink-to-etc is ../../../../../…/etc?

mtrmac · 2026-06-08T16:14:40Z

+			return false
+		}
+		clean := filepath.Clean(target)
+		if !filepath.IsAbs(target) && (clean == ".." || strings.HasPrefix(clean, ".."+string(filepath.Separator))) {


path being dir1/dir2/symlink with symlink pointing to ../Containerfile does not actually escape.

Honny1 · 2026-06-09T15:36:20Z

I used os.Root in the parts that I touched. I think that refactoring makes sense to me, but not in this PR. Maybe we should create an issue for this.

Fixes: podman-container-tools#6861 Fixes: podman-container-tools/podman#28749 Signed-off-by: Jan Rodák <hony.com@seznam.cz>

Honny1 · 2026-06-22T14:51:09Z

PTAL @nalind @mtrmac

nalind

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.

nalind · 2026-06-23T21:09:19Z

+
+  run_buildah 125 build $WITH_POLICY_JSON ${contextdir}
+  expect_output --substring "cannot find Containerfile or Dockerfile"
+  assert "$output" !~ "SHOULD-NOT-RUN"


If we're treating ${contextdir} like the root of a container rootfs (which I think approximates the desired behavior for resolving symlinks found inside of a context directory), I would have expected the target of the symlink to be readable. I believe this is an example from #6861 (comment).

nalind · 2026-06-23T21:11:59Z

+  mkdir -p ${tarsrc}
+  ln -s ${targetfile} ${tarsrc}/Dockerfile
+  local context_tar=${TEST_SCRATCH_DIR}/context.tar
+  tar -cf ${context_tar} -C ${tarsrc} Dockerfile


Might want a --owner=0:0 --numeric-owner here.

nalind · 2026-06-23T21:17:06Z

+  ln -s ${targetfile} ${tarsrc}/Dockerfile
+  local contentdir=${TEST_SCRATCH_DIR}/content
+  mkdir -p ${contentdir}
+  tar -cf ${contentdir}/context.tar -C ${tarsrc} Dockerfile


Might want a --owner=0:0 --numeric-owner here.

Honny1 force-pushed the fix-symlinks branch from 2392879 to fbce272 Compare June 2, 2026 14:45

nalind reviewed Jun 2, 2026

View reviewed changes

Comment thread pkg/parse/parse.go Outdated

nalind reviewed Jun 2, 2026

View reviewed changes

Comment thread tests/bud.bats Outdated

Honny1 force-pushed the fix-symlinks branch 3 times, most recently from b845b72 to e462928 Compare June 5, 2026 12:46

Honny1 marked this pull request as ready for review June 5, 2026 13:02

Honny1 requested a review from nalind June 5, 2026 13:03

mtrmac reviewed Jun 5, 2026

View reviewed changes

nalind requested changes Jun 5, 2026

View reviewed changes

Honny1 force-pushed the fix-symlinks branch from e462928 to a83f676 Compare June 8, 2026 14:57

Honny1 requested review from mtrmac and nalind June 8, 2026 15:29

mtrmac reviewed Jun 8, 2026

View reviewed changes

Honny1 force-pushed the fix-symlinks branch from a83f676 to 6ae98d7 Compare June 9, 2026 15:21

Honny1 requested a review from mtrmac June 9, 2026 15:36

Reject symlinked Containerfile and ignore files escaping build context

0e5a0f2

Fixes: podman-container-tools#6861 Fixes: podman-container-tools/podman#28749 Signed-off-by: Jan Rodák <hony.com@seznam.cz>

Honny1 force-pushed the fix-symlinks branch from 6ae98d7 to 0e5a0f2 Compare June 10, 2026 08:15

nalind reviewed Jun 23, 2026

View reviewed changes

	(cd ${contextdir} && ln -s ../context/file Dockerfile)
	ln -s ../context/file ${contextdir}/Dockerfile

	(cd ${contextdir} && ln -s ../ign Dockerfile.dockerignore)
	ln -s ../ign ${contextdir}/Dockerfile.dockerignore

Conversation

Honny1 commented Jun 2, 2026

What type of PR is this?

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Uh oh!

Uh oh!

Uh oh!

Honny1 commented Jun 5, 2026

Uh oh!

mtrmac Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nalind left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Honny1 commented Jun 9, 2026

Uh oh!

Honny1 commented Jun 22, 2026

Uh oh!

nalind left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mtrmac Jun 5, 2026 •

edited

Loading