From 201e6fc19f5e6f4f92295e6d91a05f09ae7cf7df Mon Sep 17 00:00:00 2001 From: cl-ment Date: Sun, 23 Aug 2026 09:25:48 +0200 Subject: [PATCH 1/9] security(update): prevent tar extraction from traversing escaping symlinks (fixes #920) When extracting tar archives, lexical path cleaning alone does not prevent subsequent file entries from being written through a previously extracted directory symlink that resolves outside destDir (Zip/Tar Slip). This introduces verifyNoSymlinkEscape in safeExtractPath, which recursively inspects existing path components between destDir and target using os.Lstat and filepath.EvalSymlinks to reject entries attempting to write through escaping symlinks. --- internal/update/extract.go | 36 +++++++++++++++++++++++++++++++++ internal/update/extract_test.go | 35 ++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+) diff --git a/internal/update/extract.go b/internal/update/extract.go index 84b86ecef..3e1740b82 100644 --- a/internal/update/extract.go +++ b/internal/update/extract.go @@ -167,9 +167,45 @@ func safeExtractPath(destDir string, name string) (string, error) { if target != destDirClean && !strings.HasPrefix(target, destDirClean+string(os.PathSeparator)) { return "", fmt.Errorf("archive entry escapes destination: %s", name) } + if err := verifyNoSymlinkEscape(destDirClean, target); err != nil { + return "", err + } return target, nil } +func verifyNoSymlinkEscape(destDirClean string, target string) error { + rel, err := filepath.Rel(destDirClean, target) + if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { + return fmt.Errorf("archive entry escapes destination: %s", target) + } + current := destDirClean + parts := strings.Split(rel, string(os.PathSeparator)) + for _, part := range parts { + if part == "" || part == "." { + continue + } + current = filepath.Join(current, part) + info, err := os.Lstat(current) + if err != nil { + if os.IsNotExist(err) { + break + } + return err + } + if info.Mode()&os.ModeSymlink != 0 { + resolved, err := filepath.EvalSymlinks(current) + if err != nil { + return err + } + resolved = filepath.Clean(resolved) + if resolved != destDirClean && !strings.HasPrefix(resolved, destDirClean+string(os.PathSeparator)) { + return fmt.Errorf("archive symlink %s escapes destination: %s", current, resolved) + } + } + } + return nil +} + // findByBasename recursively searches root for the first regular file whose // basename matches name, mirroring scripts/postinstall.mjs's lookup so // helper binaries nested under archive subdirectories (e.g. helpers/) are diff --git a/internal/update/extract_test.go b/internal/update/extract_test.go index 1742866ea..f516bb140 100644 --- a/internal/update/extract_test.go +++ b/internal/update/extract_test.go @@ -308,3 +308,38 @@ func TestExtractTarGzRejectsEscapingSymlink(t *testing.T) { t.Fatal("expected error extracting escaping symlink") } } + +func TestExtractTarGzRejectsChainedSymlinkEscapingFile(t *testing.T) { + if !symlinksSupported(t) { + t.Skip("symlinks not supported") + } + dir := t.TempDir() + archivePath := filepath.Join(dir, "archive.tar.gz") + + file, err := os.Create(archivePath) + if err != nil { + t.Fatalf("Create: %v", err) + } + gw := gzip.NewWriter(file) + tw := tar.NewWriter(gw) + + // Symlink pointing outside + h1 := &tar.Header{ + Name: "sub/link_dir", + Typeflag: tar.TypeSymlink, + Linkname: "../../outside_dir", + } + if err := tw.WriteHeader(h1); err != nil { + t.Fatalf("WriteHeader h1: %v", err) + } + + tw.Close() + gw.Close() + file.Close() + + destDir := filepath.Join(dir, "extracted") + if err := extractArchive(archivePath, destDir); err == nil { + t.Fatal("expected error extracting escaping symlink directory") + } +} + From d88c4d77a60eb7f244750efba1498e757ed94241 Mon Sep 17 00:00:00 2001 From: cl-ment Date: Sun, 23 Aug 2026 10:16:35 +0200 Subject: [PATCH 2/9] test(update): exercise chained symlink escape during file extract The previous case was a single escaping symlink already rejected by the existing Linkname check. Seed a mid -> outside chain under destDir and extract a regular file through it so verifyNoSymlinkEscape is what fails, with no write outside destDir. --- internal/update/extract_test.go | 42 ++++++++++++++++----------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/internal/update/extract_test.go b/internal/update/extract_test.go index f516bb140..0304f5c13 100644 --- a/internal/update/extract_test.go +++ b/internal/update/extract_test.go @@ -314,32 +314,32 @@ func TestExtractTarGzRejectsChainedSymlinkEscapingFile(t *testing.T) { t.Skip("symlinks not supported") } dir := t.TempDir() - archivePath := filepath.Join(dir, "archive.tar.gz") - - file, err := os.Create(archivePath) - if err != nil { - t.Fatalf("Create: %v", err) + destDir := filepath.Join(dir, "extracted") + outsideDir := filepath.Join(dir, "outside") + if err := os.MkdirAll(destDir, 0o755); err != nil { + t.Fatalf("Mkdir dest: %v", err) } - gw := gzip.NewWriter(file) - tw := tar.NewWriter(gw) - - // Symlink pointing outside - h1 := &tar.Header{ - Name: "sub/link_dir", - Typeflag: tar.TypeSymlink, - Linkname: "../../outside_dir", + if err := os.MkdirAll(outsideDir, 0o755); err != nil { + t.Fatalf("Mkdir outside: %v", err) } - if err := tw.WriteHeader(h1); err != nil { - t.Fatalf("WriteHeader h1: %v", err) + // chain -> mid -> outsideDir. Lexical path destDir/chain/pwned.txt stays + // under destDir; EvalSymlinks of the chain does not. + if err := os.Symlink(outsideDir, filepath.Join(destDir, "mid")); err != nil { + t.Fatalf("symlink mid: %v", err) + } + if err := os.Symlink("mid", filepath.Join(destDir, "chain")); err != nil { + t.Fatalf("symlink chain: %v", err) } - tw.Close() - gw.Close() - file.Close() + archivePath := filepath.Join(dir, "archive.tar.gz") + writeTestTarGz(t, archivePath, map[string]string{ + "chain/pwned.txt": "escaped", + }) - destDir := filepath.Join(dir, "extracted") if err := extractArchive(archivePath, destDir); err == nil { - t.Fatal("expected error extracting escaping symlink directory") + t.Fatal("expected error extracting a file through a chained escaping symlink") + } + if _, err := os.Stat(filepath.Join(outsideDir, "pwned.txt")); !os.IsNotExist(err) { + t.Fatalf("escaped file exists outside destDir: %v", err) } } - From 9ec4c2ba5598dde37f469a5a14247be7a0f28eca Mon Sep 17 00:00:00 2001 From: cl-ment Date: Mon, 24 Aug 2026 18:30:20 +0200 Subject: [PATCH 3/9] fix(update): resolve destDir symlinks and permit valid dangling symlinks in archive --- internal/update/extract.go | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/internal/update/extract.go b/internal/update/extract.go index 3e1740b82..cbadc59f3 100644 --- a/internal/update/extract.go +++ b/internal/update/extract.go @@ -178,6 +178,13 @@ func verifyNoSymlinkEscape(destDirClean string, target string) error { if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { return fmt.Errorf("archive entry escapes destination: %s", target) } + destDirResolved, err := filepath.EvalSymlinks(destDirClean) + if err != nil { + destDirResolved = destDirClean + } else { + destDirResolved = filepath.Clean(destDirResolved) + } + current := destDirClean parts := strings.Split(rel, string(os.PathSeparator)) for _, part := range parts { @@ -195,10 +202,23 @@ func verifyNoSymlinkEscape(destDirClean string, target string) error { if info.Mode()&os.ModeSymlink != 0 { resolved, err := filepath.EvalSymlinks(current) if err != nil { - return err + if os.IsNotExist(err) { + // Dangling symlink is allowed if its relative destination stays within destDir. + linkTarget, readErr := os.Readlink(current) + if readErr != nil { + return readErr + } + if filepath.IsAbs(linkTarget) { + return fmt.Errorf("archive symlink %s has absolute target: %s", current, linkTarget) + } + resolved = filepath.Clean(filepath.Join(filepath.Dir(current), linkTarget)) + } else { + return err + } } resolved = filepath.Clean(resolved) - if resolved != destDirClean && !strings.HasPrefix(resolved, destDirClean+string(os.PathSeparator)) { + if resolved != destDirResolved && !strings.HasPrefix(resolved, destDirResolved+string(os.PathSeparator)) && + resolved != destDirClean && !strings.HasPrefix(resolved, destDirClean+string(os.PathSeparator)) { return fmt.Errorf("archive symlink %s escapes destination: %s", current, resolved) } } From 9645ce969f38b472ab0daebc3c927cf201582ed0 Mon Sep 17 00:00:00 2001 From: hazyhaar Date: Thu, 27 Aug 2026 15:13:31 +0200 Subject: [PATCH 4/9] docs(update): document POSIX-only scope for verifyNoSymlinkEscape --- internal/update/extract.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/update/extract.go b/internal/update/extract.go index cbadc59f3..4924d9de6 100644 --- a/internal/update/extract.go +++ b/internal/update/extract.go @@ -173,6 +173,12 @@ func safeExtractPath(destDir string, name string) (string, error) { return target, nil } +// verifyNoSymlinkEscape ensures that intermediate path segments do not traverse +// symlinks that escape destDir. +// +// Note: This is a POSIX-only control covering os.ModeSymlink. Windows NTFS +// directory junctions (reparse points reported as ModeIrregular rather than +// ModeSymlink) are not covered. func verifyNoSymlinkEscape(destDirClean string, target string) error { rel, err := filepath.Rel(destDirClean, target) if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { From d5588822ccef9b6b0ab7ab3e548634956bdae1bc Mon Sep 17 00:00:00 2001 From: hazyhaar Date: Fri, 28 Aug 2026 15:38:30 +0200 Subject: [PATCH 5/9] internal/update: test extracting files through directory symlinks --- internal/update/extract_test.go | 38 +++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/internal/update/extract_test.go b/internal/update/extract_test.go index 0304f5c13..663dd2d9d 100644 --- a/internal/update/extract_test.go +++ b/internal/update/extract_test.go @@ -257,6 +257,36 @@ func TestExtractTarGzAllowsSafeSymlink(t *testing.T) { t.Fatalf("Write target: %v", err) } + // Couvre le cas d'un sous-dossier lié avec extraction d'un fichier traversant le lien + h3 := &tar.Header{ + Name: "sublink", + Typeflag: tar.TypeSymlink, + Linkname: "subdir", + } + if err := tw.WriteHeader(h3); err != nil { + t.Fatalf("WriteHeader sublink: %v", err) + } + h4 := &tar.Header{ + Name: "subdir", + Typeflag: tar.TypeDir, + Mode: 0o755, + } + if err := tw.WriteHeader(h4); err != nil { + t.Fatalf("WriteHeader subdir: %v", err) + } + h5 := &tar.Header{ + Name: "sublink/nested.txt", + Typeflag: tar.TypeReg, + Mode: 0o644, + Size: 6, + } + if err := tw.WriteHeader(h5); err != nil { + t.Fatalf("WriteHeader nested: %v", err) + } + if _, err := tw.Write([]byte("nested")); err != nil { + t.Fatalf("Write nested: %v", err) + } + tw.Close() gw.Close() file.Close() @@ -274,6 +304,14 @@ func TestExtractTarGzAllowsSafeSymlink(t *testing.T) { if gotLink != "target.txt" { t.Fatalf("link target = %q, want %q", gotLink, "target.txt") } + + nestedData, err := os.ReadFile(filepath.Join(destDir, "subdir", "nested.txt")) + if err != nil { + t.Fatalf("ReadFile nested: %v", err) + } + if string(nestedData) != "nested" { + t.Fatalf("nestedData = %q, want %q", string(nestedData), "nested") + } } func TestExtractTarGzRejectsEscapingSymlink(t *testing.T) { From 39ea2fa6dd85e294757c5912dbbb7bfcaa112085 Mon Sep 17 00:00:00 2001 From: hazyhaar Date: Sat, 29 Aug 2026 10:14:46 +0200 Subject: [PATCH 6/9] fix(update): reject dangling symlinks instead of lexical fallback EvalSymlinks failure used filepath.Clean on the link text, which does not preserve already-traversed links. A dangling member is now an error. --- internal/update/extract.go | 14 +------------- internal/update/extract_test.go | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/internal/update/extract.go b/internal/update/extract.go index 4924d9de6..cfb87abf4 100644 --- a/internal/update/extract.go +++ b/internal/update/extract.go @@ -208,19 +208,7 @@ func verifyNoSymlinkEscape(destDirClean string, target string) error { if info.Mode()&os.ModeSymlink != 0 { resolved, err := filepath.EvalSymlinks(current) if err != nil { - if os.IsNotExist(err) { - // Dangling symlink is allowed if its relative destination stays within destDir. - linkTarget, readErr := os.Readlink(current) - if readErr != nil { - return readErr - } - if filepath.IsAbs(linkTarget) { - return fmt.Errorf("archive symlink %s has absolute target: %s", current, linkTarget) - } - resolved = filepath.Clean(filepath.Join(filepath.Dir(current), linkTarget)) - } else { - return err - } + return fmt.Errorf("archive symlink %s is dangling or unresolvable: %w", current, err) } resolved = filepath.Clean(resolved) if resolved != destDirResolved && !strings.HasPrefix(resolved, destDirResolved+string(os.PathSeparator)) && diff --git a/internal/update/extract_test.go b/internal/update/extract_test.go index 663dd2d9d..5a945ef70 100644 --- a/internal/update/extract_test.go +++ b/internal/update/extract_test.go @@ -381,3 +381,20 @@ func TestExtractTarGzRejectsChainedSymlinkEscapingFile(t *testing.T) { t.Fatalf("escaped file exists outside destDir: %v", err) } } + +func TestVerifyNoSymlinkEscapeRejectsDanglingLink(t *testing.T) { + if !symlinksSupported(t) { + t.Skip("symlinks not supported") + } + destDir := t.TempDir() + if err := os.Symlink("missing-target", filepath.Join(destDir, "d")); err != nil { + t.Fatal(err) + } + if err := os.Symlink("..", filepath.Join(destDir, "d", "s")); err == nil { + t.Log("created d/s through dangling d") + } + target := filepath.Join(destDir, "d", "s", "x") + if err := verifyNoSymlinkEscape(destDir, target); err == nil { + t.Fatal("expected dangling or unresolvable symlink to be rejected") + } +} From c04b5b3d9a3ff732a97b950368d91d1261d030f6 Mon Sep 17 00:00:00 2001 From: hazyhaar Date: Sun, 30 Aug 2026 17:01:52 +0200 Subject: [PATCH 7/9] fix(update): resolve dangling symlink chains from the physical parent --- internal/update/extract.go | 97 ++++++++++++++++++++++++++++-- internal/update/extract_test.go | 101 ++++++++++++++++++++++++++++---- 2 files changed, 184 insertions(+), 14 deletions(-) diff --git a/internal/update/extract.go b/internal/update/extract.go index cfb87abf4..df7256913 100644 --- a/internal/update/extract.go +++ b/internal/update/extract.go @@ -208,11 +208,17 @@ func verifyNoSymlinkEscape(destDirClean string, target string) error { if info.Mode()&os.ModeSymlink != 0 { resolved, err := filepath.EvalSymlinks(current) if err != nil { - return fmt.Errorf("archive symlink %s is dangling or unresolvable: %w", current, err) + if !os.IsNotExist(err) { + return err + } + resolved, err = resolveDanglingSymlink(destDirClean, destDirResolved, current) + if err != nil { + return err + } + } else { + resolved = filepath.Clean(resolved) } - resolved = filepath.Clean(resolved) - if resolved != destDirResolved && !strings.HasPrefix(resolved, destDirResolved+string(os.PathSeparator)) && - resolved != destDirClean && !strings.HasPrefix(resolved, destDirClean+string(os.PathSeparator)) { + if !pathInsideDest(destDirClean, destDirResolved, resolved) { return fmt.Errorf("archive symlink %s escapes destination: %s", current, resolved) } } @@ -220,6 +226,89 @@ func verifyNoSymlinkEscape(destDirClean string, target string) error { return nil } +func pathInsideDest(destDirClean, destDirResolved, path string) bool { + path = filepath.Clean(path) + sep := string(os.PathSeparator) + return path == destDirResolved || strings.HasPrefix(path, destDirResolved+sep) || + path == destDirClean || strings.HasPrefix(path, destDirClean+sep) +} + +// resolveDanglingSymlink resolves a symlink whose final target does not yet +// exist. Each already-existing link in the target chain is followed from the +// physical parent; a resolved prefix outside destDir is rejected. A missing +// suffix is allowed only when that prefix stays inside destDir. Lexical +// Join(Dir(link), target) is not used, because Dir does not preserve links +// already traversed. +func resolveDanglingSymlink(destDirClean, destDirResolved, linkPath string) (string, error) { + linkTarget, err := os.Readlink(linkPath) + if err != nil { + return "", err + } + if filepath.IsAbs(linkTarget) { + return "", fmt.Errorf("archive symlink %s has absolute target: %s", linkPath, linkTarget) + } + parentResolved, err := filepath.EvalSymlinks(filepath.Dir(linkPath)) + if err != nil { + return "", fmt.Errorf("archive symlink %s parent is unresolvable: %w", linkPath, err) + } + return walkLinkTarget(destDirClean, destDirResolved, filepath.Clean(parentResolved), linkTarget, 0) +} + +func walkLinkTarget(destDirClean, destDirResolved, base, linkTarget string, depth int) (string, error) { + if depth > 255 { + return "", fmt.Errorf("archive symlink nest exceeds limit") + } + current := base + for _, part := range strings.Split(strings.ReplaceAll(linkTarget, "\\", "/"), "/") { + if part == "" || part == "." { + continue + } + parent := current + if part == ".." { + current = filepath.Dir(current) + } else { + current = filepath.Join(current, part) + } + if !pathInsideDest(destDirClean, destDirResolved, current) { + return "", fmt.Errorf("archive symlink target escapes destination: %s", current) + } + info, err := os.Lstat(current) + if err != nil { + if os.IsNotExist(err) { + continue + } + return "", err + } + if info.Mode()&os.ModeSymlink == 0 { + continue + } + nextTarget, err := os.Readlink(current) + if err != nil { + return "", err + } + if filepath.IsAbs(nextTarget) { + return "", fmt.Errorf("archive symlink %s has absolute target: %s", current, nextTarget) + } + resolved, err := filepath.EvalSymlinks(current) + if err != nil { + if !os.IsNotExist(err) { + return "", err + } + resolved, err = walkLinkTarget(destDirClean, destDirResolved, parent, nextTarget, depth+1) + if err != nil { + return "", err + } + } else { + resolved = filepath.Clean(resolved) + } + if !pathInsideDest(destDirClean, destDirResolved, resolved) { + return "", fmt.Errorf("archive symlink %s escapes destination: %s", current, resolved) + } + current = resolved + } + return current, nil +} + // findByBasename recursively searches root for the first regular file whose // basename matches name, mirroring scripts/postinstall.mjs's lookup so // helper binaries nested under archive subdirectories (e.g. helpers/) are diff --git a/internal/update/extract_test.go b/internal/update/extract_test.go index 5a945ef70..3f82abb99 100644 --- a/internal/update/extract_test.go +++ b/internal/update/extract_test.go @@ -257,7 +257,7 @@ func TestExtractTarGzAllowsSafeSymlink(t *testing.T) { t.Fatalf("Write target: %v", err) } - // Couvre le cas d'un sous-dossier lié avec extraction d'un fichier traversant le lien + // Cover a linked subdirectory with a file extracted through the link. h3 := &tar.Header{ Name: "sublink", Typeflag: tar.TypeSymlink, @@ -382,19 +382,100 @@ func TestExtractTarGzRejectsChainedSymlinkEscapingFile(t *testing.T) { } } -func TestVerifyNoSymlinkEscapeRejectsDanglingLink(t *testing.T) { +type testTarEntry struct { + name string + typeflag byte + linkname string + body string +} + +func writeTestTarGzEntries(t *testing.T, archivePath string, entries []testTarEntry) { + t.Helper() + file, err := os.Create(archivePath) + if err != nil { + t.Fatalf("Create archive: %v", err) + } + defer func() { _ = file.Close() }() + gzipWriter := gzip.NewWriter(file) + tarWriter := tar.NewWriter(gzipWriter) + for _, entry := range entries { + header := &tar.Header{ + Name: entry.name, + Typeflag: entry.typeflag, + Linkname: entry.linkname, + Mode: 0o644, + Size: int64(len(entry.body)), + } + if entry.typeflag == tar.TypeSymlink || entry.typeflag == tar.TypeDir { + header.Size = 0 + } + if err := tarWriter.WriteHeader(header); err != nil { + t.Fatalf("WriteHeader %s: %v", entry.name, err) + } + if header.Size > 0 { + if _, err := tarWriter.Write([]byte(entry.body)); err != nil { + t.Fatalf("Write %s: %v", entry.name, err) + } + } + } + if err := tarWriter.Close(); err != nil { + t.Fatalf("close tar writer: %v", err) + } + if err := gzipWriter.Close(); err != nil { + t.Fatalf("close gzip writer: %v", err) + } +} + +// A tar can plant d -> ., then d/s -> .. (physically destDir/s -> .. because +// d is already a link), then l -> d/s/missing, then a regular file named l. +// EvalSymlinks(l) is ENOENT; lexical Join would accept destDir/d/s/missing +// while open follows d and s and writes missing beside destDir. +func TestExtractTarGzRejectsDanglingSymlinkChainEscape(t *testing.T) { if !symlinksSupported(t) { t.Skip("symlinks not supported") } - destDir := t.TempDir() - if err := os.Symlink("missing-target", filepath.Join(destDir, "d")); err != nil { - t.Fatal(err) + dir := t.TempDir() + destDir := filepath.Join(dir, "extracted") + archivePath := filepath.Join(dir, "archive.tar.gz") + writeTestTarGzEntries(t, archivePath, []testTarEntry{ + {name: "d", typeflag: tar.TypeSymlink, linkname: "."}, + {name: "d/s", typeflag: tar.TypeSymlink, linkname: ".."}, + {name: "l", typeflag: tar.TypeSymlink, linkname: "d/s/missing"}, + {name: "l", typeflag: tar.TypeReg, body: "pwned"}, + }) + extractErr := extractArchive(archivePath, destDir) + escaped := filepath.Join(dir, "missing") + _, statErr := os.Stat(escaped) + if !os.IsNotExist(statErr) { + t.Fatalf("escaped file exists outside destDir: %v", statErr) + } + if extractErr == nil { + t.Fatal("expected extractArchive to reject a dangling symlink chain that escapes destDir") + } +} + +func TestExtractTarGzAllowsSafeDanglingRelative(t *testing.T) { + if !symlinksSupported(t) { + t.Skip("symlinks not supported") + } + dir := t.TempDir() + destDir := filepath.Join(dir, "extracted") + archivePath := filepath.Join(dir, "archive.tar.gz") + writeTestTarGzEntries(t, archivePath, []testTarEntry{ + {name: "l", typeflag: tar.TypeSymlink, linkname: "not-yet-there"}, + {name: "l", typeflag: tar.TypeReg, body: "safe-content"}, + }) + if err := extractArchive(archivePath, destDir); err != nil { + t.Fatalf("extractArchive: %v", err) + } + data, err := os.ReadFile(filepath.Join(destDir, "not-yet-there")) + if err != nil { + t.Fatalf("ReadFile not-yet-there: %v", err) } - if err := os.Symlink("..", filepath.Join(destDir, "d", "s")); err == nil { - t.Log("created d/s through dangling d") + if string(data) != "safe-content" { + t.Fatalf("not-yet-there content = %q", data) } - target := filepath.Join(destDir, "d", "s", "x") - if err := verifyNoSymlinkEscape(destDir, target); err == nil { - t.Fatal("expected dangling or unresolvable symlink to be rejected") + if _, err := os.Stat(filepath.Join(dir, "not-yet-there")); err == nil { + t.Fatal("safe dangling target was written outside destDir") } } From 67f151ff14c7114cd46a2dd9f220cb7935092c68 Mon Sep 17 00:00:00 2001 From: hazyhaar Date: Sun, 30 Aug 2026 19:06:10 +0200 Subject: [PATCH 8/9] fix(update): authorize tar paths through OpenRoot and reject outbound symlink entries --- internal/update/extract.go | 259 +++++++++++++++----------------- internal/update/extract_test.go | 53 +++++++ 2 files changed, 175 insertions(+), 137 deletions(-) diff --git a/internal/update/extract.go b/internal/update/extract.go index df7256913..f3b8dc5ed 100644 --- a/internal/update/extract.go +++ b/internal/update/extract.go @@ -21,6 +21,17 @@ func extractArchive(archivePath string, destDir string) error { } func extractTarGz(archivePath string, destDir string) error { + if err := os.MkdirAll(destDir, 0o755); err != nil { + return err + } + destRoot, err := os.OpenRoot(destDir) + if err != nil { + return err + } + defer func() { + _ = destRoot.Close() + }() + file, err := os.Open(archivePath) if err != nil { return err @@ -44,37 +55,40 @@ func extractTarGz(archivePath string, destDir string) error { if err != nil { return err } - target, err := safeExtractPath(destDir, header.Name) + cleanName, err := cleanEntryPath(header.Name) if err != nil { return err } + if cleanName == "." { + continue + } switch header.Typeflag { case tar.TypeDir: - if err := os.MkdirAll(target, 0o755); err != nil { - return err + if err := destRoot.MkdirAll(cleanName, 0o755); err != nil { + return fmt.Errorf("archive directory entry %s escapes destination: %w", header.Name, err) } case tar.TypeSymlink: - if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { - return err - } - if filepath.IsAbs(header.Linkname) { - return fmt.Errorf("absolute symlink targets are not supported: %s -> %s", header.Name, header.Linkname) + parent := filepath.Dir(cleanName) + if parent != "." { + if err := destRoot.MkdirAll(parent, 0o755); err != nil { + return fmt.Errorf("archive symlink parent %s escapes destination: %w", parent, err) + } } - // Verify that the symlink target, when resolved, does not escape destDir. - resolvedTarget := filepath.Join(filepath.Dir(target), header.Linkname) - destDirClean := filepath.Clean(destDir) - if !strings.HasPrefix(resolvedTarget, destDirClean+string(os.PathSeparator)) && resolvedTarget != destDirClean { - return fmt.Errorf("archive symlink target escapes destination: %s -> %s", header.Name, header.Linkname) + if err := validateSymlinkTarget(destRoot, parent, header.Linkname); err != nil { + return fmt.Errorf("archive symlink target escapes destination: %s -> %s: %w", header.Name, header.Linkname, err) } - _ = os.Remove(target) - if err := os.Symlink(header.Linkname, target); err != nil { + _ = destRoot.Remove(cleanName) + if err := destRoot.Symlink(header.Linkname, cleanName); err != nil { return err } case tar.TypeReg: - if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { - return err + parent := filepath.Dir(cleanName) + if parent != "." { + if err := destRoot.MkdirAll(parent, 0o755); err != nil { + return fmt.Errorf("archive file parent %s escapes destination: %w", parent, err) + } } - if err := writeExtractedFile(target, tarReader, fs.FileMode(header.Mode)); err != nil { + if err := writeExtractedFile(destRoot, cleanName, tarReader, fs.FileMode(header.Mode)); err != nil { return err } default: @@ -86,6 +100,17 @@ func extractTarGz(archivePath string, destDir string) error { } func extractZip(archivePath string, destDir string) error { + if err := os.MkdirAll(destDir, 0o755); err != nil { + return err + } + destRoot, err := os.OpenRoot(destDir) + if err != nil { + return err + } + defer func() { + _ = destRoot.Close() + }() + reader, err := zip.OpenReader(archivePath) if err != nil { return err @@ -94,13 +119,16 @@ func extractZip(archivePath string, destDir string) error { _ = reader.Close() }() for _, entry := range reader.File { - target, err := safeExtractPath(destDir, entry.Name) + cleanName, err := cleanEntryPath(entry.Name) if err != nil { return err } + if cleanName == "." { + continue + } if entry.FileInfo().IsDir() { - if err := os.MkdirAll(target, 0o755); err != nil { - return err + if err := destRoot.MkdirAll(cleanName, 0o755); err != nil { + return fmt.Errorf("archive directory entry %s escapes destination: %w", entry.Name, err) } continue } @@ -117,8 +145,11 @@ func extractZip(archivePath string, destDir string) error { if !entry.Mode().IsRegular() { return fmt.Errorf("unsupported archive entry type for %s", entry.Name) } - if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { - return err + parent := filepath.Dir(cleanName) + if parent != "." { + if err := destRoot.MkdirAll(parent, 0o755); err != nil { + return fmt.Errorf("archive file parent %s escapes destination: %w", parent, err) + } } if err := func() error { entryReader, err := entry.Open() @@ -128,7 +159,7 @@ func extractZip(archivePath string, destDir string) error { defer func() { _ = entryReader.Close() }() - return writeExtractedFile(target, entryReader, entry.Mode()) + return writeExtractedFile(destRoot, cleanName, entryReader, entry.Mode()) }(); err != nil { return err } @@ -136,11 +167,12 @@ func extractZip(archivePath string, destDir string) error { return nil } -func writeExtractedFile(target string, source io.Reader, mode fs.FileMode) error { - if mode == 0 { - mode = 0o644 +func writeExtractedFile(destRoot *os.Root, name string, source io.Reader, mode fs.FileMode) error { + perm := mode.Perm() + if perm == 0 { + perm = 0o644 } - out, err := os.OpenFile(target, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, mode) + out, err := destRoot.OpenFile(name, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, perm) if err != nil { return err } @@ -152,159 +184,112 @@ func writeExtractedFile(target string, source io.Reader, mode fs.FileMode) error return closeErr } -// safeExtractPath resolves an archive entry name against destDir, rejecting -// absolute paths or entries that would escape destDir via "..". -func safeExtractPath(destDir string, name string) (string, error) { +// cleanEntryPath resolves an archive entry name, rejecting absolute paths or +// entries that would escape the destination via "..". +func cleanEntryPath(name string) (string, error) { cleanName := filepath.Clean(strings.ReplaceAll(name, "\\", "/")) if cleanName == "." { - return destDir, nil - } - if filepath.IsAbs(cleanName) || cleanName == ".." || strings.HasPrefix(cleanName, "../") { - return "", fmt.Errorf("archive entry escapes destination: %s", name) + return ".", nil } - target := filepath.Join(destDir, cleanName) - destDirClean := filepath.Clean(destDir) - if target != destDirClean && !strings.HasPrefix(target, destDirClean+string(os.PathSeparator)) { + if filepath.IsAbs(cleanName) || strings.HasPrefix(name, "/") || strings.HasPrefix(name, "\\") || + cleanName == ".." || strings.HasPrefix(cleanName, ".."+string(os.PathSeparator)) || strings.HasPrefix(cleanName, "../") || + filepath.VolumeName(cleanName) != "" || strings.Contains(cleanName, ":") { return "", fmt.Errorf("archive entry escapes destination: %s", name) } - if err := verifyNoSymlinkEscape(destDirClean, target); err != nil { - return "", err - } - return target, nil + return cleanName, nil } -// verifyNoSymlinkEscape ensures that intermediate path segments do not traverse -// symlinks that escape destDir. -// -// Note: This is a POSIX-only control covering os.ModeSymlink. Windows NTFS -// directory junctions (reparse points reported as ModeIrregular rather than -// ModeSymlink) are not covered. -func verifyNoSymlinkEscape(destDirClean string, target string) error { - rel, err := filepath.Rel(destDirClean, target) - if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { - return fmt.Errorf("archive entry escapes destination: %s", target) +func validateSymlinkTarget(root *os.Root, parentRel, linkTarget string) error { + if filepath.IsAbs(linkTarget) || strings.HasPrefix(linkTarget, "/") || strings.HasPrefix(linkTarget, "\\") || + filepath.VolumeName(linkTarget) != "" || strings.Contains(linkTarget, ":") { + return fmt.Errorf("archive symlink has absolute or invalid target: %s", linkTarget) } - destDirResolved, err := filepath.EvalSymlinks(destDirClean) - if err != nil { - destDirResolved = destDirClean - } else { - destDirResolved = filepath.Clean(destDirResolved) + base := parentRel + if base == "" { + base = "." } - - current := destDirClean - parts := strings.Split(rel, string(os.PathSeparator)) - for _, part := range parts { - if part == "" || part == "." { - continue - } - current = filepath.Join(current, part) - info, err := os.Lstat(current) - if err != nil { - if os.IsNotExist(err) { - break - } - return err - } - if info.Mode()&os.ModeSymlink != 0 { - resolved, err := filepath.EvalSymlinks(current) - if err != nil { - if !os.IsNotExist(err) { - return err - } - resolved, err = resolveDanglingSymlink(destDirClean, destDirResolved, current) - if err != nil { - return err - } - } else { - resolved = filepath.Clean(resolved) - } - if !pathInsideDest(destDirClean, destDirResolved, resolved) { - return fmt.Errorf("archive symlink %s escapes destination: %s", current, resolved) - } - } + resolvedBase, err := followUnderRoot(root, base, 0) + if err != nil { + return err } - return nil -} - -func pathInsideDest(destDirClean, destDirResolved, path string) bool { - path = filepath.Clean(path) - sep := string(os.PathSeparator) - return path == destDirResolved || strings.HasPrefix(path, destDirResolved+sep) || - path == destDirClean || strings.HasPrefix(path, destDirClean+sep) + _, err = walkUnderRoot(root, resolvedBase, linkTarget, 0) + return err } -// resolveDanglingSymlink resolves a symlink whose final target does not yet -// exist. Each already-existing link in the target chain is followed from the -// physical parent; a resolved prefix outside destDir is rejected. A missing -// suffix is allowed only when that prefix stays inside destDir. Lexical -// Join(Dir(link), target) is not used, because Dir does not preserve links -// already traversed. -func resolveDanglingSymlink(destDirClean, destDirResolved, linkPath string) (string, error) { - linkTarget, err := os.Readlink(linkPath) +func followUnderRoot(root *os.Root, rel string, depth int) (string, error) { + if depth > 255 { + return "", fmt.Errorf("archive symlink nest exceeds limit") + } + if rel == "" || rel == "." { + return ".", nil + } + info, err := root.Lstat(rel) if err != nil { + if os.IsNotExist(err) { + return rel, nil + } return "", err } - if filepath.IsAbs(linkTarget) { - return "", fmt.Errorf("archive symlink %s has absolute target: %s", linkPath, linkTarget) + if info.Mode()&os.ModeSymlink == 0 { + return rel, nil } - parentResolved, err := filepath.EvalSymlinks(filepath.Dir(linkPath)) + tgt, err := root.Readlink(rel) if err != nil { - return "", fmt.Errorf("archive symlink %s parent is unresolvable: %w", linkPath, err) + return "", err } - return walkLinkTarget(destDirClean, destDirResolved, filepath.Clean(parentResolved), linkTarget, 0) + if filepath.IsAbs(tgt) || strings.HasPrefix(tgt, "/") { + return "", fmt.Errorf("archive symlink %s has absolute target: %s", rel, tgt) + } + parent := filepath.Dir(rel) + if parent == "" { + parent = "." + } + return walkUnderRoot(root, parent, tgt, depth+1) } -func walkLinkTarget(destDirClean, destDirResolved, base, linkTarget string, depth int) (string, error) { +func walkUnderRoot(root *os.Root, base, linkTarget string, depth int) (string, error) { if depth > 255 { return "", fmt.Errorf("archive symlink nest exceeds limit") } current := base + if current == "" { + current = "." + } for _, part := range strings.Split(strings.ReplaceAll(linkTarget, "\\", "/"), "/") { if part == "" || part == "." { continue } - parent := current if part == ".." { + if current == "." { + return "", fmt.Errorf("archive symlink target escapes destination") + } current = filepath.Dir(current) - } else { - current = filepath.Join(current, part) + if current == "" { + current = "." + } + continue } - if !pathInsideDest(destDirClean, destDirResolved, current) { - return "", fmt.Errorf("archive symlink target escapes destination: %s", current) + next := part + if current != "." { + next = filepath.Join(current, part) } - info, err := os.Lstat(current) + info, err := root.Lstat(next) if err != nil { if os.IsNotExist(err) { + current = next continue } return "", err } if info.Mode()&os.ModeSymlink == 0 { + current = next continue } - nextTarget, err := os.Readlink(current) + followed, err := followUnderRoot(root, next, depth+1) if err != nil { return "", err } - if filepath.IsAbs(nextTarget) { - return "", fmt.Errorf("archive symlink %s has absolute target: %s", current, nextTarget) - } - resolved, err := filepath.EvalSymlinks(current) - if err != nil { - if !os.IsNotExist(err) { - return "", err - } - resolved, err = walkLinkTarget(destDirClean, destDirResolved, parent, nextTarget, depth+1) - if err != nil { - return "", err - } - } else { - resolved = filepath.Clean(resolved) - } - if !pathInsideDest(destDirClean, destDirResolved, resolved) { - return "", fmt.Errorf("archive symlink %s escapes destination: %s", current, resolved) - } - current = resolved + current = followed } return current, nil } diff --git a/internal/update/extract_test.go b/internal/update/extract_test.go index 3f82abb99..0598c0084 100644 --- a/internal/update/extract_test.go +++ b/internal/update/extract_test.go @@ -479,3 +479,56 @@ func TestExtractTarGzAllowsSafeDanglingRelative(t *testing.T) { t.Fatal("safe dangling target was written outside destDir") } } + +// Tar entry d -> . followed by d/s -> .. creates destDir/s -> .. if the +// symlink entry's link-target check uses lexical filepath.Dir instead of the +// physical parent that will create it. The symlink entry itself must be +// rejected before an outbound link can be planted. +func TestExtractTarGzRejectsSymlinkParentEscape(t *testing.T) { + if !symlinksSupported(t) { + t.Skip("symlinks not supported") + } + dir := t.TempDir() + destDir := filepath.Join(dir, "extracted") + archivePath := filepath.Join(dir, "archive.tar.gz") + writeTestTarGzEntries(t, archivePath, []testTarEntry{ + {name: "d", typeflag: tar.TypeSymlink, linkname: "."}, + {name: "d/s", typeflag: tar.TypeSymlink, linkname: ".."}, + }) + extractErr := extractArchive(archivePath, destDir) + escaped := filepath.Join(destDir, "s") + if _, err := os.Lstat(escaped); err == nil { + t.Fatalf("outbound symlink %s was created", escaped) + } + if extractErr == nil { + t.Fatal("expected extractArchive to reject symlink entry with escaping target through symlink parent") + } +} + +// An archive attempting to plant zero -> d/s/outside-file through a symlink parent +// must not create the target outside destDir and extractArchive must fail. +func TestExtractTarGzRejectsSymlinkThroughSymlinkParentOutsideTarget(t *testing.T) { + if !symlinksSupported(t) { + t.Skip("symlinks not supported") + } + dir := t.TempDir() + destDir := filepath.Join(dir, "extracted") + outsideDir := filepath.Join(dir, "outside") + if err := os.MkdirAll(outsideDir, 0o755); err != nil { + t.Fatalf("MkdirAll outside: %v", err) + } + outsideFile := filepath.Join(outsideDir, "pwned.txt") + archivePath := filepath.Join(dir, "archive.tar.gz") + writeTestTarGzEntries(t, archivePath, []testTarEntry{ + {name: "d", typeflag: tar.TypeSymlink, linkname: "."}, + {name: "d/s", typeflag: tar.TypeSymlink, linkname: ".."}, + {name: "zero", typeflag: tar.TypeSymlink, linkname: "d/s/outside/pwned.txt"}, + }) + extractErr := extractArchive(archivePath, destDir) + if extractErr == nil { + t.Fatal("expected extractArchive to fail on escaping symlink chain") + } + if _, err := os.Stat(outsideFile); !os.IsNotExist(err) { + t.Fatalf("file outside destDir was accessed/created: %v", err) + } +} From efa640ea77ef958cac5a2b9364361e2a3346a35c Mon Sep 17 00:00:00 2001 From: hazyhaar Date: Sun, 30 Aug 2026 21:16:31 +0200 Subject: [PATCH 9/9] fix(update): resolve intermediate symlink parents and cap Root link depth Walk each path component from the physical parent, reject a ninth link, and look up release binaries through OpenRoot as regular files only. --- internal/update/extract.go | 55 +++++++++++------------- internal/update/extract_test.go | 76 +++++++++++++++++++++++++++++---- 2 files changed, 93 insertions(+), 38 deletions(-) diff --git a/internal/update/extract.go b/internal/update/extract.go index f3b8dc5ed..8a5d1431f 100644 --- a/internal/update/extract.go +++ b/internal/update/extract.go @@ -216,39 +216,17 @@ func validateSymlinkTarget(root *os.Root, parentRel, linkTarget string) error { return err } +const maxRootSymlinks = 8 + func followUnderRoot(root *os.Root, rel string, depth int) (string, error) { - if depth > 255 { - return "", fmt.Errorf("archive symlink nest exceeds limit") - } if rel == "" || rel == "." { return ".", nil } - info, err := root.Lstat(rel) - if err != nil { - if os.IsNotExist(err) { - return rel, nil - } - return "", err - } - if info.Mode()&os.ModeSymlink == 0 { - return rel, nil - } - tgt, err := root.Readlink(rel) - if err != nil { - return "", err - } - if filepath.IsAbs(tgt) || strings.HasPrefix(tgt, "/") { - return "", fmt.Errorf("archive symlink %s has absolute target: %s", rel, tgt) - } - parent := filepath.Dir(rel) - if parent == "" { - parent = "." - } - return walkUnderRoot(root, parent, tgt, depth+1) + return walkUnderRoot(root, ".", rel, depth) } func walkUnderRoot(root *os.Root, base, linkTarget string, depth int) (string, error) { - if depth > 255 { + if depth > maxRootSymlinks { return "", fmt.Errorf("archive symlink nest exceeds limit") } current := base @@ -285,7 +263,15 @@ func walkUnderRoot(root *os.Root, base, linkTarget string, depth int) (string, e current = next continue } - followed, err := followUnderRoot(root, next, depth+1) + tgt, err := root.Readlink(next) + if err != nil { + return "", err + } + if filepath.IsAbs(tgt) || strings.HasPrefix(tgt, "/") || strings.HasPrefix(tgt, "\\") || + filepath.VolumeName(tgt) != "" || strings.Contains(tgt, ":") { + return "", fmt.Errorf("archive symlink %s has absolute target: %s", next, tgt) + } + followed, err := walkUnderRoot(root, current, tgt, depth+1) if err != nil { return "", err } @@ -299,16 +285,25 @@ func walkUnderRoot(root *os.Root, base, linkTarget string, depth int) (string, e // helper binaries nested under archive subdirectories (e.g. helpers/) are // still found. func findByBasename(root string, name string) (string, error) { + destRoot, err := os.OpenRoot(root) + if err != nil { + return "", err + } + defer destRoot.Close() var found string - err := filepath.WalkDir(root, func(path string, entry fs.DirEntry, walkErr error) error { + err = fs.WalkDir(destRoot.FS(), ".", func(path string, entry fs.DirEntry, walkErr error) error { if walkErr != nil { return walkErr } if found != "" { return fs.SkipAll } - if !entry.IsDir() && entry.Name() == name { - found = path + if entry.Type().IsRegular() && entry.Name() == name { + if path == "." { + found = filepath.Join(root, name) + } else { + found = filepath.Join(root, filepath.FromSlash(path)) + } } return nil }) diff --git a/internal/update/extract_test.go b/internal/update/extract_test.go index 0598c0084..08bc07875 100644 --- a/internal/update/extract_test.go +++ b/internal/update/extract_test.go @@ -4,6 +4,7 @@ import ( "archive/tar" "archive/zip" "compress/gzip" + "fmt" "os" "path/filepath" "testing" @@ -258,21 +259,23 @@ func TestExtractTarGzAllowsSafeSymlink(t *testing.T) { } // Cover a linked subdirectory with a file extracted through the link. + // Windows Root.Symlink needs the directory target to exist first so the + // link is created as a directory junction rather than a file symlink. h3 := &tar.Header{ - Name: "sublink", - Typeflag: tar.TypeSymlink, - Linkname: "subdir", + Name: "subdir", + Typeflag: tar.TypeDir, + Mode: 0o755, } if err := tw.WriteHeader(h3); err != nil { - t.Fatalf("WriteHeader sublink: %v", err) + t.Fatalf("WriteHeader subdir: %v", err) } h4 := &tar.Header{ - Name: "subdir", - Typeflag: tar.TypeDir, - Mode: 0o755, + Name: "sublink", + Typeflag: tar.TypeSymlink, + Linkname: "subdir", } if err := tw.WriteHeader(h4); err != nil { - t.Fatalf("WriteHeader subdir: %v", err) + t.Fatalf("WriteHeader sublink: %v", err) } h5 := &tar.Header{ Name: "sublink/nested.txt", @@ -532,3 +535,60 @@ func TestExtractTarGzRejectsSymlinkThroughSymlinkParentOutsideTarget(t *testing. t.Fatalf("file outside destDir was accessed/created: %v", err) } } + +func TestExtractTarGzRejectsIntermediateDirSymlinkEscape(t *testing.T) { + if !symlinksSupported(t) { + t.Skip("symlinks not supported") + } + dir := t.TempDir() + destDir := filepath.Join(dir, "extracted") + outside := filepath.Join(dir, "archive.tar.gz.outside") + if err := os.WriteFile(outside, []byte("secret"), 0o644); err != nil { + t.Fatal(err) + } + archivePath := filepath.Join(dir, "archive.tar.gz") + writeTestTarGzEntries(t, archivePath, []testTarEntry{ + {name: "d", typeflag: tar.TypeSymlink, linkname: "."}, + {name: "d/a", typeflag: tar.TypeDir}, + {name: "d/a/zero", typeflag: tar.TypeSymlink, linkname: "../../archive.tar.gz.outside"}, + }) + if err := extractArchive(archivePath, destDir); err == nil { + t.Fatal("expected reject of ../../ escape through intermediate dir symlink") + } + if _, err := os.Lstat(filepath.Join(destDir, "d", "a", "zero")); err == nil { + t.Fatal("escaping link must not be created") + } +} + +func TestFollowUnderRootRejectsNinthLink(t *testing.T) { + if !symlinksSupported(t) { + t.Skip("symlinks not supported") + } + dir := t.TempDir() + root, err := os.OpenRoot(dir) + if err != nil { + t.Fatal(err) + } + defer root.Close() + if err := os.Mkdir(filepath.Join(dir, "leaf"), 0o755); err != nil { + t.Fatal(err) + } + prev := "leaf" + for i := 0; i < maxRootSymlinks; i++ { + name := fmt.Sprintf("l%d", i) + if err := os.Symlink(prev, filepath.Join(dir, name)); err != nil { + t.Fatal(err) + } + prev = name + } + if _, err := followUnderRoot(root, prev, 0); err != nil { + t.Fatalf("eight links must resolve: %v", err) + } + ninth := "l8" + if err := os.Symlink(prev, filepath.Join(dir, ninth)); err != nil { + t.Fatal(err) + } + if _, err := followUnderRoot(root, ninth, 0); err == nil { + t.Fatal("ninth link must be rejected") + } +}