From 644294c0d27d9038af1bff63e33f729e3286c5f8 Mon Sep 17 00:00:00 2001 From: rgarcia <72655+rgarcia@users.noreply.github.com> Date: Sat, 8 Aug 2026 02:06:18 +0000 Subject: [PATCH 1/6] Add symlink zip round-trip test --- server/lib/ziputil/ziputil_test.go | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/server/lib/ziputil/ziputil_test.go b/server/lib/ziputil/ziputil_test.go index 832ada59..4b6e726e 100644 --- a/server/lib/ziputil/ziputil_test.go +++ b/server/lib/ziputil/ziputil_test.go @@ -10,6 +10,32 @@ import ( "github.com/stretchr/testify/require" ) +func TestZipDirPreservesSymlinks(t *testing.T) { + sourceDir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(sourceDir, "target.txt"), []byte("target contents"), 0644)) + require.NoError(t, os.Symlink("target.txt", filepath.Join(sourceDir, "link.txt"))) + + zipContent, err := ZipDir(sourceDir) + require.NoError(t, err) + + zipFile, err := os.CreateTemp(t.TempDir(), "archive-*.zip") + require.NoError(t, err) + _, err = zipFile.Write(zipContent) + require.NoError(t, err) + require.NoError(t, zipFile.Close()) + + destDir := t.TempDir() + require.NoError(t, Unzip(zipFile.Name(), destDir)) + + linkPath := filepath.Join(destDir, "link.txt") + info, err := os.Lstat(linkPath) + require.NoError(t, err) + assert.True(t, info.Mode()&os.ModeSymlink != 0) + target, err := os.Readlink(linkPath) + require.NoError(t, err) + assert.Equal(t, "target.txt", target) +} + func TestUnzipFile(t *testing.T) { // Create a temporary directory for test files sourceDir, err := os.MkdirTemp("", "zip-source-*") From 65e8a88993008f9b4858c54ec8938b1f3db4f0eb Mon Sep 17 00:00:00 2001 From: rgarcia <72655+rgarcia@users.noreply.github.com> Date: Sat, 8 Aug 2026 02:06:18 +0000 Subject: [PATCH 2/6] Preserve symlinks in zip transfers --- server/lib/ziputil/ziputil.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/server/lib/ziputil/ziputil.go b/server/lib/ziputil/ziputil.go index 980097bb..a7678d6d 100644 --- a/server/lib/ziputil/ziputil.go +++ b/server/lib/ziputil/ziputil.go @@ -53,6 +53,17 @@ func ZipDir(sourceDir string) ([]byte, error) { return nil } + if info.Mode()&os.ModeSymlink != 0 { + target, err := os.Readlink(path) + if err != nil { + return fmt.Errorf("read symlink %s: %w", path, err) + } + if _, err := writer.Write([]byte(target)); err != nil { + return fmt.Errorf("write symlink %s: %w", path, err) + } + return nil + } + // Only include regular files. Skip sockets, devices, FIFOs, etc. if !info.Mode().IsRegular() { return nil @@ -123,6 +134,17 @@ func Unzip(zipFilePath, destDir string) error { } defer fileReader.Close() + if file.Mode()&os.ModeSymlink != 0 { + target, err := io.ReadAll(fileReader) + if err != nil { + return fmt.Errorf("failed to read symlink target: %w", err) + } + if err := os.Symlink(string(target), destPath); err != nil { + return fmt.Errorf("failed to create symlink: %w", err) + } + continue + } + // Create the destination file destFile, err := os.OpenFile(destPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, file.Mode()) if err != nil { From ede3c1834c4e6b5c6e90a385d7b318b6b1eeaa3e Mon Sep 17 00:00:00 2001 From: rgarcia <72655+rgarcia@users.noreply.github.com> Date: Sat, 8 Aug 2026 02:19:58 +0000 Subject: [PATCH 3/6] Validate symlinks during ZIP extraction --- server/lib/ziputil/ziputil.go | 16 +++++++++-- server/lib/ziputil/ziputil_test.go | 44 ++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/server/lib/ziputil/ziputil.go b/server/lib/ziputil/ziputil.go index a7678d6d..fd7d2f93 100644 --- a/server/lib/ziputil/ziputil.go +++ b/server/lib/ziputil/ziputil.go @@ -104,13 +104,15 @@ func Unzip(zipFilePath, destDir string) error { if err := os.MkdirAll(destDir, 0755); err != nil { return fmt.Errorf("failed to create destination directory: %w", err) } + cleanDestDir := filepath.Clean(destDir) + // Extract each file for _, file := range reader.File { // Create the full destination path destPath := filepath.Join(destDir, file.Name) // Check for directory traversal vulnerabilities - if !strings.HasPrefix(destPath, filepath.Clean(destDir)+string(os.PathSeparator)) { + if !strings.HasPrefix(destPath, cleanDestDir+string(os.PathSeparator)) { return fmt.Errorf("illegal file path: %s", file.Name) } @@ -139,7 +141,17 @@ func Unzip(zipFilePath, destDir string) error { if err != nil { return fmt.Errorf("failed to read symlink target: %w", err) } - if err := os.Symlink(string(target), destPath); err != nil { + targetPath := string(target) + if !filepath.IsAbs(targetPath) { + resolvedTarget := filepath.Clean(filepath.Join(filepath.Dir(destPath), targetPath)) + if resolvedTarget != cleanDestDir && !strings.HasPrefix(resolvedTarget, cleanDestDir+string(os.PathSeparator)) { + return fmt.Errorf("illegal symlink target: %s -> %s", file.Name, targetPath) + } + } + if err := os.Remove(destPath); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("failed to remove existing symlink path: %w", err) + } + if err := os.Symlink(targetPath, destPath); err != nil { return fmt.Errorf("failed to create symlink: %w", err) } continue diff --git a/server/lib/ziputil/ziputil_test.go b/server/lib/ziputil/ziputil_test.go index 4b6e726e..c970e1a5 100644 --- a/server/lib/ziputil/ziputil_test.go +++ b/server/lib/ziputil/ziputil_test.go @@ -36,6 +36,50 @@ func TestZipDirPreservesSymlinks(t *testing.T) { assert.Equal(t, "target.txt", target) } +func TestUnzipRejectsEscapingSymlink(t *testing.T) { + zipPath := createSymlinkZip(t, "../outside.txt") + + err := Unzip(zipPath, t.TempDir()) + require.Error(t, err) + assert.Contains(t, err.Error(), "illegal symlink target") +} + +func TestUnzipOverwritesFileWithSymlink(t *testing.T) { + zipPath := createSymlinkZip(t, "target.txt") + destDir := t.TempDir() + linkPath := filepath.Join(destDir, "link.txt") + require.NoError(t, os.WriteFile(linkPath, []byte("old contents"), 0644)) + + require.NoError(t, Unzip(zipPath, destDir)) + + info, err := os.Lstat(linkPath) + require.NoError(t, err) + assert.True(t, info.Mode()&os.ModeSymlink != 0) + target, err := os.Readlink(linkPath) + require.NoError(t, err) + assert.Equal(t, "target.txt", target) +} + +func createSymlinkZip(t *testing.T, target string) string { + t.Helper() + + zipPath := filepath.Join(t.TempDir(), "symlink.zip") + zipFile, err := os.Create(zipPath) + require.NoError(t, err) + + zipWriter := zip.NewWriter(zipFile) + header := &zip.FileHeader{Name: "link.txt", Method: zip.Store} + header.SetMode(os.ModeSymlink | 0777) + writer, err := zipWriter.CreateHeader(header) + require.NoError(t, err) + _, err = writer.Write([]byte(target)) + require.NoError(t, err) + require.NoError(t, zipWriter.Close()) + require.NoError(t, zipFile.Close()) + + return zipPath +} + func TestUnzipFile(t *testing.T) { // Create a temporary directory for test files sourceDir, err := os.MkdirTemp("", "zip-source-*") From 32415d825db4b4d8115f0e738be1688290b8505f Mon Sep 17 00:00:00 2001 From: rgarcia <72655+rgarcia@users.noreply.github.com> Date: Sat, 8 Aug 2026 02:36:33 +0000 Subject: [PATCH 4/6] Resolve symlinks in ZIP path checks --- server/lib/ziputil/ziputil.go | 65 +++++++++++++++++++++++++++--- server/lib/ziputil/ziputil_test.go | 36 +++++++++++++++++ 2 files changed, 96 insertions(+), 5 deletions(-) diff --git a/server/lib/ziputil/ziputil.go b/server/lib/ziputil/ziputil.go index fd7d2f93..f4c61f54 100644 --- a/server/lib/ziputil/ziputil.go +++ b/server/lib/ziputil/ziputil.go @@ -104,15 +104,31 @@ func Unzip(zipFilePath, destDir string) error { if err := os.MkdirAll(destDir, 0755); err != nil { return fmt.Errorf("failed to create destination directory: %w", err) } - cleanDestDir := filepath.Clean(destDir) + cleanDestDir, err := filepath.Abs(destDir) + if err != nil { + return fmt.Errorf("failed to resolve destination directory: %w", err) + } + cleanDestDir, err = filepath.EvalSymlinks(cleanDestDir) + if err != nil { + return fmt.Errorf("failed to evaluate destination directory: %w", err) + } // Extract each file for _, file := range reader.File { + entryPath := filepath.FromSlash(file.Name) + // Create the full destination path - destPath := filepath.Join(destDir, file.Name) + destPath := filepath.Join(cleanDestDir, entryPath) // Check for directory traversal vulnerabilities - if !strings.HasPrefix(destPath, cleanDestDir+string(os.PathSeparator)) { + if !isPathWithinDir(cleanDestDir, destPath) { + return fmt.Errorf("illegal file path: %s", file.Name) + } + resolvedParentPath, err := resolvePathWithSymlinks(cleanDestDir, filepath.Dir(entryPath)) + if err != nil { + return fmt.Errorf("failed to resolve destination path %s: %w", file.Name, err) + } + if !isPathWithinDir(cleanDestDir, resolvedParentPath) { return fmt.Errorf("illegal file path: %s", file.Name) } @@ -143,8 +159,11 @@ func Unzip(zipFilePath, destDir string) error { } targetPath := string(target) if !filepath.IsAbs(targetPath) { - resolvedTarget := filepath.Clean(filepath.Join(filepath.Dir(destPath), targetPath)) - if resolvedTarget != cleanDestDir && !strings.HasPrefix(resolvedTarget, cleanDestDir+string(os.PathSeparator)) { + resolvedTarget, err := resolvePathWithSymlinks(resolvedParentPath, targetPath) + if err != nil { + return fmt.Errorf("failed to resolve symlink target: %w", err) + } + if !isPathWithinDir(cleanDestDir, resolvedTarget) { return fmt.Errorf("illegal symlink target: %s -> %s", file.Name, targetPath) } } @@ -157,6 +176,12 @@ func Unzip(zipFilePath, destDir string) error { continue } + if info, err := os.Lstat(destPath); err == nil && info.Mode()&os.ModeSymlink != 0 { + if err := os.Remove(destPath); err != nil { + return fmt.Errorf("failed to remove existing symlink: %w", err) + } + } + // Create the destination file destFile, err := os.OpenFile(destPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, file.Mode()) if err != nil { @@ -172,3 +197,33 @@ func Unzip(zipFilePath, destDir string) error { return nil } + +func isPathWithinDir(dir, path string) bool { + return path == dir || strings.HasPrefix(path, dir+string(os.PathSeparator)) +} + +func resolvePathWithSymlinks(baseDir, relativePath string) (string, error) { + currentPath := filepath.Clean(baseDir) + for _, part := range strings.Split(filepath.FromSlash(relativePath), string(os.PathSeparator)) { + switch part { + case "", ".": + continue + case "..": + currentPath = filepath.Dir(currentPath) + continue + } + + nextPath := filepath.Join(currentPath, part) + resolvedPath, err := filepath.EvalSymlinks(nextPath) + if err == nil { + currentPath = resolvedPath + continue + } + if !os.IsNotExist(err) { + return "", fmt.Errorf("evaluate symlinks for %s: %w", nextPath, err) + } + currentPath = nextPath + } + + return filepath.Clean(currentPath), nil +} diff --git a/server/lib/ziputil/ziputil_test.go b/server/lib/ziputil/ziputil_test.go index c970e1a5..e3a5a83f 100644 --- a/server/lib/ziputil/ziputil_test.go +++ b/server/lib/ziputil/ziputil_test.go @@ -44,6 +44,15 @@ func TestUnzipRejectsEscapingSymlink(t *testing.T) { assert.Contains(t, err.Error(), "illegal symlink target") } +func TestUnzipRejectsSymlinkChainEscape(t *testing.T) { + zipPath := createSymlinkChainEscapeZip(t) + destDir := filepath.Join(t.TempDir(), "extract") + + err := Unzip(zipPath, destDir) + require.Error(t, err) + assert.Contains(t, err.Error(), "illegal symlink target") +} + func TestUnzipOverwritesFileWithSymlink(t *testing.T) { zipPath := createSymlinkZip(t, "target.txt") destDir := t.TempDir() @@ -80,6 +89,33 @@ func createSymlinkZip(t *testing.T, target string) string { return zipPath } +func createSymlinkChainEscapeZip(t *testing.T) string { + t.Helper() + + zipPath := filepath.Join(t.TempDir(), "chain-escape.zip") + zipFile, err := os.Create(zipPath) + require.NoError(t, err) + + zipWriter := zip.NewWriter(zipFile) + linkHeader := &zip.FileHeader{Name: "link", Method: zip.Store} + linkHeader.SetMode(os.ModeSymlink | 0777) + linkWriter, err := zipWriter.CreateHeader(linkHeader) + require.NoError(t, err) + _, err = linkWriter.Write([]byte(".")) + require.NoError(t, err) + + escapeHeader := &zip.FileHeader{Name: "escape", Method: zip.Store} + escapeHeader.SetMode(os.ModeSymlink | 0777) + escapeWriter, err := zipWriter.CreateHeader(escapeHeader) + require.NoError(t, err) + _, err = escapeWriter.Write([]byte("link/..")) + require.NoError(t, err) + + require.NoError(t, zipWriter.Close()) + require.NoError(t, zipFile.Close()) + return zipPath +} + func TestUnzipFile(t *testing.T) { // Create a temporary directory for test files sourceDir, err := os.MkdirTemp("", "zip-source-*") From 9ff9d02301d1070c9e985a6cd2801787d79e3e67 Mon Sep 17 00:00:00 2001 From: rgarcia <72655+rgarcia@users.noreply.github.com> Date: Sat, 8 Aug 2026 17:00:45 +0000 Subject: [PATCH 5/6] Tighten ZIP symlink containment --- server/lib/ziputil/ziputil.go | 19 +++++------ server/lib/ziputil/ziputil_test.go | 52 +++++++++++++++++++++++++++++- 2 files changed, 61 insertions(+), 10 deletions(-) diff --git a/server/lib/ziputil/ziputil.go b/server/lib/ziputil/ziputil.go index f4c61f54..3b17138e 100644 --- a/server/lib/ziputil/ziputil.go +++ b/server/lib/ziputil/ziputil.go @@ -121,7 +121,7 @@ func Unzip(zipFilePath, destDir string) error { destPath := filepath.Join(cleanDestDir, entryPath) // Check for directory traversal vulnerabilities - if !isPathWithinDir(cleanDestDir, destPath) { + if destPath == cleanDestDir || !isPathWithinDir(cleanDestDir, destPath) { return fmt.Errorf("illegal file path: %s", file.Name) } resolvedParentPath, err := resolvePathWithSymlinks(cleanDestDir, filepath.Dir(entryPath)) @@ -158,14 +158,15 @@ func Unzip(zipFilePath, destDir string) error { return fmt.Errorf("failed to read symlink target: %w", err) } targetPath := string(target) - if !filepath.IsAbs(targetPath) { - resolvedTarget, err := resolvePathWithSymlinks(resolvedParentPath, targetPath) - if err != nil { - return fmt.Errorf("failed to resolve symlink target: %w", err) - } - if !isPathWithinDir(cleanDestDir, resolvedTarget) { - return fmt.Errorf("illegal symlink target: %s -> %s", file.Name, targetPath) - } + if filepath.IsAbs(targetPath) { + return fmt.Errorf("illegal symlink target: %s -> %s", file.Name, targetPath) + } + resolvedTarget, err := resolvePathWithSymlinks(resolvedParentPath, targetPath) + if err != nil { + return fmt.Errorf("failed to resolve symlink target: %w", err) + } + if !isPathWithinDir(cleanDestDir, resolvedTarget) { + return fmt.Errorf("illegal symlink target: %s -> %s", file.Name, targetPath) } if err := os.Remove(destPath); err != nil && !os.IsNotExist(err) { return fmt.Errorf("failed to remove existing symlink path: %w", err) diff --git a/server/lib/ziputil/ziputil_test.go b/server/lib/ziputil/ziputil_test.go index e3a5a83f..8e5174c8 100644 --- a/server/lib/ziputil/ziputil_test.go +++ b/server/lib/ziputil/ziputil_test.go @@ -53,6 +53,35 @@ func TestUnzipRejectsSymlinkChainEscape(t *testing.T) { assert.Contains(t, err.Error(), "illegal symlink target") } +func TestUnzipRejectsAbsoluteSymlink(t *testing.T) { + zipPath := createSymlinkZip(t, filepath.Join(t.TempDir(), "target.txt")) + + err := Unzip(zipPath, t.TempDir()) + require.Error(t, err) + assert.Contains(t, err.Error(), "illegal symlink target") +} + +func TestUnzipRejectsRootEntry(t *testing.T) { + zipPath := createNamedSymlinkZip(t, ".", "target.txt") + destDir := t.TempDir() + + err := Unzip(zipPath, destDir) + require.Error(t, err) + assert.Contains(t, err.Error(), "illegal file path") +} + +func TestUnzipRejectsEntryUnderExistingSymlink(t *testing.T) { + zipPath := createFileZip(t, "link/file.txt") + destDir := t.TempDir() + outsideDir := t.TempDir() + require.NoError(t, os.Symlink(outsideDir, filepath.Join(destDir, "link"))) + + err := Unzip(zipPath, destDir) + require.Error(t, err) + assert.Contains(t, err.Error(), "illegal file path") + require.NoFileExists(t, filepath.Join(outsideDir, "file.txt")) +} + func TestUnzipOverwritesFileWithSymlink(t *testing.T) { zipPath := createSymlinkZip(t, "target.txt") destDir := t.TempDir() @@ -71,13 +100,18 @@ func TestUnzipOverwritesFileWithSymlink(t *testing.T) { func createSymlinkZip(t *testing.T, target string) string { t.Helper() + return createNamedSymlinkZip(t, "link.txt", target) +} + +func createNamedSymlinkZip(t *testing.T, name, target string) string { + t.Helper() zipPath := filepath.Join(t.TempDir(), "symlink.zip") zipFile, err := os.Create(zipPath) require.NoError(t, err) zipWriter := zip.NewWriter(zipFile) - header := &zip.FileHeader{Name: "link.txt", Method: zip.Store} + header := &zip.FileHeader{Name: name, Method: zip.Store} header.SetMode(os.ModeSymlink | 0777) writer, err := zipWriter.CreateHeader(header) require.NoError(t, err) @@ -89,6 +123,22 @@ func createSymlinkZip(t *testing.T, target string) string { return zipPath } +func createFileZip(t *testing.T, name string) string { + t.Helper() + + zipPath := filepath.Join(t.TempDir(), "file.zip") + zipFile, err := os.Create(zipPath) + require.NoError(t, err) + zipWriter := zip.NewWriter(zipFile) + writer, err := zipWriter.Create(name) + require.NoError(t, err) + _, err = writer.Write([]byte("contents")) + require.NoError(t, err) + require.NoError(t, zipWriter.Close()) + require.NoError(t, zipFile.Close()) + return zipPath +} + func createSymlinkChainEscapeZip(t *testing.T) string { t.Helper() From 41114aff40ba902014a4e27b56da98cb5a37acd4 Mon Sep 17 00:00:00 2001 From: rgarcia <72655+rgarcia@users.noreply.github.com> Date: Sat, 8 Aug 2026 17:09:00 +0000 Subject: [PATCH 6/6] Preserve absolute symlinks in ZIP transfers --- server/lib/ziputil/ziputil.go | 17 ++++++++--------- server/lib/ziputil/ziputil_test.go | 13 ++++++++----- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/server/lib/ziputil/ziputil.go b/server/lib/ziputil/ziputil.go index 3b17138e..3a857f09 100644 --- a/server/lib/ziputil/ziputil.go +++ b/server/lib/ziputil/ziputil.go @@ -158,15 +158,14 @@ func Unzip(zipFilePath, destDir string) error { return fmt.Errorf("failed to read symlink target: %w", err) } targetPath := string(target) - if filepath.IsAbs(targetPath) { - return fmt.Errorf("illegal symlink target: %s -> %s", file.Name, targetPath) - } - resolvedTarget, err := resolvePathWithSymlinks(resolvedParentPath, targetPath) - if err != nil { - return fmt.Errorf("failed to resolve symlink target: %w", err) - } - if !isPathWithinDir(cleanDestDir, resolvedTarget) { - return fmt.Errorf("illegal symlink target: %s -> %s", file.Name, targetPath) + if !filepath.IsAbs(targetPath) { + resolvedTarget, err := resolvePathWithSymlinks(resolvedParentPath, targetPath) + if err != nil { + return fmt.Errorf("failed to resolve symlink target: %w", err) + } + if !isPathWithinDir(cleanDestDir, resolvedTarget) { + return fmt.Errorf("illegal symlink target: %s -> %s", file.Name, targetPath) + } } if err := os.Remove(destPath); err != nil && !os.IsNotExist(err) { return fmt.Errorf("failed to remove existing symlink path: %w", err) diff --git a/server/lib/ziputil/ziputil_test.go b/server/lib/ziputil/ziputil_test.go index 8e5174c8..0f357f62 100644 --- a/server/lib/ziputil/ziputil_test.go +++ b/server/lib/ziputil/ziputil_test.go @@ -53,12 +53,15 @@ func TestUnzipRejectsSymlinkChainEscape(t *testing.T) { assert.Contains(t, err.Error(), "illegal symlink target") } -func TestUnzipRejectsAbsoluteSymlink(t *testing.T) { - zipPath := createSymlinkZip(t, filepath.Join(t.TempDir(), "target.txt")) +func TestUnzipPreservesAbsoluteSymlink(t *testing.T) { + target := filepath.Join(t.TempDir(), "target.txt") + zipPath := createSymlinkZip(t, target) + destDir := t.TempDir() - err := Unzip(zipPath, t.TempDir()) - require.Error(t, err) - assert.Contains(t, err.Error(), "illegal symlink target") + require.NoError(t, Unzip(zipPath, destDir)) + actualTarget, err := os.Readlink(filepath.Join(destDir, "link.txt")) + require.NoError(t, err) + assert.Equal(t, target, actualTarget) } func TestUnzipRejectsRootEntry(t *testing.T) {