Skip to content

Commit a370e4b

Browse files
tdunlap607claude
andauthored
fix(util): use securejoin for path resolution in tar extraction (#326)
* fix(util): use securejoin consistently for path resolution in tar extraction Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: tdunlap607 <trevor.dunlap@chainguard.dev> * fix(util): scope securejoin in GetFSFromLayers to whiteout entries only Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: tdunlap607 <trevor.dunlap@chainguard.dev> * fix(util): handle ELOOP gracefully in ExtractFile securejoin Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: tdunlap607 <trevor.dunlap@chainguard.dev> --------- Signed-off-by: tdunlap607 <trevor.dunlap@chainguard.dev> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 263fa1e commit a370e4b

File tree

2 files changed

+301
-4
lines changed

2 files changed

+301
-4
lines changed

pkg/util/fs_util.go

Lines changed: 45 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ import (
3333

3434
"github.com/chainguard-dev/kaniko/pkg/config"
3535
"github.com/chainguard-dev/kaniko/pkg/timing"
36+
securejoin "github.com/cyphar/filepath-securejoin"
3637
"github.com/docker/docker/pkg/archive"
3738
v1 "github.com/google/go-containerregistry/pkg/v1"
3839
"github.com/karrick/godirwalk"
@@ -189,12 +190,22 @@ func GetFSFromLayers(root string, layers []v1.Layer, opts ...FSOpt) ([]string, e
189190
}
190191

191192
cleanedName := filepath.Clean(hdr.Name)
193+
if cleanedName == ".." || strings.HasPrefix(cleanedName, "../") {
194+
return nil, fmt.Errorf("tar entry %q is not allowed: references parent directory", hdr.Name)
195+
}
192196
path := filepath.Join(root, cleanedName)
193197
base := filepath.Base(path)
194-
dir := filepath.Dir(path)
195198

196199
if strings.HasPrefix(base, archive.WhiteoutPrefix) {
197-
logrus.Tracef("Whiting out %s", path)
200+
// SecureJoin resolves symlinks and ensures the result stays
201+
// within root, which is needed because this code path calls
202+
// os.RemoveAll.
203+
securePath, err := securejoin.SecureJoin(root, cleanedName)
204+
if err != nil {
205+
return nil, fmt.Errorf("resolving whiteout path for %q: %w", hdr.Name, err)
206+
}
207+
dir := filepath.Dir(securePath)
208+
logrus.Tracef("Whiting out %s", securePath)
198209

199210
name := strings.TrimPrefix(base, archive.WhiteoutPrefix)
200211
path := filepath.Join(dir, name)
@@ -300,7 +311,20 @@ func UnTar(r io.Reader, dest string) ([]string, error) {
300311
}
301312

302313
func ExtractFile(dest string, hdr *tar.Header, cleanedName string, tr io.Reader) error {
303-
path := filepath.Join(dest, cleanedName)
314+
if cleanedName == ".." || strings.HasPrefix(cleanedName, "../") {
315+
return fmt.Errorf("tar entry %q is not allowed: references parent directory", hdr.Name)
316+
}
317+
318+
path, err := securejoin.SecureJoin(dest, cleanedName)
319+
if err != nil {
320+
// During layer extraction, symlink chains may be incomplete,
321+
// causing ELOOP. Fall back to the lexical path — the OS will
322+
// encounter the same resolution failure if the path is used.
323+
if !errors.Is(err, syscall.ELOOP) {
324+
return fmt.Errorf("resolving path for %q: %w", hdr.Name, err)
325+
}
326+
path = filepath.Join(dest, cleanedName)
327+
}
304328
base := filepath.Base(path)
305329
dir := filepath.Dir(path)
306330
mode := hdr.FileInfo().Mode()
@@ -388,13 +412,30 @@ func ExtractFile(dest string, hdr *tar.Header, cleanedName string, tr io.Reader)
388412
return errors.Wrapf(err, "error removing %s to make way for new link", hdr.Name)
389413
}
390414
}
391-
link := filepath.Clean(filepath.Join(dest, hdr.Linkname))
415+
cleanedLink := filepath.Clean(hdr.Linkname)
416+
if cleanedLink == ".." || strings.HasPrefix(cleanedLink, "../") {
417+
return fmt.Errorf("hardlink target %q is not allowed: references parent directory", hdr.Linkname)
418+
}
419+
link, err := securejoin.SecureJoin(dest, hdr.Linkname)
420+
if err != nil {
421+
return fmt.Errorf("invalid hardlink target %q: %w", hdr.Linkname, err)
422+
}
392423
if err := os.Link(link, path); err != nil {
393424
return err
394425
}
395426

396427
case tar.TypeSymlink:
397428
logrus.Tracef("Symlink from %s to %s", hdr.Linkname, path)
429+
// Resolve the symlink target relative to the entry's parent directory
430+
// to get the effective path from the extraction root, then verify it
431+
// stays within the destination.
432+
effectivePath := filepath.Clean(filepath.Join(filepath.Dir(cleanedName), hdr.Linkname))
433+
if filepath.IsAbs(hdr.Linkname) {
434+
effectivePath = filepath.Clean(hdr.Linkname)
435+
}
436+
if effectivePath == ".." || strings.HasPrefix(effectivePath, "../") {
437+
return fmt.Errorf("symlink target %q resolves outside destination", hdr.Linkname)
438+
}
398439
// The base directory for a symlink may not exist before it is created.
399440
if err := os.MkdirAll(dir, 0o755); err != nil {
400441
return err

pkg/util/fs_util_test.go

Lines changed: 256 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -820,6 +820,262 @@ func TestExtractFile(t *testing.T) {
820820
}
821821
}
822822

823+
func TestExtractFile_PathTraversal(t *testing.T) {
824+
defaultTestTime, err := time.Parse(time.RFC3339, "1912-06-23T00:00:00Z")
825+
if err != nil {
826+
t.Fatal(err)
827+
}
828+
829+
t.Run("regular file with dotdot", func(t *testing.T) {
830+
dest := t.TempDir()
831+
hdr := fileHeader("../outside.txt", "data", 0o644, defaultTestTime)
832+
err := ExtractFile(dest, hdr, filepath.Clean(hdr.Name), bytes.NewReader([]byte("data")))
833+
if err == nil {
834+
t.Fatal("expected error for parent directory reference, got nil")
835+
}
836+
if _, statErr := os.Stat(filepath.Join(dest, "..", "outside.txt")); statErr == nil {
837+
t.Fatal("file was written outside dest")
838+
}
839+
})
840+
841+
t.Run("directory with dotdot", func(t *testing.T) {
842+
dest := t.TempDir()
843+
hdr := dirHeader("../outsidedir", 0o755)
844+
err := ExtractFile(dest, hdr, filepath.Clean(hdr.Name), bytes.NewReader(nil))
845+
if err == nil {
846+
t.Fatal("expected error for parent directory reference, got nil")
847+
}
848+
})
849+
850+
t.Run("nested dotdot", func(t *testing.T) {
851+
dest := t.TempDir()
852+
hdr := fileHeader("foo/../../outside.txt", "data", 0o644, defaultTestTime)
853+
err := ExtractFile(dest, hdr, filepath.Clean(hdr.Name), bytes.NewReader([]byte("data")))
854+
if err == nil {
855+
t.Fatal("expected error for parent directory reference, got nil")
856+
}
857+
})
858+
859+
t.Run("hardlink target outside dest", func(t *testing.T) {
860+
dest := t.TempDir()
861+
legitimateHdr := fileHeader("./legit.txt", "hello", 0o644, defaultTestTime)
862+
if err := ExtractFile(dest, legitimateHdr, filepath.Clean(legitimateHdr.Name), bytes.NewReader([]byte("hello"))); err != nil {
863+
t.Fatal(err)
864+
}
865+
hdr := hardlinkHeader("./link", "../somefile")
866+
err := ExtractFile(dest, hdr, filepath.Clean(hdr.Name), bytes.NewReader(nil))
867+
if err == nil {
868+
t.Fatal("expected error for hardlink target outside dest, got nil")
869+
}
870+
if !strings.Contains(err.Error(), "references parent directory") {
871+
t.Fatalf("expected 'references parent directory' in error, got: %v", err)
872+
}
873+
})
874+
875+
t.Run("hardlink with absolute target is confined", func(t *testing.T) {
876+
dest := t.TempDir()
877+
// Create a file inside dest so there is something to link to after
878+
// securejoin confines the absolute path.
879+
legitimateHdr := fileHeader("etc/passwd", "confined", 0o644, defaultTestTime)
880+
if err := ExtractFile(dest, legitimateHdr, filepath.Clean(legitimateHdr.Name), bytes.NewReader([]byte("confined"))); err != nil {
881+
t.Fatal(err)
882+
}
883+
// Hardlink with absolute target — securejoin should confine it to dest.
884+
hdr := hardlinkHeader("./link", "/etc/passwd")
885+
err := ExtractFile(dest, hdr, filepath.Clean(hdr.Name), bytes.NewReader(nil))
886+
if err != nil {
887+
t.Fatalf("hardlink with absolute target should be confined, not rejected: %v", err)
888+
}
889+
// The link must resolve inside dest, not to the real /etc/passwd.
890+
got, err := os.ReadFile(filepath.Join(dest, "link"))
891+
if err != nil {
892+
t.Fatalf("reading link: %v", err)
893+
}
894+
if string(got) != "confined" {
895+
t.Fatalf("link content = %q, want %q (should point inside dest)", got, "confined")
896+
}
897+
})
898+
899+
t.Run("file through symlink is confined", func(t *testing.T) {
900+
dest := t.TempDir()
901+
outsideDir := t.TempDir()
902+
903+
// Create a symlink inside dest that points outside.
904+
symlinkHdr := linkHeader("./extlink", outsideDir)
905+
err := ExtractFile(dest, symlinkHdr, filepath.Clean(symlinkHdr.Name), bytes.NewReader(nil))
906+
if err != nil {
907+
t.Fatalf("symlink creation should succeed (absolute target is allowed): %v", err)
908+
}
909+
910+
// Verify the symlink was actually created on disk.
911+
target, err := os.Readlink(filepath.Join(dest, "extlink"))
912+
if err != nil {
913+
t.Fatalf("symlink was not created: %v", err)
914+
}
915+
if target != outsideDir {
916+
t.Fatalf("symlink target = %q, want %q", target, outsideDir)
917+
}
918+
919+
// Try to write a file through the symlink.
920+
writeHdr := fileHeader("./extlink/written.txt", "data", 0o644, defaultTestTime)
921+
_ = ExtractFile(dest, writeHdr, filepath.Clean(writeHdr.Name), bytes.NewReader([]byte("data")))
922+
923+
// The file must not appear outside dest.
924+
if _, statErr := os.Stat(filepath.Join(outsideDir, "written.txt")); statErr == nil {
925+
t.Fatal("file was written outside dest through symlink")
926+
}
927+
})
928+
929+
t.Run("symlink target outside dest via relative path", func(t *testing.T) {
930+
dest := t.TempDir()
931+
hdr := linkHeader("./link", "../outside")
932+
err := ExtractFile(dest, hdr, filepath.Clean(hdr.Name), bytes.NewReader(nil))
933+
if err == nil {
934+
t.Fatal("expected error for symlink target outside dest, got nil")
935+
}
936+
if !strings.Contains(err.Error(), "resolves outside destination") {
937+
t.Fatalf("unexpected error: %v", err)
938+
}
939+
})
940+
941+
t.Run("absolute symlink target resolves within dest", func(t *testing.T) {
942+
dest := t.TempDir()
943+
hdr := linkHeader("./link", "/subdir/target")
944+
err := ExtractFile(dest, hdr, filepath.Clean(hdr.Name), bytes.NewReader(nil))
945+
if err != nil {
946+
t.Fatalf("absolute symlink within dest should be allowed: %v", err)
947+
}
948+
})
949+
950+
t.Run("relative symlink within dest is allowed", func(t *testing.T) {
951+
dest := t.TempDir()
952+
os.MkdirAll(filepath.Join(dest, "foo"), 0o755)
953+
hdr := linkHeader("./foo/link", "../bar")
954+
err := ExtractFile(dest, hdr, filepath.Clean(hdr.Name), bytes.NewReader(nil))
955+
if err != nil {
956+
t.Fatalf("symlink within dest should be allowed: %v", err)
957+
}
958+
})
959+
960+
t.Run("legitimate file succeeds", func(t *testing.T) {
961+
dest := t.TempDir()
962+
hdr := fileHeader("./subdir/file.txt", "content", 0o644, defaultTestTime)
963+
err := ExtractFile(dest, hdr, filepath.Clean(hdr.Name), bytes.NewReader([]byte("content")))
964+
if err != nil {
965+
t.Fatalf("legitimate extraction should succeed: %v", err)
966+
}
967+
got, err := os.ReadFile(filepath.Join(dest, "subdir", "file.txt"))
968+
if err != nil {
969+
t.Fatal(err)
970+
}
971+
if string(got) != "content" {
972+
t.Fatalf("file contents = %q, want %q", got, "content")
973+
}
974+
})
975+
}
976+
977+
func TestUnTar_PathTraversal(t *testing.T) {
978+
makeTar := func(t *testing.T, hdrs ...tar.Header) *bytes.Buffer {
979+
t.Helper()
980+
var buf bytes.Buffer
981+
tw := tar.NewWriter(&buf)
982+
for _, hdr := range hdrs {
983+
h := hdr
984+
if err := tw.WriteHeader(&h); err != nil {
985+
t.Fatal(err)
986+
}
987+
if h.Size > 0 {
988+
if _, err := tw.Write(make([]byte, h.Size)); err != nil {
989+
t.Fatal(err)
990+
}
991+
}
992+
}
993+
if err := tw.Close(); err != nil {
994+
t.Fatal(err)
995+
}
996+
return &buf
997+
}
998+
999+
t.Run("entry with dotdot is rejected", func(t *testing.T) {
1000+
buf := makeTar(t, tar.Header{
1001+
Name: "../outside.txt", Size: 5, Mode: 0o644, Typeflag: tar.TypeReg,
1002+
})
1003+
dest := t.TempDir()
1004+
if _, err := UnTar(buf, dest); err == nil {
1005+
t.Fatal("expected error for parent directory reference, got nil")
1006+
}
1007+
// Verify no file was written outside dest.
1008+
if _, err := os.Stat(filepath.Join(dest, "..", "outside.txt")); err == nil {
1009+
t.Fatal("file was written outside dest")
1010+
}
1011+
})
1012+
1013+
t.Run("symlink with dotdot target is rejected", func(t *testing.T) {
1014+
buf := makeTar(t, tar.Header{
1015+
Name: "ext-link", Typeflag: tar.TypeSymlink, Linkname: "../../etc/passwd",
1016+
})
1017+
dest := t.TempDir()
1018+
if _, err := UnTar(buf, dest); err == nil {
1019+
t.Fatal("expected error for symlink with dotdot target, got nil")
1020+
}
1021+
})
1022+
1023+
t.Run("hardlink with dotdot target is rejected", func(t *testing.T) {
1024+
buf := makeTar(t,
1025+
tar.Header{
1026+
Name: "legit.txt", Size: 5, Mode: 0o644, Typeflag: tar.TypeReg,
1027+
Uid: os.Getuid(), Gid: os.Getgid(),
1028+
},
1029+
tar.Header{
1030+
Name: "link", Typeflag: tar.TypeLink, Linkname: "../etc/passwd",
1031+
},
1032+
)
1033+
dest := t.TempDir()
1034+
if _, err := UnTar(buf, dest); err == nil {
1035+
t.Fatal("expected error for hardlink with dotdot target, got nil")
1036+
}
1037+
})
1038+
1039+
t.Run("file through symlink is confined", func(t *testing.T) {
1040+
outsideDir := t.TempDir()
1041+
dest := t.TempDir()
1042+
1043+
// Tar contains a symlink pointing to an absolute path outside dest,
1044+
// followed by a file written under that symlink name.
1045+
buf := makeTar(t,
1046+
tar.Header{
1047+
Name: "extlink", Typeflag: tar.TypeSymlink, Linkname: outsideDir,
1048+
},
1049+
tar.Header{
1050+
Name: "extlink/written.txt", Size: 5, Mode: 0o644, Typeflag: tar.TypeReg,
1051+
Uid: os.Getuid(), Gid: os.Getgid(),
1052+
},
1053+
)
1054+
// Extraction may or may not return an error; either way the file
1055+
// must not appear outside dest.
1056+
UnTar(buf, dest)
1057+
1058+
if _, err := os.Stat(filepath.Join(outsideDir, "written.txt")); err == nil {
1059+
t.Fatal("file was written outside dest through symlink")
1060+
}
1061+
})
1062+
1063+
t.Run("legitimate entries succeed", func(t *testing.T) {
1064+
buf := makeTar(t, tar.Header{
1065+
Name: "subdir/file.txt", Size: 5, Mode: 0o644, Typeflag: tar.TypeReg,
1066+
Uid: os.Getuid(), Gid: os.Getgid(),
1067+
})
1068+
dest := t.TempDir()
1069+
files, err := UnTar(buf, dest)
1070+
if err != nil {
1071+
t.Fatalf("legitimate extraction should succeed: %v", err)
1072+
}
1073+
if len(files) != 1 {
1074+
t.Fatalf("expected 1 file, got %d", len(files))
1075+
}
1076+
})
1077+
}
1078+
8231079
func TestCopySymlink(t *testing.T) {
8241080
type tc struct {
8251081
name string

0 commit comments

Comments
 (0)