diff --git a/7z.go b/7z.go index afb2536..59a6f81 100644 --- a/7z.go +++ b/7z.go @@ -3,7 +3,6 @@ package xtractr import ( "fmt" "path/filepath" - "sync" "github.com/bodgit/sevenzip" ) @@ -23,14 +22,12 @@ func Extract7z(xFile *XFile) (size uint64, filesList, archiveList []string, err } for idx, password := range passwords { - size, files, archives, err := extract7z(&XFile{ - FilePath: xFile.FilePath, - OutputDir: xFile.OutputDir, - FileMode: xFile.FileMode, - DirMode: xFile.DirMode, - Password: password, - FileWorkers: xFile.FileWorkers, - }) + // Copy the input so the retry keeps the logger, progress callbacks, + // SquashRoot and the rest of the caller-provided configuration. + attempt := *xFile + attempt.Password = password + + size, files, archives, err := extract7z(&attempt) if err != nil && idx == len(passwords)-1 { return size, files, archives, fmt.Errorf("used password %d of %d: %w", idx+1, len(passwords), err) } else if err == nil { @@ -107,7 +104,7 @@ func (x *XFile) extract7zParallel(sevenZip *sevenzip.ReadCloser) (uint64, []stri return x.prog.Wrote, files, normalizeVolumes(sevenZip.Volumes(), x.FilePath), err } - workerErr := x.sevenZipDispatchWorkers(entries) + workerErr := dispatchWorkers(x.FileWorkers, entries, x.extract7zEntry) if workerErr != nil { return x.prog.Wrote, files, normalizeVolumes(sevenZip.Volumes(), x.FilePath), workerErr } @@ -148,39 +145,6 @@ func (x *XFile) sevenZipPrepareEntries(sevenZip *sevenzip.ReadCloser) ([]sevenZi return entries, files, nil } -// sevenZipDispatchWorkers sends file entries to a bounded worker pool for extraction. -func (x *XFile) sevenZipDispatchWorkers(entries []sevenZipEntry) error { - var ( - waitGroup sync.WaitGroup - firstErr error - errOnce sync.Once - semaphore = make(chan struct{}, x.FileWorkers) - ) - - for idx := range entries { - entry := entries[idx] - - if firstErr != nil { - break - } - - semaphore <- struct{}{} // acquire worker slot - - waitGroup.Go(func() { - defer func() { <-semaphore }() // release worker slot - - err := x.extract7zEntry(entry) - if err != nil { - errOnce.Do(func() { firstErr = err }) - } - }) - } - - waitGroup.Wait() - - return firstErr -} - // extract7zEntry extracts a single 7z file entry (used by parallel workers). func (x *XFile) extract7zEntry(entry sevenZipEntry) error { zFile, err := entry.sevenZipFile.Open() diff --git a/cpio.go b/cpio.go index cdb7dd1..eb66643 100644 --- a/cpio.go +++ b/cpio.go @@ -94,7 +94,13 @@ func (x *XFile) uncpioFile(cpioFile *cpio.Header, cpioReader *cpio.Reader) (uint // This turns hard links into symlinks. if cpioFile.Linkname != "" { - err := x.createSymlink(file.Path, cpioFile.Linkname) + // The link's parent folder may not have its own entry in the archive. + err := x.mkDir(filepath.Dir(file.Path), x.DirMode, cpioFile.ModTime) + if err != nil { + return 0, fmt.Errorf("making cpio link parent dir: %w", err) + } + + err = x.createSymlink(file.Path, cpioFile.Linkname) if err != nil { return 0, fmt.Errorf("%s: %w", cpioFile.FileInfo().Name(), err) } diff --git a/cue.go b/cue.go index 684d17c..6effcaf 100644 --- a/cue.go +++ b/cue.go @@ -106,9 +106,15 @@ func ExtractCUE(xFile *XFile) (size uint64, files, archives []string, err error) // Write the CUE sheet into the output directory so the folder is self-contained // (tracks, art, and the exact split definition for archival and re-rip verification). + // filepath.Base strips any directory components, and the join is verified to + // stay inside the output folder. cueBase := filepath.Base(xFile.FilePath) cueDest := filepath.Join(xFile.OutputDir, cueBase) + if !xFile.pathWithinOutput(cueDest) { + return 0, nil, nil, fmt.Errorf("%s: %w: %s", xFile.FilePath, ErrInvalidPath, cueDest) + } + writeErr := copyCueToOutput(xFile.FilePath, cueDest, xFile.FileMode) if writeErr != nil { xFile.Debugf("Copying CUE sheet to output: %s", writeErr) @@ -307,6 +313,12 @@ func parseCueSheet(reader io.Reader) (*CueSheet, []cueTimestamp, error) { //noli func resolveCueAudioPath(cueDir, cueFile, cueFilePath string) (string, error) { path := filepath.Join(cueDir, cueFile) + // A CUE sheet must not reference audio outside its own folder; a crafted + // FILE entry like "../../secret.flac" would read and copy that file. + if !pathWithin(cueDir, path) { + return "", fmt.Errorf("%w: %s", ErrInvalidPath, cueFile) + } + _, err := os.Stat(path) if err == nil { return path, nil @@ -995,7 +1007,9 @@ func copyCueToOutput(srcPath, destPath string, fileMode os.FileMode) error { return fmt.Errorf("reading cue sheet: %w", err) } - err = os.WriteFile(destPath, data, fileMode) //nolint:gosec // ffs. + // destPath is built from filepath.Base(srcPath) joined onto the output + // folder and verified to stay inside it at the call site; it cannot traverse. + err = os.WriteFile(destPath, data, fileMode) //nolint:gosec // G703: destPath is output-folder-contained (see above). if err != nil { return fmt.Errorf("writing cue sheet: %w", err) } diff --git a/cue_internal_test.go b/cue_internal_test.go new file mode 100644 index 0000000..e179417 --- /dev/null +++ b/cue_internal_test.go @@ -0,0 +1,47 @@ +package xtractr + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestResolveCueAudioPathTraversal ensures a CUE sheet cannot reference an +// audio file outside the folder the CUE sheet lives in. +func TestResolveCueAudioPathTraversal(t *testing.T) { + t.Parallel() + + cueDir := t.TempDir() + outside := filepath.Join(cueDir, "..", "outside.flac") + require.NoError(t, os.WriteFile(outside, []byte("not audio"), 0o600)) + + for _, cueFile := range []string{ + "../outside.flac", + "../../outside.flac", + "sub/../../outside.flac", + "..", + } { + _, err := resolveCueAudioPath(cueDir, cueFile, filepath.Join(cueDir, "disc.cue")) + require.Error(t, err, cueFile) + assert.ErrorIs(t, err, ErrInvalidPath, cueFile) + } +} + +// TestResolveCueAudioPathNested ensures audio in a subfolder of the CUE sheet +// still resolves (a legitimate layout some rips use). +func TestResolveCueAudioPathNested(t *testing.T) { + t.Parallel() + + cueDir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(cueDir, "disc1"), 0o750)) + + audioPath := filepath.Join(cueDir, "disc1", "album.flac") + require.NoError(t, os.WriteFile(audioPath, []byte("not audio"), 0o600)) + + resolved, err := resolveCueAudioPath(cueDir, "disc1/album.flac", filepath.Join(cueDir, "disc.cue")) + require.NoError(t, err) + assert.Equal(t, audioPath, resolved) +} diff --git a/files.go b/files.go index ecd86e5..997024b 100644 --- a/files.go +++ b/files.go @@ -13,6 +13,8 @@ import ( "slices" "strconv" "strings" + "sync" + "sync/atomic" "time" "unicode/utf8" ) @@ -94,6 +96,43 @@ func ChngInt(smallFn func(*XFile) (uint64, []string, error)) Interface { } } +// dispatchWorkers runs work for each entry using a bounded worker pool. +// Dispatch stops when a worker reports an error, in-flight entries finish, +// and the first error encountered is returned. Used by the random-access +// extractors (ZIP, 7z) when XFile.FileWorkers > 1. +func dispatchWorkers[T any](count int, entries []T, work func(T) error) error { + var ( + waitGroup sync.WaitGroup + firstErr atomic.Pointer[error] + semaphore = make(chan struct{}, count) + ) + + for idx := range entries { + if firstErr.Load() != nil { + break + } + + semaphore <- struct{}{} // acquire worker slot + + waitGroup.Go(func() { + defer func() { <-semaphore }() // release worker slot + + err := work(entries[idx]) + if err != nil { + firstErr.CompareAndSwap(nil, &err) + } + }) + } + + waitGroup.Wait() + + if err := firstErr.Load(); err != nil { + return *err + } + + return nil +} + // SupportedExtensions returns a slice of file extensions this library recognizes. func SupportedExtensions() []string { exts := make([]string, len(extension2function)) @@ -425,7 +464,12 @@ func ExtractFile(xFile *XFile) (size uint64, filesList, archiveList []string, er } // Fall back to file signature (magic number) detection. - xFile.Debugf("falling back to signature detection for %s (extension error: %v)", xFile.FilePath, err) + if err != nil { + xFile.Debugf("extension-based extraction failed for %s, falling back to signature detection: %v", + xFile.FilePath, err) + } else { + xFile.Debugf("no extension match for %s, falling back to signature detection", xFile.FilePath) + } extractFn, archiveType, sigErr := detectBySignature(xFile.FilePath) if sigErr != nil { @@ -639,12 +683,14 @@ func (x *Xtractr) Rename(oldpath, newpath string) error { return &ExtractError{Errs: []error{origErr, fmt.Errorf("os.Stat(): %w", err)}} } - oldFile, err := os.Open(oldpath) // do not forget to close this! + oldFile, err := os.Open(oldpath) if err != nil { return &ExtractError{Errs: []error{origErr, fmt.Errorf("os.Open(): %w", err)}} } - newFile, _, err := openFile(newpath, os.O_TRUNC|os.O_CREATE|os.O_WRONLY, oldFileStat.Mode()) + defer oldFile.Close() // also closed explicitly before the delete below. + + newFile, pathUsed, err := openFile(newpath, os.O_TRUNC|os.O_CREATE|os.O_WRONLY, oldFileStat.Mode()) if err != nil { return &ExtractError{Errs: []error{origErr, err}} } @@ -655,7 +701,8 @@ func (x *Xtractr) Rename(oldpath, newpath string) error { return &ExtractError{Errs: []error{origErr, fmt.Errorf("io.Copy(): %w", err)}} } - _ = os.Chtimes(newpath, oldFileStat.ModTime(), oldFileStat.ModTime()) + // pathUsed may differ from newpath if the name had to be truncated. + _ = os.Chtimes(pathUsed, oldFileStat.ModTime(), oldFileStat.ModTime()) // The copy was successful, so now delete the original file _ = oldFile.Close() // Needs to be closed before delete. _ = os.Remove(oldpath) @@ -794,9 +841,21 @@ func openStatFile(path string) (*os.File, os.FileInfo, error) { return file, stat, nil } +// mkDir creates a folder (and parents) with safe permissions. +// It refuses to leave the output folder through a pre-existing symlink. func (x *XFile) mkDir(path string, mode os.FileMode, mtime time.Time) error { defer os.Chtimes(path, time.Time{}, mtime) - return os.MkdirAll(path, x.safeDirMode(mode)) //nolint:wrapcheck + + err := os.MkdirAll(path, x.safeDirMode(mode)) + if err != nil { + return err //nolint:wrapcheck + } + + if !x.resolvedWithinOutput(path) { + return fmt.Errorf("%s: %w: %s resolves outside the output folder", x.FilePath, ErrInvalidPath, path) + } + + return nil } // write a file from an io reader, making sure all parent directories exist. @@ -827,6 +886,17 @@ func (x *XFile) writeFile(file *file, parallel bool) (uint64, error) { return 0, err } + // A symlink already sitting at the target path is followed by O_TRUNC, + // writing the archived data wherever the link points. Remove it so the + // archive member replaces it instead of writing through it. + info, statErr := os.Lstat(file.Path) + if statErr == nil && info.Mode()&os.ModeSymlink != 0 { + err := os.Remove(file.Path) + if err != nil { + return 0, fmt.Errorf("removing symlink at archived file path '%s': %w", file.Path, err) + } + } + fout, pathUsed, err := openFile(file.Path, os.O_RDWR|os.O_CREATE|os.O_TRUNC, x.safeFileMode(file.FileMode)) if err != nil { return 0, err @@ -903,14 +973,11 @@ func (x *XFile) clean(filePath string, trim ...string) string { return filepath.Clean(filepath.Join(x.OutputDir, filePath)) } -// pathWithinOutput reports whether path is OutputDir or a descendant of it. -// Uses filepath.Rel so sibling-prefix tricks like OutputDir=/tmp/out and -// path=/tmp/out_evil fail (unlike strings.HasPrefix). -func (x *XFile) pathWithinOutput(path string) bool { - outputDir := filepath.Clean(x.OutputDir) - cleanPath := filepath.Clean(path) - - rel, err := filepath.Rel(outputDir, cleanPath) +// pathWithin reports whether target is base or a descendant of it. +// Uses filepath.Rel so sibling-prefix tricks like base=/tmp/out and +// target=/tmp/out_evil fail (unlike strings.HasPrefix). +func pathWithin(base, target string) bool { + rel, err := filepath.Rel(filepath.Clean(base), filepath.Clean(target)) if err != nil { return false } @@ -918,6 +985,57 @@ func (x *XFile) pathWithinOutput(path string) bool { return rel == "." || (rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator))) } +// pathWithinOutput reports whether path is OutputDir or a descendant of it, +// comparing the cleaned paths lexically. +func (x *XFile) pathWithinOutput(path string) bool { + return pathWithin(x.OutputDir, path) +} + +// resolveExisting resolves symlinks in the deepest existing portion of path, +// then re-appends the not-yet-created tail. This normalizes a path for +// containment checks when some of its components may not exist yet, or when +// the path itself lives behind a symlink (e.g. /var -> /private/var on macOS). +// If symlink resolution fails, the cleaned path is returned unchanged. +func resolveExisting(path string) string { + probe := filepath.Clean(path) + tail := []string{} + + for { + _, err := os.Lstat(probe) + if err == nil { + break + } + + parent := filepath.Dir(probe) + if parent == probe { + return probe // reached the filesystem root; nothing exists to resolve. + } + + tail = append([]string{filepath.Base(probe)}, tail...) + probe = parent + } + + resolved, err := filepath.EvalSymlinks(probe) + if err != nil { + resolved = probe + } + + for _, segment := range tail { + resolved = filepath.Join(resolved, segment) + } + + return resolved +} + +// resolvedWithinOutput reports whether path stays inside OutputDir after +// resolving symlinks in the existing portions of both paths. The lexical +// check alone is not enough: a symlink already present in the output folder +// (planted by a previous download, another app, or an attacker) is followed +// by os.MkdirAll and os.OpenFile, writing files outside the output folder. +func (x *XFile) resolvedWithinOutput(path string) bool { + return pathWithin(resolveExisting(x.OutputDir), resolveExisting(path)) +} + // resolveLinkTarget returns the cleaned filesystem path a link would resolve to. func resolveLinkTarget(linkPath, linkName string) string { if filepath.IsAbs(linkName) { @@ -972,7 +1090,7 @@ func (x *XFile) createHardLink(path, linkName string) error { } target := x.clean(linkName) - if !x.pathWithinOutput(target) { + if !x.pathWithinOutput(target) || !x.resolvedWithinOutput(target) { return fmt.Errorf("%s: %w: %s (from: %s)", x.FilePath, ErrInvalidPath, target, linkName) } diff --git a/iso.go b/iso.go index 3acfbb9..9bf4297 100644 --- a/iso.go +++ b/iso.go @@ -98,7 +98,14 @@ func (x *XFile) uniso(isoFile *iso9660.File, parent string) (uint64, []string, e } if itemName != "" { - err := x.mkDir(filepath.Join(x.OutputDir, itemName), isoFile.Mode(), isoFile.ModTime()) + dirPath := x.clean(itemName) + if !x.pathWithinOutput(dirPath) { + // The directory is trying to land outside of our base path. Malicious ISO? + return 0, nil, fmt.Errorf("%s: %w: %s (from: %s)", + x.FilePath, ErrInvalidPath, dirPath, isoFile.Name()) + } + + err := x.mkDir(dirPath, isoFile.Mode(), isoFile.ModTime()) if err != nil { return 0, nil, fmt.Errorf("making iso directory %s: %w", isoFile.Name(), err) } diff --git a/magic.go b/magic.go index e7d1cdd..545a395 100644 --- a/magic.go +++ b/magic.go @@ -4,7 +4,9 @@ package xtractr import ( "bytes" + "errors" "fmt" + "io" "os" ) @@ -82,8 +84,10 @@ func detectBySignature(filePath string) (Interface, string, error) { buf := make([]byte, readSize) - n, err := file.Read(buf) - if err != nil { + // Read the full buffer: a single Read is not guaranteed to fill it, and a + // short read would miss signatures stored deeper in the file (e.g. ISO). + n, err := io.ReadFull(file, buf) + if err != nil && !errors.Is(err, io.ErrUnexpectedEOF) { return nil, "", fmt.Errorf("reading file for signature detection: %w", err) } diff --git a/parallel_test.go b/parallel_test.go index 8bb3fec..a638c60 100644 --- a/parallel_test.go +++ b/parallel_test.go @@ -191,6 +191,53 @@ func TestParallelZIPErrorPropagation(t *testing.T) { assert.ErrorIs(t, extractErr, xtractr.ErrInvalidPath) } +// TestParallelZIPWorkerErrorPropagation covers errors that surface in the +// worker phase (after dispatch), unlike TestParallelZIPErrorPropagation which +// fails during the sequential prepare pass. Run with -race to validate the +// shared first-error handling in dispatchWorkers. +func TestParallelZIPWorkerErrorPropagation(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + zipPath := filepath.Join(tmpDir, "worker_error.zip") + outFile, err := os.Create(zipPath) + require.NoError(t, err) + + zipWriter := zip.NewWriter(outFile) + + // A directory entry, then a file entry with the same name: the file write + // fails in a worker because "adir" already exists as a directory. + _, err = zipWriter.Create("adir/") + require.NoError(t, err) + + badWriter, err := zipWriter.Create("adir") + require.NoError(t, err) + + _, err = badWriter.Write([]byte("collides with the directory")) + require.NoError(t, err) + + // Extra files keep the dispatch loop running while the worker error lands. + for fileIdx := range parallelFileCount { + writer, createErr := zipWriter.Create(fmt.Sprintf("extra/file_%03d.txt", fileIdx)) + require.NoError(t, createErr) + + _, writeErr := writer.Write(generateTestContent(fileIdx, testContentSize)) + require.NoError(t, writeErr) + } + + require.NoError(t, zipWriter.Close()) + require.NoError(t, outFile.Close()) + + _, _, extractErr := xtractr.ExtractZIP(&xtractr.XFile{ + FilePath: zipPath, + OutputDir: filepath.Join(tmpDir, "out"), + FileMode: 0o600, + DirMode: 0o700, + FileWorkers: parallelWorkerCount, + }) + require.Error(t, extractErr) +} + func TestFileWorkersDefault(t *testing.T) { t.Parallel() diff --git a/rar.go b/rar.go index 82a4269..494c61f 100644 --- a/rar.go +++ b/rar.go @@ -26,13 +26,12 @@ func ExtractRAR(xFile *XFile) (size uint64, filesList, archiveList []string, err } for idx, password := range passwords { - size, files, archives, err := extractRAR(&XFile{ - FilePath: xFile.FilePath, - OutputDir: xFile.OutputDir, - FileMode: xFile.FileMode, - DirMode: xFile.DirMode, - Password: password, - }) + // Copy the input so the retry keeps the logger, progress callbacks, + // SquashRoot and the rest of the caller-provided configuration. + attempt := *xFile + attempt.Password = password + + size, files, archives, err := extractRAR(&attempt) if err == nil { return size, files, archives, nil } @@ -46,12 +45,10 @@ func ExtractRAR(xFile *XFile) (size uint64, filesList, archiveList []string, err } // No password worked, try without a password. - return extractRAR(&XFile{ - FilePath: xFile.FilePath, - OutputDir: xFile.OutputDir, - FileMode: xFile.FileMode, - DirMode: xFile.DirMode, - }) + attempt := *xFile + attempt.Password = "" + + return extractRAR(&attempt) } // extractRAR extracts a rar file. to a destination. This wraps github.com/nwaples/rardecode. diff --git a/rar_test.go b/rar_test.go index 392ef0a..475b0e0 100644 --- a/rar_test.go +++ b/rar_test.go @@ -4,6 +4,7 @@ import ( "fmt" "os" "path/filepath" + "sync/atomic" "testing" "github.com/stretchr/testify/assert" @@ -27,6 +28,48 @@ func TestExtractRAR(t *testing.T) { assert.Len(t, files, len(filesInTestArchive)) } +// TestExtractRARPasswordRetryKeepsConfig guards against the password-retry +// path dropping the caller-provided XFile configuration: the retries used to +// build a bare XFile, silently losing the logger (and SquashRoot, progress +// callbacks, etc.) for every attempt after the first. +func TestExtractRARPasswordRetryKeepsConfig(t *testing.T) { + t.Parallel() + + logger := &countingLogger{t: t} + xFile := &xtractr.XFile{ + FilePath: "./test_data/archive.rar", + OutputDir: t.TempDir(), + Passwords: []string{"wrong-password", "some_password"}, // correct password is second. + } + xFile.SetLogger(logger) + + size, files, archives, err := xtractr.ExtractRAR(xFile) + + require.NoError(t, err) + assert.Equal(t, testDataSize, size) + assert.Len(t, archives, 1) + assert.Len(t, files, len(filesInTestArchive)) + assert.Positive(t, logger.debugCount.Load(), + "the winning password retry must keep the caller's logger") +} + +// countingLogger counts log lines so tests can verify a logger is wired up. +type countingLogger struct { + t *testing.T + printCount atomic.Int64 + debugCount atomic.Int64 +} + +func (c *countingLogger) Printf(format string, v ...any) { + c.printCount.Add(1) + c.t.Logf(format, v...) +} + +func (c *countingLogger) Debugf(format string, v ...any) { + c.debugCount.Add(1) + c.t.Logf(format, v...) +} + // TestExtractRARMultiVolume guards against the regression where only the entry // file (instead of every volume) was returned in the archive list, which left // the sibling parts of a multi-part archive orphaned during cleanup. @@ -110,7 +153,9 @@ func TestExtractRARMultiVolumeOldScheme(t *testing.T) { } link := filepath.Join(dir, name) - if err := os.Symlink(payload, link); err != nil { + + err := os.Symlink(payload, link) + if err != nil { t.Skipf("symlinks unavailable on this platform: %v", err) } diff --git a/symlink_test.go b/symlink_test.go index 0333556..b987d40 100644 --- a/symlink_test.go +++ b/symlink_test.go @@ -196,6 +196,106 @@ func TestZipSymlinkTooLong(t *testing.T) { assert.ErrorIs(t, err, xtractr.ErrSymlinkTooLong) } +// TestPreExistingSymlinkDirEscape ensures a symlink already present in the +// output folder is not followed when an archive writes files beneath it. +// The lexical path (out/sub/file.txt) looks safe, but sub -> ../evil would +// land the payload outside the output folder. +func TestPreExistingSymlinkDirEscape(t *testing.T) { + t.Parallel() + + tmp := t.TempDir() + + err := os.Symlink("target", filepath.Join(tmp, "symlink-probe")) + if err != nil { + t.Skipf("symlinks unavailable on this platform: %v", err) + } + + payload := []byte("escape attempt") + + for _, archive := range []struct { + name string + path string + extract func(*xtractr.XFile) (uint64, []string, error) + }{ + {name: "zip", path: filepath.Join(tmp, "escape.zip"), extract: xtractr.ExtractZIP}, + {name: "tar", path: filepath.Join(tmp, "escape.tar"), extract: xtractr.ExtractTar}, + } { + switch filepath.Ext(archive.path) { + case ".zip": + require.NoError(t, writeZipWithTraversal(archive.path, "sub/file.txt", string(payload))) + case ".tar": + require.NoError(t, writeTarWithTraversal(archive.path, "sub/file.txt", string(payload))) + } + + outputDir := filepath.Join(tmp, archive.name+"-out") + evilDir := filepath.Join(tmp, archive.name+"-evil") + + require.NoError(t, os.MkdirAll(outputDir, 0o750)) + require.NoError(t, os.MkdirAll(evilDir, 0o750)) + // The pre-existing symlink an attacker left in the output folder. + require.NoError(t, os.Symlink(evilDir, filepath.Join(outputDir, "sub"))) + + _, _, err := archive.extract(&xtractr.XFile{ + FilePath: archive.path, + OutputDir: outputDir, + FileMode: 0o644, + DirMode: 0o755, + }) + require.Error(t, err, archive.name) + require.ErrorIs(t, err, xtractr.ErrInvalidPath, archive.name) + + _, statErr := os.Stat(filepath.Join(evilDir, "file.txt")) + assert.ErrorIs(t, statErr, os.ErrNotExist, archive.name+": payload must not land outside the output folder") + } +} + +// TestFinalComponentSymlinkNotFollowed ensures a pre-existing symlink at the +// exact path an archive member writes to is replaced, not followed through. +func TestFinalComponentSymlinkNotFollowed(t *testing.T) { + t.Parallel() + + tmp := t.TempDir() + + err := os.Symlink("target", filepath.Join(tmp, "symlink-probe")) + if err != nil { + t.Skipf("symlinks unavailable on this platform: %v", err) + } + + const payload = "archive payload" + + victim := filepath.Join(tmp, "victim.txt") + require.NoError(t, os.WriteFile(victim, []byte("original"), 0o600)) + + archivePath := filepath.Join(tmp, "replace.zip") + require.NoError(t, writeZipWithTraversal(archivePath, "file.txt", payload)) + + outputDir := filepath.Join(tmp, "out") + require.NoError(t, os.MkdirAll(outputDir, 0o750)) + require.NoError(t, os.Symlink(victim, filepath.Join(outputDir, "file.txt"))) + + _, _, err = xtractr.ExtractZIP(&xtractr.XFile{ + FilePath: archivePath, + OutputDir: outputDir, + FileMode: 0o644, + DirMode: 0o755, + }) + require.NoError(t, err) + + // The victim file outside the output folder must be untouched. + data, err := os.ReadFile(victim) + require.NoError(t, err) + assert.Equal(t, "original", string(data)) + + // And the archive member must have replaced the link with a regular file. + info, err := os.Lstat(filepath.Join(outputDir, "file.txt")) + require.NoError(t, err) + assert.Zero(t, info.Mode()&os.ModeSymlink, "symlink must be replaced by a regular file") + + data, err = os.ReadFile(filepath.Join(outputDir, "file.txt")) + require.NoError(t, err) + assert.Equal(t, payload, string(data)) +} + func createSymlinkZip(dest string) error { archiveFile, err := os.Create(dest) if err != nil { diff --git a/udf.go b/udf.go index 580b2af..bede6a3 100644 --- a/udf.go +++ b/udf.go @@ -91,8 +91,15 @@ func (x *XFile) unUDFEntry(udfImage *udf.Udf, entry *udf.File, parent string) (u func (x *XFile) unUDFDir(udfImage *udf.Udf, entry *udf.File, parent string) (uint64, []string, error) { dirPath := filepath.Join(parent, entry.Name()) + cleanPath := x.clean(dirPath) - err := x.mkDir(filepath.Join(x.OutputDir, dirPath), entry.Mode(), entry.ModTime()) + if !x.pathWithinOutput(cleanPath) { + // The directory is trying to land outside of our base path. Malicious UDF image? + return 0, nil, fmt.Errorf("%s: %w: %s (from: %s)", + x.FilePath, ErrInvalidPath, cleanPath, entry.Name()) + } + + err := x.mkDir(cleanPath, entry.Mode(), entry.ModTime()) if err != nil { return 0, nil, fmt.Errorf("making UDF directory %s: %w", entry.Name(), err) } diff --git a/zip.go b/zip.go index 52ffc2c..8fa1caa 100644 --- a/zip.go +++ b/zip.go @@ -4,7 +4,6 @@ import ( "archive/zip" "fmt" "path/filepath" - "sync" "time" ) @@ -75,7 +74,7 @@ func (x *XFile) extractZIPParallel( return x.prog.Wrote, files, err } - workerErr := x.zipDispatchWorkers(fileEntries) + workerErr := dispatchWorkers(x.FileWorkers, fileEntries, x.extractZIPEntry) if workerErr != nil { return x.prog.Wrote, files, workerErr } @@ -120,39 +119,6 @@ func (x *XFile) zipPrepareEntries( return entries, files, nil } -// zipDispatchWorkers sends file entries to a bounded worker pool for extraction. -func (x *XFile) zipDispatchWorkers(entries []zipFileEntry) error { - var ( - waitGroup sync.WaitGroup - firstErr error - errOnce sync.Once - semaphore = make(chan struct{}, x.FileWorkers) - ) - - for idx := range entries { - entry := entries[idx] - - if firstErr != nil { - break - } - - semaphore <- struct{}{} // acquire worker slot - - waitGroup.Go(func() { - defer func() { <-semaphore }() // release worker slot - - err := x.extractZIPEntry(entry) - if err != nil { - errOnce.Do(func() { firstErr = err }) - } - }) - } - - waitGroup.Wait() - - return firstErr -} - // extractZIPEntry extracts a single zip file entry (used by parallel workers). func (x *XFile) extractZIPEntry(entry zipFileEntry) error { zFile, err := entry.zipFile.Open()