diff --git a/integration/config.go b/integration/config.go index ee15b8bb3..849499ea8 100644 --- a/integration/config.go +++ b/integration/config.go @@ -28,6 +28,7 @@ type integrationTestConfig struct { onbuildBaseImage string onbuildCopyImage string hardlinkBaseImage string + hijackBaseImage string serviceAccount string dockerMajorVersion int gcsClient *storage.Client diff --git a/integration/dockerfiles/Dockerfile_test_ignore_path_subtree b/integration/dockerfiles/Dockerfile_test_ignore_path_subtree new file mode 100644 index 000000000..9315a099d --- /dev/null +++ b/integration/dockerfiles/Dockerfile_test_ignore_path_subtree @@ -0,0 +1,4 @@ +FROM scratch +# mz560: in kaniko v1.27.0, when built with --ignore-path=/dest, +# the file copied into /dest/subdir/ would slip through and end up in the layer. +COPY context/foo /dest/subdir/file.txt diff --git a/integration/dockerfiles/Dockerfile_test_issue_mz560 b/integration/dockerfiles/Dockerfile_test_issue_mz560 new file mode 100644 index 000000000..267634085 --- /dev/null +++ b/integration/dockerfiles/Dockerfile_test_issue_mz560 @@ -0,0 +1,46 @@ +# mz560: In kaniko v1.27.0 there is no consistency check for /kaniko binaries. +# A bug was discovered where tarballs can be constructed to unpack arbitrary files into +# /kaniko directory. As kaniko is no longer a single-binary application, +# every RUN runs via tini, this means that that arbitrary payload will be executed. +# But actually it is way simpler than that, as shown here the same can be achieved with ONBUILD. +# This sounds bad, but is actually by design. It is not less secure than having an `ONBUILD` per-se. +# Docker build is RCE as a service, that is the reason why kaniko does run without privileges. +ARG IMAGE_REPO +FROM busybox AS base +# We simulate command injection by hijacking the tini binary + +# First we hijack it by adding a hijacked binary to this base image. +# This base image gets built with docker, so it is part of the image. +# When kaniko unrolls the filesystem it might already override the binary with our hijacked version. +COPY --chmod=755 < /tmp/kaniko/tini \ + && echo "echo 'WARN HIJACKED'" >> /tmp/kaniko/tini \ + && chmod +x /tmp/kaniko/tini + +ONBUILD RUN --mount=type=cache,id=hijack,target=/kaniko \ + ls -la /kaniko && cat /kaniko/tini || true + +# Thirdly, we can use a `ONBUILD RUN` statement directly +# here kaniko has no chance to block us as RUN statements +# are per design an all powerful blackbox. +ONBUILD RUN mv /kaniko/tini /dev/null || true \ + && echo "#!/bin/sh" > /kaniko/tini \ + && echo "echo 'WARN HIJACKED'" >> /kaniko/tini \ + && chmod +x /kaniko/tini + +FROM ${IMAGE_REPO}hijack:latest +# /kaniko/tini gets implicitly executed here +RUN ls -la /kaniko && cat /kaniko/tini || true diff --git a/integration/dockerfiles/Dockerfile_test_prefix_match_only b/integration/dockerfiles/Dockerfile_test_prefix_match_only new file mode 100644 index 000000000..f24884696 --- /dev/null +++ b/integration/dockerfiles/Dockerfile_test_prefix_match_only @@ -0,0 +1,7 @@ +FROM alpine +# mz560: Simulates apt-key writing temporary key material under /tmp/apt-key-gpghome. +# The directory itself should appear in the image layer, but files inside it +# should be excluded because /tmp/apt-key-gpghome is on the ignore list with +# PrefixMatchOnly=true. +RUN mkdir /tmp/apt-key-gpghome && \ + echo "key material" > /tmp/apt-key-gpghome/pubring.gpg diff --git a/integration/images.go b/integration/images.go index 83345b679..f30280cde 100644 --- a/integration/images.go +++ b/integration/images.go @@ -19,6 +19,7 @@ package integration import ( "bytes" "context" + "errors" "fmt" "os" "os/exec" @@ -122,6 +123,7 @@ var additionalKanikoFlagsMap = map[string][]string{ "Dockerfile_test_maintainer": {"--single-snapshot"}, "Dockerfile_test_target": {"--target=second"}, "Dockerfile_test_snapshotter_ignorelist": {"--use-new-run=true", "-v=trace"}, + "Dockerfile_test_ignore_path_subtree": {"--ignore-path=/dest"}, "Dockerfile_test_cache": {"--cache-copy-layers=true"}, "Dockerfile_test_cache_oci": {"--cache-copy-layers=true"}, "Dockerfile_test_cache_install": {"--cache-copy-layers=true"}, @@ -139,6 +141,10 @@ var additionalKanikoFlagsMap = map[string][]string{ "Dockerfile_test_issue_mz529": {"--cleanup"}, } +var expectErr = map[string]int{ + "Dockerfile_test_issue_mz560": 1, +} + // Arguments to diffoci when comparing dockerfiles var diffArgsMap = map[string][]string{ // /root/.config 0x1c0 0x1ed @@ -444,11 +450,21 @@ func (d *DockerFileBuilder) BuildImageWithContext(t *testing.T, config *integrat kanikoImage := GetKanikoImage(imageRepo, dockerfile) timer = timing.Start(dockerfile + "_kaniko") - if _, err := buildKanikoImage(t.Logf, dockerfilesPath, dockerfile, buildArgs, additionalKanikoFlags, kanikoImage, - contextDir, gcsBucket, gcsClient, serviceAccount, true); err != nil { + defer timing.DefaultRun.Stop(timer) + _, err := buildKanikoImage(t.Logf, dockerfilesPath, dockerfile, buildArgs, additionalKanikoFlags, kanikoImage, + contextDir, gcsBucket, gcsClient, serviceAccount, true) + if expectErr, ok := expectErr[dockerfile]; ok { + var exitErr *exec.ExitError + if errors.As(err, &exitErr) && exitErr.ExitCode() == expectErr { + return nil + } + if err == nil { + return fmt.Errorf("expected exit code %d but command succeeded", expectErr) + } + } + if err != nil { return err } - timing.DefaultRun.Stop(timer) d.filesBuilt[dockerfile] = struct{}{} diff --git a/integration/integration_test.go b/integration/integration_test.go index ce021eb1c..3c7e36ce6 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -183,6 +183,12 @@ func buildRequiredImages() error { }, { name: "Building kaniko image with leftover stuff in the filesystem", command: []string{"docker", "build", "-t", ExecutorImageTainted, "-f", fmt.Sprintf("%s/Dockerfile_test_issue_mz455", dockerfilesPath), "--target", "kaniko", "."}, + }, { + name: "Building hijack base image", + command: []string{"docker", "build", "-t", config.hijackBaseImage, "-f", fmt.Sprintf("%s/Dockerfile_test_issue_mz560", dockerfilesPath), "--target", "base", "."}, + }, { + name: "Pushing hijack base image", + command: []string{"docker", "push", config.hijackBaseImage}, }} for _, setupCmd := range setupCommands { @@ -635,6 +641,67 @@ func TestLayers(t *testing.T) { } } +// mz560: TestPrefixMatchOnlyIgnoreList verifies that an ignore list entry with +// PrefixMatchOnly=true excludes files inside the directory from the snapshot +// but still captures the directory node itself. +// The canonical example is /tmp/apt-key-gpghome: apt-key writes temporary GPG +// key files there during a build; they must not end up in the image layer. +func TestPrefixMatchOnlyIgnoreList(t *testing.T) { + dockerfile := "Dockerfile_test_prefix_match_only" + buildImage(t, dockerfile, imageBuilder) + kanikoImage := GetKanikoImage(config.imageRepo, dockerfile) + + kanikoFiles, err := getLastLayerFiles(kanikoImage) + if err != nil { + t.Fatal(err) + } + + const ( + gpghomeDir = "tmp/apt-key-gpghome" + gpghomeFile = "tmp/apt-key-gpghome/pubring.gpg" + ) + + var hasDir, hasFile bool + for _, f := range kanikoFiles { + switch strings.TrimSuffix(f, "/") { + case gpghomeDir: + hasDir = true + case gpghomeFile: + hasFile = true + } + } + + if !hasDir { + t.Errorf("expected %s directory to be present in layer (PrefixMatchOnly=true should not exclude the directory itself), got %v", gpghomeDir, kanikoFiles) + } + if hasFile { + t.Errorf("expected %s to be excluded from layer (child of PrefixMatchOnly=true ignore entry), got %v", gpghomeFile, kanikoFiles) + } +} + +// mz560: TestIgnorePathSubtree verifies that --ignore-path excludes not just the named +// path but also any files nested beneath it. The observable difference: with an +// exact-match-only check (IsInProvidedIgnoreList), a COPY into /dest/subdir/ +// would pass the ResolvePaths input guard and appear in the layer even when +// --ignore-path=/dest is set. CheckCleanedPathAgainstProvidedIgnoreList (prefix +// match) catches the nested path and keeps it out of the layer. +func TestIgnorePathSubtree(t *testing.T) { + dockerfile := "Dockerfile_test_ignore_path_subtree" + buildImage(t, dockerfile, imageBuilder) + kanikoImage := GetKanikoImage(config.imageRepo, dockerfile) + + kanikoFiles, err := getLastLayerFiles(kanikoImage) + if err != nil { + t.Fatal(err) + } + + for _, f := range kanikoFiles { + if strings.HasPrefix(strings.TrimSuffix(f, "/"), "dest/") { + t.Errorf("file %s should be excluded from layer because it is under --ignore-path=/dest, got layer contents: %v", f, kanikoFiles) + } + } +} + func TestReplaceFolderWithFileOrLink(t *testing.T) { dockerfiles := []string{"TestReplaceFolderWithFile", "TestReplaceFolderWithLink"} for _, dockerfile := range dockerfiles { @@ -1290,6 +1357,7 @@ func initIntegrationTestConfig() *integrationTestConfig { c.onbuildBaseImage = c.imageRepo + "onbuild-base:latest" c.onbuildCopyImage = c.imageRepo + "onbuild-copy:latest" c.hardlinkBaseImage = c.imageRepo + "hardlink-base:latest" + c.hijackBaseImage = c.imageRepo + "hijack:latest" return &c } diff --git a/pkg/commands/copy.go b/pkg/commands/copy.go index e3e392951..3648d2924 100644 --- a/pkg/commands/copy.go +++ b/pkg/commands/copy.go @@ -150,6 +150,10 @@ func (c *CopyCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.Bu if err != nil { return fmt.Errorf("find destination path: %w", err) } + if util.CheckIgnoreList(destPath) { + logrus.Debugf("Skipping copy for ignored path: %s", destPath) + return nil + } srcFile := strings.NewReader(src.Data) err = util.CreateFile(destPath, srcFile, chmod, uint32(uid), uint32(gid)) diff --git a/pkg/commands/run.go b/pkg/commands/run.go index b00951706..0c8d92854 100644 --- a/pkg/commands/run.go +++ b/pkg/commands/run.go @@ -77,6 +77,7 @@ func runCommandWithFlags(config *v1.Config, buildArgs *dockerfile.BuildArgs, cmd switch { // https://docs.docker.com/reference/dockerfile/#run---mounttypecache case m.Type == instructions.MountTypeCache: + assertProtectedKanikoDir(m.Target) cacheId := m.CacheID if cacheId == "" { cacheId = filepath.Clean(m.Target) @@ -177,6 +178,7 @@ func runCommandWithFlags(config *v1.Config, buildArgs *dockerfile.BuildArgs, cmd if target == "" { target = fmt.Sprintf("/run/secrets/%s", secretId) } + assertProtectedKanikoDir(target) parent := filepath.Dir(target) created, err := ensureDir(parent) if err != nil { @@ -514,6 +516,18 @@ func ensureDir(target string) (string, error) { return firstCreated, nil } +func assertProtectedKanikoDir(target string) { + wl := []util.IgnoreListEntry{ + { + Path: kConfig.KanikoDir, + PrefixMatchOnly: false, + }, + } + if util.CheckCleanedPathAgainstProvidedIgnoreList(target, wl) { + logrus.Fatalf("mount option targetting protected KanikoDir (%s), this could be indicative of a hijacking attempt", kConfig.KanikoDir) + } +} + func assignIfNil(dst *error, fn func() error) { if err := fn(); err != nil && *dst == nil { *dst = err diff --git a/pkg/executor/build.go b/pkg/executor/build.go index fdb50071a..099febc27 100644 --- a/pkg/executor/build.go +++ b/pkg/executor/build.go @@ -82,7 +82,7 @@ func makeSnapshotter(opts *config.KanikoOptions) (*snapshot.Snapshotter, error) return nil, err } l := snapshot.NewLayeredMap(hasher) - return snapshot.NewSnapshotter(l, config.RootDir), nil + return snapshot.NewSnapshotter(l, config.RootDir, util.IgnoreList()), nil } // newStageBuilder returns a new type stageBuilder which contains all the information required to build the stage @@ -338,6 +338,13 @@ func (s *stageBuilder) build(compositeKey CompositeCache, opts *config.KanikoOpt timing.DefaultRun.Stop(t) initSnapshotTaken = true } + kanikoDirSnapshotter := snapshot.NewSnapshotter(snapshot.NewLayeredMap(util.Hasher()), config.KanikoDir, []util.IgnoreListEntry{ + { + Path: config.KanikoCacheDir, + PrefixMatchOnly: true, + }, + }) + kanikoDirSnapshotter.Init() cacheGroup := errgroup.Group{} for index, command := range s.cmds { @@ -381,9 +388,27 @@ func (s *stageBuilder) build(compositeKey CompositeCache, opts *config.KanikoOpt initSnapshotTaken = true } + if !command.ProvidesFilesToSnapshot() { + _, _, err = kanikoDirSnapshotter.ScanFullFilesystem() + if err != nil { + return err + } + } + if err := command.ExecuteCommand(&s.cf.Config, s.args); err != nil { return fmt.Errorf("failed to execute command: %w", err) } + + if !command.ProvidesFilesToSnapshot() { + add, del, err := kanikoDirSnapshotter.ScanFullFilesystem() + if err != nil { + return err + } + if len(add) > 0 || len(del) > 0 { + logrus.Fatalf("We noticed a diff in the KanikoDir (%s), this could be indicative of a hijacking attempt", config.KanikoDir) + } + } + files = command.FilesToSnapshot() timing.DefaultRun.Stop(t) diff --git a/pkg/filesystem/resolve.go b/pkg/filesystem/resolve.go index b0cb9bcea..654223962 100644 --- a/pkg/filesystem/resolve.go +++ b/pkg/filesystem/resolve.go @@ -42,7 +42,7 @@ func ResolvePaths(paths []string, wl []util.IgnoreListEntry) (pathsToAdd []strin for _, f := range paths { // If the given path is part of the ignorelist ignore it - if util.IsInProvidedIgnoreList(f, wl) { + if util.CheckCleanedPathAgainstProvidedIgnoreList(filepath.Clean(f), wl) { logrus.Debugf("Path %s is in list to ignore, ignoring it", f) continue } diff --git a/pkg/filesystem/resolve_test.go b/pkg/filesystem/resolve_test.go index d54f69640..5a8698a73 100644 --- a/pkg/filesystem/resolve_test.go +++ b/pkg/filesystem/resolve_test.go @@ -118,7 +118,7 @@ func Test_ResolvePaths(t *testing.T) { link := filepath.Join(dir, "link", f) inputFiles = append(inputFiles, link) - if util.IsInProvidedIgnoreList(link, wl) { + if util.CheckCleanedPathAgainstProvidedIgnoreList(filepath.Clean(link), wl) { t.Logf("skipping %s", link) continue } @@ -127,7 +127,7 @@ func Test_ResolvePaths(t *testing.T) { target := filepath.Join(dir, "target", f) - if util.IsInProvidedIgnoreList(target, wl) { + if util.CheckCleanedPathAgainstProvidedIgnoreList(filepath.Clean(target), wl) { t.Logf("skipping %s", target) continue } diff --git a/pkg/snapshot/snapshot.go b/pkg/snapshot/snapshot.go index d73d0fc74..f22d4b99a 100644 --- a/pkg/snapshot/snapshot.go +++ b/pkg/snapshot/snapshot.go @@ -44,14 +44,14 @@ type Snapshotter struct { } // NewSnapshotter creates a new snapshotter rooted at d -func NewSnapshotter(l *LayeredMap, d string) *Snapshotter { - return &Snapshotter{l: l, directory: d, ignorelist: util.IgnoreList()} +func NewSnapshotter(l *LayeredMap, d string, wl []util.IgnoreListEntry) *Snapshotter { + return &Snapshotter{l: l, directory: d, ignorelist: wl} } // Init initializes a new snapshotter func (s *Snapshotter) Init() error { - logrus.Info("Initializing snapshotter ...") - _, _, err := s.scanFullFilesystem() + logrus.Debug("Initializing snapshotter ...") + _, _, err := s.ScanFullFilesystem() return err } @@ -95,7 +95,7 @@ func (s *Snapshotter) TakeSnapshot(files []string, shdCheckDelete bool) (string, // Get whiteout paths var filesToWhiteout []string if shdCheckDelete { - _, deletedFiles, err := util.WalkFS(s.directory, s.l.GetCurrentPaths(), func(s string) (bool, error) { + _, deletedFiles, err := util.WalkFS(s.directory, s.l.GetCurrentPaths(), s.ignorelist, func(s string) (bool, error) { return true, nil }) if err != nil { @@ -138,7 +138,7 @@ func (s *Snapshotter) TakeSnapshotFS() (string, error) { t := util.NewTar(f) defer t.Close() - filesToAdd, filesToWhiteOut, err := s.scanFullFilesystem() + filesToAdd, filesToWhiteOut, err := s.ScanFullFilesystem() if err != nil { return "", err } @@ -156,8 +156,8 @@ func (s *Snapshotter) getSnashotPathPrefix() string { return snapshotPathPrefix } -func (s *Snapshotter) scanFullFilesystem() ([]string, []string, error) { - logrus.Info("Taking snapshot of full filesystem...") +func (s *Snapshotter) ScanFullFilesystem() ([]string, []string, error) { + logrus.Debugf("Taking snapshot of %s filesystem...", s.directory) // Some of the operations that follow (e.g. hashing) depend on the file system being synced, // for example the hashing function that determines if files are equal uses the mtime of the files, @@ -188,25 +188,16 @@ func (s *Snapshotter) scanFullFilesystem() ([]string, []string, error) { logrus.Debugf("Current image filesystem: %v", s.l.currentImage) - changedPaths, deletedPaths, err := util.WalkFS(s.directory, s.l.GetCurrentPaths(), s.l.CheckFileChange) + changedPaths, deletedPaths, err := util.WalkFS(s.directory, s.l.GetCurrentPaths(), s.ignorelist, s.l.CheckFileChange) if err != nil { return nil, nil, err } timer := timing.Start("Resolving Paths") - filesToAdd := []string{} - resolvedFiles, err := filesystem.ResolvePaths(changedPaths, s.ignorelist) + filesToAdd, err := filesystem.ResolvePaths(changedPaths, s.ignorelist) if err != nil { return nil, nil, err } - for _, path := range resolvedFiles { - if util.CheckIgnoreList(path) { - logrus.Debugf("Not adding %s to layer, as it's ignored", path) - continue - } - filesToAdd = append(filesToAdd, path) - } - logrus.Debugf("Adding to layer: %v", filesToAdd) logrus.Debugf("Deleting in layer: %v", deletedPaths) diff --git a/pkg/snapshot/snapshot_test.go b/pkg/snapshot/snapshot_test.go index 49eccfd0b..becbd1c3f 100644 --- a/pkg/snapshot/snapshot_test.go +++ b/pkg/snapshot/snapshot_test.go @@ -619,7 +619,7 @@ func setUpTest(t *testing.T) (string, *Snapshotter, func(), error) { // Take the initial snapshot l := NewLayeredMap(util.Hasher()) - snapshotter := NewSnapshotter(l, testDir) + snapshotter := NewSnapshotter(l, testDir, util.IgnoreList()) if err := snapshotter.Init(); err != nil { return "", nil, nil, fmt.Errorf("initializing snapshotter: %w", err) } diff --git a/pkg/util/fs_util.go b/pkg/util/fs_util.go index 47f3be86e..30a2890b8 100644 --- a/pkg/util/fs_util.go +++ b/pkg/util/fs_util.go @@ -419,24 +419,9 @@ func ExtractFile(dest string, hdr *tar.Header, cleanedName string, tr io.Reader) return nil } -func IsInProvidedIgnoreList(path string, wl []IgnoreListEntry) bool { - path = filepath.Clean(path) - for _, entry := range wl { - if !entry.PrefixMatchOnly && path == entry.Path { - return true - } - } - - return false -} - -func IsInIgnoreList(path string) bool { - return IsInProvidedIgnoreList(path, ignorelist) -} - func CheckCleanedPathAgainstProvidedIgnoreList(path string, wl []IgnoreListEntry) bool { - for _, wl := range ignorelist { - if hasCleanedFilepathPrefix(path, wl.Path, wl.PrefixMatchOnly) { + for _, w := range wl { + if hasCleanedFilepathPrefix(path, w.Path, w.PrefixMatchOnly) { return true } } @@ -790,6 +775,10 @@ func CopySymlink(src, dest string, context FileContext) (bool, error) { logrus.Debugf("%s found in .dockerignore, ignoring", src) return true, nil } + if CheckIgnoreList(dest) { + logrus.Debugf("Skipping copy for ignored path: %s", dest) + return true, nil + } if FilepathExists(dest) { if err := os.RemoveAll(dest); err != nil { return false, err @@ -811,6 +800,10 @@ func CopyFile(src, dest string, context FileContext, uid, gid int64, chmod fs.Fi logrus.Debugf("%s found in .dockerignore, ignoring", src) return true, nil } + if CheckIgnoreList(dest) { + logrus.Debugf("Skipping copy for ignored path: %s", dest) + return true, nil + } if src == dest { // This is a no-op. Move on, but don't list it as ignored. // We have to make sure we do this so we don't overwrite our own file. @@ -1262,6 +1255,7 @@ type walkFSResult struct { func WalkFS( dir string, existingPaths map[string]struct{}, + wl []IgnoreListEntry, changeFunc func(string) (bool, error), ) ([]string, map[string]struct{}, error) { timeOutStr := os.Getenv(snapshotTimeout) @@ -1278,7 +1272,7 @@ func WalkFS( ch := make(chan walkFSResult, 1) go func() { - filesAdded, existingPaths, err := gowalkDir(dir, existingPaths, changeFunc) + filesAdded, existingPaths, err := gowalkDir(dir, existingPaths, wl, changeFunc) ch <- walkFSResult{filesAdded, existingPaths, err} }() @@ -1294,7 +1288,7 @@ func WalkFS( } } -func gowalkDir(dir string, existingPaths map[string]struct{}, changeFunc func(string) (bool, error)) ([]string, map[string]struct{}, error) { +func gowalkDir(dir string, existingPaths map[string]struct{}, wl []IgnoreListEntry, changeFunc func(string) (bool, error)) ([]string, map[string]struct{}, error) { foundPaths := make([]string, 0) deletedFiles := existingPaths // Make a reference. @@ -1304,7 +1298,7 @@ func gowalkDir(dir string, existingPaths map[string]struct{}, changeFunc func(st return err } - if IsInIgnoreList(path) { + if CheckCleanedPathAgainstProvidedIgnoreList(filepath.Clean(path), wl) { if IsDestDir(path) && info.IsDir() { logrus.Tracef("Skipping paths under '%s', as it is an ignored directory", path) return filepath.SkipDir