Harden zip symlink extraction and overwrite handling

Author: cursoragent

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(filepath.Clean(destPath), cleanDestDir+string(os.PathSeparator)) {
 			return fmt.Errorf("illegal file path: %s", file.Name)
 		}
 
@@ -139,7 +141,22 @@ 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)
+
+			// Relative symlink targets must not escape destDir.
+			// Absolute symlinks are allowed to preserve archive behavior.
+			if !filepath.IsAbs(targetPath) {
+				symlinkDir := filepath.Dir(destPath)
+				resolvedTarget := filepath.Clean(filepath.Join(symlinkDir, targetPath))
+				if resolvedTarget != cleanDestDir && !strings.HasPrefix(resolvedTarget, cleanDestDir+string(os.PathSeparator)) {
+					return fmt.Errorf("illegal symlink target (escapes destination): %s -> %s", file.Name, targetPath)
+				}
+			}
+
+			if err := os.Remove(destPath); err != nil && !os.IsNotExist(err) {
+				return fmt.Errorf("failed to remove existing path for symlink: %w", err)
+			}
+			if err := os.Symlink(targetPath, destPath); err != nil {
 				return fmt.Errorf("failed to create symlink: %w", err)
 			}
 			continue

server/lib/ziputil/ziputil_test.go

@@ -36,6 +36,67 @@ func TestZipDirPreservesSymlinks(t *testing.T) {
 	assert.Equal(t, "target.txt", target)
 }
 
+func TestUnzipRejectsEscapingRelativeSymlink(t *testing.T) {
+	zipPath := filepath.Join(t.TempDir(), "escape-symlink.zip")
+	zipFile, err := os.Create(zipPath)
+	require.NoError(t, err)
+
+	zipWriter := zip.NewWriter(zipFile)
+	symlinkHeader := &zip.FileHeader{
+		Name:   "link.txt",
+		Method: zip.Store,
+	}
+	symlinkHeader.SetMode(os.ModeSymlink | 0777)
+	linkWriter, err := zipWriter.CreateHeader(symlinkHeader)
+	require.NoError(t, err)
+	_, err = linkWriter.Write([]byte(".."))
+	require.NoError(t, err)
+	require.NoError(t, zipWriter.Close())
+	require.NoError(t, zipFile.Close())
+
+	err = Unzip(zipPath, t.TempDir())
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "illegal symlink target")
+}
+
+func TestUnzipSymlinkOverwritesExistingPath(t *testing.T) {
+	zipPath := filepath.Join(t.TempDir(), "overwrite-symlink.zip")
+	zipFile, err := os.Create(zipPath)
+	require.NoError(t, err)
+
+	zipWriter := zip.NewWriter(zipFile)
+	targetWriter, err := zipWriter.Create("target.txt")
+	require.NoError(t, err)
+	_, err = targetWriter.Write([]byte("target contents"))
+	require.NoError(t, err)
+
+	symlinkHeader := &zip.FileHeader{
+		Name:   "link.txt",
+		Method: zip.Store,
+	}
+	symlinkHeader.SetMode(os.ModeSymlink | 0777)
+	linkWriter, err := zipWriter.CreateHeader(symlinkHeader)
+	require.NoError(t, err)
+	_, err = linkWriter.Write([]byte("target.txt"))
+	require.NoError(t, err)
+
+	require.NoError(t, zipWriter.Close())
+	require.NoError(t, zipFile.Close())
+
+	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 TestUnzipFile(t *testing.T) {
 	// Create a temporary directory for test files
 	sourceDir, err := os.MkdirTemp("", "zip-source-*")