diff --git a/internal/cli/build.go b/internal/cli/build.go index e71466d..0f7ba28 100644 --- a/internal/cli/build.go +++ b/internal/cli/build.go @@ -662,8 +662,8 @@ func cacheFromForDockerfileBuild(flagCacheFrom []string, cfg *config.DevContaine if cfg == nil || cfg.Build == nil || len(cfg.Build.CacheFrom) == 0 { return flagCacheFrom } - combined := make([]string, 0, len(flagCacheFrom)+len(cfg.Build.CacheFrom)) - combined = append(combined, flagCacheFrom...) + combined := make([]string, len(flagCacheFrom)) + copy(combined, flagCacheFrom) combined = append(combined, cfg.Build.CacheFrom...) return combined } diff --git a/internal/cli/build_cachefrom_test.go b/internal/cli/build_cachefrom_test.go index 5ff6d7e..bddd86d 100644 --- a/internal/cli/build_cachefrom_test.go +++ b/internal/cli/build_cachefrom_test.go @@ -42,10 +42,17 @@ func TestCacheFromForDockerfileBuild(t *testing.T) { // aliasing/appending into the caller's flag slice (which extendImageWithFeatures // still passes verbatim for the feature layers). func TestCacheFromForDockerfileBuildDoesNotMutateFlag(t *testing.T) { - flag := []string{"flag1"} + flagBacking := []string{"flag1", "sentinel"} + flag := flagBacking[:1] cfg := &config.DevContainer{Build: &config.Build{CacheFrom: config.StringOrStrings{"cfg1"}}} - _ = cacheFromForDockerfileBuild(flag, cfg) + got := cacheFromForDockerfileBuild(flag, cfg) if len(flag) != 1 || flag[0] != "flag1" { t.Fatalf("flag slice mutated: %v", flag) } + if flagBacking[1] != "sentinel" { + t.Fatalf("flag backing array mutated: %v", flagBacking) + } + if !reflect.DeepEqual(got, []string{"flag1", "cfg1"}) { + t.Fatalf("merged cache-from = %v, want [flag1 cfg1]", got) + } } diff --git a/internal/cli/extract_targz_test.go b/internal/cli/extract_targz_test.go index 57b5558..ae43eef 100644 --- a/internal/cli/extract_targz_test.go +++ b/internal/cli/extract_targz_test.go @@ -86,3 +86,25 @@ func TestExtractTarGz_ZipSlip(t *testing.T) { t.Error("zip-slip guard failed: a file was written outside the destination") } } + +// TestExtractTarGz_SymlinkEscape proves that lexical path checks are not the +// only boundary: an existing link below the extraction directory must not let +// a regular archive entry write outside it. +func TestExtractTarGz_SymlinkEscape(t *testing.T) { + arc := writeTarGz(t, [][2]string{ + {"link/escaped.txt", "pwned"}, + }) + dest := t.TempDir() + outside := t.TempDir() + if err := os.Symlink(outside, filepath.Join(dest, "link")); err != nil { + t.Skipf("symlinks are unavailable: %v", err) + } + + err := extractTarGz(arc, dest) + if err == nil { + t.Fatal("expected a symlink escape rejection, got nil") + } + if _, statErr := os.Stat(filepath.Join(outside, "escaped.txt")); !os.IsNotExist(statErr) { + t.Fatalf("symlink escape wrote outside the destination: %v", statErr) + } +} diff --git a/internal/cli/feature_install.go b/internal/cli/feature_install.go index a0c3597..5b5a203 100644 --- a/internal/cli/feature_install.go +++ b/internal/cli/feature_install.go @@ -12,6 +12,7 @@ import ( "net/http" "net/url" "os" + "path" "path/filepath" "sort" "strings" @@ -764,6 +765,12 @@ func extractTarGz(archivePath, destDir string) error { } tr := tar.NewReader(reader) + root, err := os.OpenRoot(destDir) + if err != nil { + return fmt.Errorf("open extraction root: %w", err) + } + defer root.Close() + for { header, err := tr.Next() if err == io.EOF { @@ -773,41 +780,41 @@ func extractTarGz(archivePath, destDir string) error { return fmt.Errorf("tar read: %w", err) } - cleanName := filepath.Clean(header.Name) - target := filepath.Join(destDir, cleanName) - - // Zip-slip guard: reject entries whose path escapes destDir (e.g. - // "../../etc/x"), so a malicious Feature tarball cannot write outside the - // extraction directory. filepath.Join cleans "..", so compare the result. - if target != destDir && !strings.HasPrefix(target, destDir+string(os.PathSeparator)) { + // Tar paths always use forward slashes. Localize rejects absolute paths, + // parent traversal, and names that cannot be represented safely on the + // current platform. Root also prevents escapes through symlinks already + // present below destDir. + cleanName := path.Clean(header.Name) + localName, err := filepath.Localize(cleanName) + if err != nil || !filepath.IsLocal(localName) { return fmt.Errorf("tar entry %q escapes the destination directory", header.Name) } switch header.Typeflag { case tar.TypeDir: - if err := os.MkdirAll(target, 0755); err != nil { - return err + if err := root.MkdirAll(localName, 0755); err != nil { + return fmt.Errorf("create directory for tar entry %q: %w", header.Name, err) } case tar.TypeReg: - if err := os.MkdirAll(filepath.Dir(target), 0755); err != nil { - return err + if err := root.MkdirAll(filepath.Dir(localName), 0755); err != nil { + return fmt.Errorf("create parent for tar entry %q: %w", header.Name, err) } - out, err := os.Create(target) + out, err := root.Create(localName) if err != nil { - return err + return fmt.Errorf("create tar entry %q: %w", header.Name, err) } // A truncated copy must fail the install, not silently produce a // corrupt Feature that we report as success. if _, err := io.Copy(out, tr); err != nil { out.Close() - return fmt.Errorf("extract %s: %w", target, err) + return fmt.Errorf("extract tar entry %q: %w", header.Name, err) } if err := out.Close(); err != nil { - return fmt.Errorf("close %s: %w", target, err) + return fmt.Errorf("close tar entry %q: %w", header.Name, err) } if header.Mode != 0 { - if err := os.Chmod(target, os.FileMode(header.Mode)); err != nil { - return err + if err := root.Chmod(localName, os.FileMode(header.Mode)); err != nil { + return fmt.Errorf("set mode on tar entry %q: %w", header.Name, err) } } }