diff --git a/imagebuildah/executor.go b/imagebuildah/executor.go index 5b234d263c..7bcaedfa4d 100644 --- a/imagebuildah/executor.go +++ b/imagebuildah/executor.go @@ -168,6 +168,8 @@ type executor struct { imageInfoLock sync.Mutex imageInfoCache map[string]imageTypeAndHistoryAndDiffIDs fromOverride string + originalFromOverride string + fromOverrideStageIndex int // stage that received --from, or -1 additionalBuildContexts map[string]*additionalBuildContext manifest string secrets map[string]define.Secret @@ -370,6 +372,8 @@ func newExecutor(logger *logrus.Logger, logPrefix string, store storage.Store, o rusageLogFile: rusageLogFile, imageInfoCache: make(map[string]imageTypeAndHistoryAndDiffIDs), fromOverride: options.From, + originalFromOverride: options.From, + fromOverrideStageIndex: -1, additionalBuildContexts: wrappedAdditionalBuildContexts, manifest: options.Manifest, secrets: secrets, @@ -944,6 +948,7 @@ func (b *executor) Build(ctx context.Context, stages imagebuilder.Stages) (image if b.fromOverride != "" { child.Next.Value = b.fromOverride b.fromOverride = "" + b.fromOverrideStageIndex = stageIndex } base := child.Next.Value if base != "" && base != buildah.BaseImageFakeName { diff --git a/imagebuildah/stage_executor.go b/imagebuildah/stage_executor.go index 04597088eb..004333ef64 100644 --- a/imagebuildah/stage_executor.go +++ b/imagebuildah/stage_executor.go @@ -982,8 +982,9 @@ func (s *stageExecutor) UnrecognizedInstruction(step *imagebuilder.Step) error { // relative path names are evaluated relative to "contextDir", it will create a // copy of the original image, under "tmpdir", which contains no symbolic // links, and return either the original image reference or a reference to a -// sanitized copy which should be used instead. -func (s *stageExecutor) sanitizeFrom(from, tmpdir string) (newFrom string, err error) { +// sanitized copy which should be used instead. allowAbsolutePaths should be +// true only when the reference originated from the --from CLI flag. +func (s *stageExecutor) sanitizeFrom(from, tmpdir string, allowAbsolutePaths bool) (newFrom string, err error) { transportName, restOfImageName, maybeHasTransportName := strings.Cut(from, ":") if !maybeHasTransportName || transports.Get(transportName) == nil { if _, err = reference.ParseNormalizedNamed(from); err == nil { @@ -997,7 +998,7 @@ func (s *stageExecutor) sanitizeFrom(from, tmpdir string) (newFrom string, err e return "", fmt.Errorf("parsing image name %q: %w", from, err) } // TODO: drop this part and just return an error... someday - return sanitize.ImageName(s.executor.store, transportName, restOfImageName, s.executor.contextDir, tmpdir) + return sanitize.ImageName(s.executor.store, transportName, restOfImageName, s.executor.contextDir, tmpdir, allowAbsolutePaths) } // prepare creates a working container based on the specified image, or if one @@ -1017,6 +1018,13 @@ func (s *stageExecutor) prepare(ctx context.Context, from string, initializeIBCo from = base } + // Decide before source policy may rewrite from: absolute paths are only + // allowed for the stage whose first FROM was replaced by --from, not for + // later stages that happen to use the same image reference string. + allowAbsolutePaths := s.executor.originalFromOverride != "" && + s.index == s.executor.fromOverrideStageIndex && + from == s.executor.originalFromOverride + // Apply source policy if one is configured and this is not "scratch" or a stage reference. // Stage references are handled separately and don't need policy evaluation since they // refer to images built within this same build. @@ -1062,7 +1070,7 @@ func (s *stageExecutor) prepare(ctx context.Context, from string, initializeIBCo } } - sanitizedFrom, err := s.sanitizeFrom(from, tmpdir.GetTempDir()) + sanitizedFrom, err := s.sanitizeFrom(from, tmpdir.GetTempDir(), allowAbsolutePaths) if err != nil { return nil, fmt.Errorf("invalid base image specification %q: %w", from, err) } diff --git a/internal/sanitize/sanitize.go b/internal/sanitize/sanitize.go index c0d189b3ce..bf1641c306 100644 --- a/internal/sanitize/sanitize.go +++ b/internal/sanitize/sanitize.go @@ -43,18 +43,15 @@ func newDirectoryDestination(tmpdir string) (string, error) { } // create an archive containing a single item from the build context +// contextRelativePath strips leading "/" and "./" so that a path is evaluated +// relative to the build context. Containerfile references like +// oci-archive:/subdir/file are context-absolute (like COPY), not host paths. +func contextRelativePath(archiveSource string) string { + return path.Clean(strings.TrimLeft(archiveSource, "/")) +} + func newSingleItemArchive(contextDir, archiveSource string) (io.ReadCloser, error) { - for { - // try to make sure the archiver doesn't get thrown by relative prefixes - if strings.HasPrefix(archiveSource, "/") && archiveSource != "/" { - archiveSource = strings.TrimPrefix(archiveSource, "/") - continue - } else if strings.HasPrefix(archiveSource, "./") && archiveSource != "./" { - archiveSource = strings.TrimPrefix(archiveSource, "./") - continue - } - break - } + archiveSource = contextRelativePath(archiveSource) // grab only that one file, ignore anything and everything else tarOptions := &archive.TarOptions{ IncludeFiles: []string{path.Clean(archiveSource)}, @@ -184,8 +181,13 @@ func writeToDirectory(root string, hdr *tar.Header, content io.Reader) error { // which refer to filesystem objects, where relative path names are evaluated // relative to "contextDir", it will create a copy of the original image, under // "tmpdir", which contains no symbolic links. It it returns a parseable -// reference to the image which should be used. -func ImageName(store storage.Store, transportName, restOfImageName, contextDir, tmpdir string) (newFrom string, err error) { +// reference to the image which should be used. If allowAbsolutePaths is true, +// absolute paths are resolved against the real filesystem root rather than +// being treated as relative to contextDir; this should only be set when the +// path was provided directly by the user (e.g. via --from on the command line) +// rather than read from a Containerfile. Paths with a leading "/" from a +// Containerfile are context-absolute (like COPY), not host absolute paths. +func ImageName(store storage.Store, transportName, restOfImageName, contextDir, tmpdir string, allowAbsolutePaths bool) (newFrom string, err error) { seenEntries := make(map[string]struct{}) // we're going to try to create a temporary directory or file, but if // we fail, make sure that they get removed immediately @@ -242,8 +244,13 @@ func ImageName(store storage.Store, transportName, restOfImageName, contextDir, } }() // archive only the archive file for copying to the new archive file - imageArchive, err = newSingleItemArchive(contextDir, archiveSource) - isEmbeddedArchive = true + if allowAbsolutePaths && filepath.IsAbs(archiveSource) { + imageArchive, err = os.Open(archiveSource) + isEmbeddedArchive = false + } else { + imageArchive, err = newSingleItemArchive(contextDir, archiveSource) + isEmbeddedArchive = true + } // generate the new reference using the temporary file's name newFrom = transportName + ":" + newImageDestination + refLeftover case ociLayoutTransport.Transport.Name(): // this is a directory tree @@ -260,27 +267,42 @@ func ImageName(store storage.Store, transportName, restOfImageName, contextDir, } // archive the entire layout directory for copying to the new layout directory tarOptions := &archive.TarOptions{} - imageArchive, err = chrootarchive.Tar(filepath.Join(contextDir, archiveSource), tarOptions, contextDir) + if allowAbsolutePaths && filepath.IsAbs(archiveSource) { + // Sanitize a host path (--from): copy the tree and reject symlinks + // that point outside of it. + imageArchive, err = chrootarchive.Tar(archiveSource, tarOptions, filepath.Dir(archiveSource)) + } else { + // Leading "/" is context-absolute (like COPY), not a host path. + // filepath.Join(contextDir, "/foo") discards contextDir on Unix. + imageArchive, err = chrootarchive.Tar(filepath.Join(contextDir, contextRelativePath(archiveSource)), tarOptions, contextDir) + } // generate the new reference using the directory newFrom = transportName + ":" + newImageDestination + refLeftover case directoryTransport.Transport.Name(): // this is also a directory tree // this takes the form of just a path - transportRef := restOfImageName + archiveSource = restOfImageName // create a new directory to use as our new image directory if newImageDestination, err = newDirectoryDestination(tmpdir); err != nil { return "", fmt.Errorf("creating temporary copy of base image: %w", err) } // archive the entire directory for copying to the new directory - archiveSource = transportRef tarOptions := &archive.TarOptions{} - imageArchive, err = chrootarchive.Tar(filepath.Join(contextDir, archiveSource), tarOptions, contextDir) + if allowAbsolutePaths && filepath.IsAbs(archiveSource) { + imageArchive, err = chrootarchive.Tar(archiveSource, tarOptions, filepath.Dir(archiveSource)) + } else { + imageArchive, err = chrootarchive.Tar(filepath.Join(contextDir, contextRelativePath(archiveSource)), tarOptions, contextDir) + } // generate the new reference using the directory newFrom = transportName + ":" + newImageDestination default: return "", fmt.Errorf("unexpected container image transport %q", transportName) } if err != nil { - return "", fmt.Errorf("error archiving source at %q under %q", archiveSource, contextDir) + archiveRoot := contextDir + if allowAbsolutePaths && filepath.IsAbs(archiveSource) { + archiveRoot = filepath.Dir(archiveSource) + } + return "", fmt.Errorf("error archiving source at %q under %q: %w", archiveSource, archiveRoot, err) } // start reading the archived content diff --git a/internal/sanitize/sanitize_test.go b/internal/sanitize/sanitize_test.go index 8a854f1b0a..5cc613fbdd 100644 --- a/internal/sanitize/sanitize_test.go +++ b/internal/sanitize/sanitize_test.go @@ -374,21 +374,35 @@ func TestSanitizeImageName(t *testing.T) { // sanitize them all goodLayoutRel, err := filepath.Rel(contextDir, goodLayout) require.NoErrorf(t, err, "converting absolute path %q to a relative one", goodLayout) - newGoodLayout, err := ImageName(store, "oci", goodLayoutRel, contextDir, t.TempDir()) + newGoodLayout, err := ImageName(store, "oci", goodLayoutRel, contextDir, t.TempDir(), false) require.NoError(t, err, "sanitizing good OCI layout") goodDirRel, err := filepath.Rel(contextDir, goodDir) require.NoErrorf(t, err, "converting absolute path %q to a relative one", goodDir) - newGoodDir, err := ImageName(store, "dir", goodDirRel, contextDir, t.TempDir()) + newGoodDir, err := ImageName(store, "dir", goodDirRel, contextDir, t.TempDir(), false) require.NoError(t, err, "sanitizing good directory") goodOCIArchiveRel, err := filepath.Rel(contextDir, goodOCIArchive) require.NoErrorf(t, err, "converting absolute path %q to a relative one", goodOCIArchive) - newGoodOCIArchive, err := ImageName(store, "oci-archive", goodOCIArchiveRel, contextDir, t.TempDir()) + newGoodOCIArchive, err := ImageName(store, "oci-archive", goodOCIArchiveRel, contextDir, t.TempDir(), false) require.NoError(t, err, "sanitizing good OCI archive") goodDockerArchiveRel, err := filepath.Rel(contextDir, goodDockerArchive) require.NoErrorf(t, err, "converting absolute path %q to a relative one", goodDockerArchive) - newGoodDockerArchive, err := ImageName(store, "docker-archive", goodDockerArchiveRel, contextDir, t.TempDir()) + newGoodDockerArchive, err := ImageName(store, "docker-archive", goodDockerArchiveRel, contextDir, t.TempDir(), false) require.NoError(t, err, "sanitizing good docker archive") + // Context-absolute paths (leading "/") must resolve under contextDir, not the host. + newGoodLayoutCtxAbs, err := ImageName(store, "oci", "/"+goodLayoutRel, contextDir, t.TempDir(), false) + require.NoError(t, err, "sanitizing context-absolute OCI layout") + requireImageReadable(t, newGoodLayoutCtxAbs) + newGoodDirCtxAbs, err := ImageName(store, "dir", "/"+goodDirRel, contextDir, t.TempDir(), false) + require.NoError(t, err, "sanitizing context-absolute directory") + requireImageReadable(t, newGoodDirCtxAbs) + newGoodOCIArchiveCtxAbs, err := ImageName(store, "oci-archive", "/"+goodOCIArchiveRel, contextDir, t.TempDir(), false) + require.NoError(t, err, "sanitizing context-absolute OCI archive") + requireImageReadable(t, newGoodOCIArchiveCtxAbs) + newGoodDockerArchiveCtxAbs, err := ImageName(store, "docker-archive", "/"+goodDockerArchiveRel, contextDir, t.TempDir(), false) + require.NoError(t, err, "sanitizing context-absolute docker archive") + requireImageReadable(t, newGoodDockerArchiveCtxAbs) + // make sure the sanitized versions can all be read without error requireImageReadable(t, newGoodLayout) requireImageReadable(t, newGoodDir) @@ -399,24 +413,97 @@ func TestSanitizeImageName(t *testing.T) { badLayout := mutateDirectory(t, contextDir, goodLayout, filepath.Join(v1.ImageBlobsDir, blobDigest.Algorithm().String(), blobDigest.Encoded())) badLayoutRel, err := filepath.Rel(contextDir, badLayout) require.NoErrorf(t, err, "converting absolute path %q to a relative one", badLayout) - _, err = ImageName(store, "oci", badLayoutRel, contextDir, t.TempDir()) + _, err = ImageName(store, "oci", badLayoutRel, contextDir, t.TempDir(), false) require.ErrorIs(t, err, os.ErrNotExist, "sanitizing bad OCI layout") badDir := mutateDirectory(t, contextDir, goodDir, blobDigest.Encoded()) badDirRel, err := filepath.Rel(contextDir, badDir) require.NoErrorf(t, err, "converting absolute path %q to a relative one", badDir) - _, err = ImageName(store, "dir", badDirRel, contextDir, t.TempDir()) + _, err = ImageName(store, "dir", badDirRel, contextDir, t.TempDir(), false) require.ErrorIs(t, err, os.ErrNotExist, "sanitizing bad directory") badOCIArchive := mutateArchive(t, contextDir, goodOCIArchive, filepath.Join(v1.ImageBlobsDir, blobDigest.Algorithm().String(), blobDigest.Encoded())) badOCIArchiveRel, err := filepath.Rel(contextDir, badOCIArchive) require.NoErrorf(t, err, "converting absolute path %q to a relative one", badOCIArchive) - _, err = ImageName(store, "oci-archive", badOCIArchiveRel, contextDir, t.TempDir()) + _, err = ImageName(store, "oci-archive", badOCIArchiveRel, contextDir, t.TempDir(), false) require.ErrorContains(t, err, "invalid symbolic link", "sanitizing bad oci archive") badDockerArchive := mutateArchive(t, contextDir, goodDockerArchive, diffDigest.Encoded()+".tar") badDockerArchiveRel, err := filepath.Rel(contextDir, badDockerArchive) require.NoErrorf(t, err, "converting absolute path %q to a relative one", badDockerArchive) - _, err = ImageName(store, "docker-archive", badDockerArchiveRel, contextDir, t.TempDir()) + _, err = ImageName(store, "docker-archive", badDockerArchiveRel, contextDir, t.TempDir(), false) require.ErrorContains(t, err, "invalid symbolic link", "sanitizing bad docker archive") + + // sanitize with absolute paths and allowAbsolutePaths=true (simulates --from CLI flag) + absDir := t.TempDir() + absLayout := filepath.Join(absDir, "abslayout") + require.NoError(t, os.Mkdir(absLayout, 0o700), "creating absolute-path OCI layout") + generateLayout(t, absLayout) + absLayoutRef, err := alltransports.ParseImageName("oci:" + absLayout) + require.NoError(t, err, "parsing absolute-path OCI layout reference") + + absDirDest := filepath.Join(absDir, "absdir") + require.NoError(t, os.Mkdir(absDirDest, 0o700), "creating absolute-path directory image") + absDirRef, err := alltransports.ParseImageName("dir:" + absDirDest) + require.NoError(t, err, "parsing absolute-path directory reference") + _, err = imageCopy.Image(ctx, policyContext, absDirRef, absLayoutRef, nil) + require.NoError(t, err, "copying layout to absolute-path directory") + + absOCIArchiveFile, err := os.CreateTemp(absDir, "absociarchive") + require.NoError(t, err, "creating absolute-path OCI archive file") + absOCIArchive := absOCIArchiveFile.Name() + absOCIArchiveRef, err := alltransports.ParseImageName("oci-archive:" + absOCIArchive) + require.NoError(t, err, "parsing absolute-path OCI archive reference") + require.NoError(t, absOCIArchiveFile.Close()) + require.NoError(t, os.Remove(absOCIArchive)) + _, err = imageCopy.Image(ctx, policyContext, absOCIArchiveRef, absLayoutRef, nil) + require.NoError(t, err, "copying layout to absolute-path OCI archive") + + absDockerArchiveFile, err := os.CreateTemp(absDir, "absdockerarchive") + require.NoError(t, err, "creating absolute-path docker archive file") + absDockerArchive := absDockerArchiveFile.Name() + absDockerArchiveRef, err := alltransports.ParseImageName("docker-archive:" + absDockerArchive) + require.NoError(t, err, "parsing absolute-path docker archive reference") + require.NoError(t, absDockerArchiveFile.Close()) + require.NoError(t, os.Remove(absDockerArchive)) + _, err = imageCopy.Image(ctx, policyContext, absDockerArchiveRef, absLayoutRef, nil) + require.NoError(t, err, "copying layout to absolute-path docker archive") + + newAbsLayout, err := ImageName(store, "oci", absLayout, contextDir, t.TempDir(), true) + require.NoError(t, err, "sanitizing absolute-path OCI layout with allowAbsolutePaths") + requireImageReadable(t, newAbsLayout) + + newAbsDir, err := ImageName(store, "dir", absDirDest, contextDir, t.TempDir(), true) + require.NoError(t, err, "sanitizing absolute-path directory with allowAbsolutePaths") + requireImageReadable(t, newAbsDir) + + newAbsOCIArchive, err := ImageName(store, "oci-archive", absOCIArchive, contextDir, t.TempDir(), true) + require.NoError(t, err, "sanitizing absolute-path OCI archive with allowAbsolutePaths") + requireImageReadable(t, newAbsOCIArchive) + + newAbsDockerArchive, err := ImageName(store, "docker-archive", absDockerArchive, contextDir, t.TempDir(), true) + require.NoError(t, err, "sanitizing absolute-path docker archive with allowAbsolutePaths") + requireImageReadable(t, newAbsDockerArchive) + + // Host absolute paths without allowAbsolutePaths are treated as context-relative. + // Sanitization may succeed with an empty tree; the result must not be a usable + // copy of the host image. + requireNotReadableHostCopy := func(t *testing.T, transport, absPath string) { + t.Helper() + newName, err := ImageName(store, transport, absPath, contextDir, t.TempDir(), false) + if err != nil { + return + } + ref, err := alltransports.ParseImageName(newName) + require.NoError(t, err, "parsing sanitized reference %q", newName) + destDir := t.TempDir() + dest, err := alltransports.ParseImageName("dir:" + destDir) + require.NoError(t, err) + _, err = imageCopy.Image(ctx, policyContext, dest, ref, nil) + require.Error(t, err, "%s sanitized from host path %q must not be readable", transport, absPath) + } + requireNotReadableHostCopy(t, "oci", absLayout) + requireNotReadableHostCopy(t, "dir", absDirDest) + requireNotReadableHostCopy(t, "oci-archive", absOCIArchive) + requireNotReadableHostCopy(t, "docker-archive", absDockerArchive) } diff --git a/tests/bud.bats b/tests/bud.bats index ee022be6d8..ef896263e8 100644 --- a/tests/bud.bats +++ b/tests/bud.bats @@ -10748,3 +10748,102 @@ _EOF done done } + +@test "bud --from with absolute path to local transport" { + skip_if_no_runtime + + _prefetch busybox + # Archives must live outside the build context to test absolute path handling + local archive_dir=${TEST_SCRATCH_DIR}/from_abs + mkdir -p "${archive_dir}" + + copy containers-storage:busybox oci-archive:"${archive_dir}"/busybox.ociarchive + copy containers-storage:busybox docker-archive:"${archive_dir}"/busybox.dockerarchive + copy containers-storage:busybox oci:"${archive_dir}"/busybox-oci + copy containers-storage:busybox dir:"${archive_dir}"/busybox-dir + + local contextdir=${TEST_SCRATCH_DIR}/context + mkdir -p "${contextdir}" + cat > "${contextdir}"/Containerfile << 'EOF' +FROM overridden +RUN touch /absolute-path-test +EOF + + local i=0 + for from in \ + oci-archive:"${archive_dir}"/busybox.ociarchive \ + docker-archive:"${archive_dir}"/busybox.dockerarchive \ + oci:"${archive_dir}"/busybox-oci \ + dir:"${archive_dir}"/busybox-dir \ + ; do + local target=from-abs-${i} + run_buildah build $WITH_POLICY_JSON \ + --from "$from" \ + -t "${target}" \ + -f "${contextdir}"/Containerfile \ + "${contextdir}" + expect_output --substring "STEP 1/2: FROM $from" + run_buildah from --quiet $WITH_POLICY_JSON "${target}" + cid="$output" + run_buildah run "$cid" test -f /absolute-path-test + run_buildah rm "$cid" + i=$((i + 1)) + done +} + +@test "bud FROM absolute path to local transport without --from fails" { + skip_if_no_runtime + + _prefetch busybox + # Archive lives outside the build context; Containerfile must not read it + local archive_dir=${TEST_SCRATCH_DIR}/from_abs_denied + mkdir -p "${archive_dir}" + copy containers-storage:busybox oci-archive:"${archive_dir}"/busybox.ociarchive + + local contextdir=${TEST_SCRATCH_DIR}/context_denied + mkdir -p "${contextdir}" + cat > "${contextdir}"/Containerfile << EOF +FROM oci-archive:${archive_dir}/busybox.ociarchive +RUN touch /should-not-run +EOF + + run_buildah 125 build $WITH_POLICY_JSON \ + -f "${contextdir}"/Containerfile \ + "${contextdir}" + # Host absolute paths in FROM are resolved under the build context, so the + # outside-context archive is not used and the build fails. +} + +@test "bud --from only overrides first stage FROM with absolute path" { + skip_if_no_runtime + + _prefetch busybox alpine + local archive_dir=${TEST_SCRATCH_DIR}/from_abs_multistage + mkdir -p "${archive_dir}" + copy containers-storage:busybox oci-archive:"${archive_dir}"/busybox.ociarchive + + local contextdir=${TEST_SCRATCH_DIR}/context_multistage + mkdir -p "${contextdir}" + cat > "${contextdir}"/Containerfile << 'EOF' +FROM overridden AS s1 +RUN touch /stage1-marker +FROM alpine AS s2 +COPY --from=s1 /stage1-marker / +RUN touch /stage2-marker +EOF + + local from="oci-archive:${archive_dir}/busybox.ociarchive" + run_buildah build $WITH_POLICY_JSON \ + --from "$from" \ + -t from-abs-multistage \ + -f "${contextdir}"/Containerfile \ + "${contextdir}" + expect_output --substring "\[1/2] STEP 1/2: FROM $from AS s1" + expect_output --substring "\[2/2] STEP 1/3: FROM alpine AS s2" + + run_buildah from --quiet $WITH_POLICY_JSON from-abs-multistage + cid="$output" + run_buildah run "$cid" test -f /stage1-marker + run_buildah run "$cid" test -f /stage2-marker + run_buildah rm "$cid" +}