Skip to content

Fix --from with absolute paths to local image transports #6975

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Honny1 wants to merge 1 commit into
base: main
Choose a base branch
from
+253 −32
Open

Fix --from with absolute paths to local image transports #6975

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions imagebuildah/executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
Expand Down
16 changes: 12 additions & 4 deletions imagebuildah/stage_executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
Expand All @@ -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.
Expand Down Expand Up @@ -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)
}
Expand Down
62 changes: 42 additions & 20 deletions internal/sanitize/sanitize.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)},
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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))

@nalind nalind Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the intent here to disrupt links to items outside of the context directory when absolute paths are being allowed? If it is not, then we don't need to be calling this function.

} 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
Expand Down
103 changes: 95 additions & 8 deletions internal/sanitize/sanitize_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,21 +374,35 @@
// 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)
Expand All @@ -399,24 +413,97 @@
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, err := os.MkdirTemp(absDir, "abslayout")

Check failure on line 439 in internal/sanitize/sanitize_test.go

View workflow job for this annotation

GitHub Actions / validate 

os.MkdirTemp() could be replaced by t.TempDir() in TestSanitizeImageName (usetesting)
require.NoError(t, err, "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, err := os.MkdirTemp(absDir, "absdir")

Check failure on line 445 in internal/sanitize/sanitize_test.go

View workflow job for this annotation

GitHub Actions / validate 

os.MkdirTemp() could be replaced by t.TempDir() in TestSanitizeImageName (usetesting)
require.NoError(t, err, "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)
}
Loading
Loading