Skip to content

fix: block symlink-follow output escapes + assorted correctness fixes#161

Open
oceanplexian wants to merge 8 commits into
golift:mainfrom
oceanplexian:fix/path-escape-and-misc
Open

fix: block symlink-follow output escapes + assorted correctness fixes#161
oceanplexian wants to merge 8 commits into
golift:mainfrom
oceanplexian:fix/path-escape-and-misc

Conversation

@oceanplexian

Copy link
Copy Markdown
Contributor

Summary

Bug-fix pass over the extractors, focused on path traversal / output-folder escapes. Each commit is small and self-contained; the three security: commits are the headline, the rest are correctness fixes found along the way. All changes include regression tests where the bug is practically reproducible.

Security fixes

1. Symlink-follow escapes in the output folder (security: block symlink-follow escapes)

The lexical path checks from #157/#159 don't stop writes through symlinks already present in the output folder: os.MkdirAll and os.OpenFile follow them. Two live variants, both reproduced before fixing:

out/sub -> /attacker/dir          (pre-existing symlink inside OutputDir)
archive contains: sub/pwned.txt   => written to /attacker/dir/pwned.txt

out/file.txt -> /attacker/victim  (pre-existing symlink at the write target)
archive contains: file.txt        => O_TRUNC follows the link, victim overwritten

Since every extractor funnels through mkDir + writeFile, the fix lives in those two choke points:

  • resolveExisting() resolves symlinks in the deepest existing portion of a path and re-appends the not-yet-created tail (also normalizes OutputDir itself, e.g. /var -> /private/var on macOS).
  • mkDir rejects any resolved folder that lands outside the resolved OutputDir.
  • writeFile removes a pre-existing symlink at the write target instead of truncating through it.
  • createHardLink now validates the resolved target too.

Note: MkdirAll can still create empty dirs through a hostile symlink before detection (component-wise secure mkdir is a bigger change); no file data is written outside anymore.

2. ISO9660/UDF directory paths unchecked (security: validate ISO9660/UDF directory paths)

File entries in ISO/UDF images get the pathWithinOutput guard, but directory entries were joined + created raw. A crafted image could create and then populate directories outside the output folder. Both now go through clean() + pathWithinOutput, matching the file-entry guards.

3. CUE sheet FILE escape (security: keep CUE-referenced audio inside the cue folder)

A CUE sheet with FILE "../../secret.flac" resolved outside the cue folder, letting a crafted archive read and copy sibling files into the extraction output. resolveCueAudioPath now rejects entries that leave the cue folder (nested subfolders still work).

Correctness fixes

  • rar/7z password retries dropped config — the retry loops built a bare XFile per attempt, silently losing the logger, progress callbacks, SquashRoot, FileWorkers (rar) and moveFiles. Now copies the caller's XFile and swaps only the password.
  • Data race in parallel zip/7z dispatch — the dispatch loop read firstErr while workers wrote it under sync.Once; flagged by -race when a worker fails mid-dispatch. Both duplicated pools are replaced with one generic dispatchWorkers helper using atomic.Pointer.
  • Rename copy fallback leaked the source fd on openFile/io.Copy errors, and applied Chtimes to the pre-truncation path.
  • Signature detection used a single Read — short reads could miss deeper signatures (ISO at 0x8001+). Now io.ReadFull. Also stops logging extension error: <nil> when no extension matched.
  • cpio link entries missed parent-dir creation — tar links already ensure the parent exists; cpio symlink/hard-link entries failed on archives without explicit directory entries.

Testing

  • go test -race ./... passes; go vet and golangci-lint run clean (one pre-existing prealloc nit in chardet.go on main).
  • New tests: pre-existing symlink dir escape (zip + tar), final-component symlink replacement, CUE traversal (incl. legit nested path), rar password-retry config retention, parallel worker-phase error propagation.

Notes for review

  • No public API changes; returned file lists and error types are unchanged (ErrInvalidPath is reused for the new rejections).
  • Out of scope, flagged for later: RPM/cpio payloads with absolute symlink targets (common in real RPMs) currently fail the strict symlink checks — that posture came from fix: restore ZIP/RAR/7z symlinks instead of file stubs #155 and may deserve a "skip with warning" mode for package payloads.

The lexical path check does not stop writes through symlinks already
present in the output folder: os.MkdirAll and os.OpenFile follow them,
landing archived data outside the output folder.

- resolveExisting() resolves symlinks in the deepest existing portion of
  a path and re-appends the not-yet-created tail.
- mkDir() now verifies the resolved folder stays inside the resolved
  output folder (covers every extractor, all funnel through mkDir).
- writeFile() removes a pre-existing symlink at the write target instead
  of truncating through it (O_TRUNC follows links).
- createHardLink() also validates the resolved link target.
File entries in ISO and UDF images are checked against the output
folder, but directory entries were joined and created unchecked, so a
crafted image could create (and then populate) directories outside the
output folder. Route both through clean() + pathWithinOutput, matching
the file-entry guards.
A crafted CUE sheet with FILE "../../secret.flac" resolved outside the
cue folder, letting an archive read and copy sibling files into the
extraction output. Reject FILE entries that leave the cue folder.
The password retry loops built a bare XFile per attempt, dropping the
logger, progress callbacks, SquashRoot, FileWorkers (rar) and the wired
moveFiles helper. Copy the caller's XFile and only swap the password.
The dispatch loop read firstErr while worker goroutines wrote it under
sync.Once - an unsynchronized read the race detector flags when a worker
fails mid-dispatch. Replace both duplicated pools with one generic
dispatchWorkers helper that shares the first error via atomic.Pointer.
The cross-device rename fallback leaked oldFile when openFile or io.Copy
failed, and applied Chtimes to the pre-truncation path.
A single Read may return short data, missing signatures stored deeper in
the file (ISO at 0x8001+). Also stop printing 'extension error: <nil>'
in the fallback debug line when no extension matched.
Unlike tar links, cpio symlink/hard-link entries were created without
ensuring the parent folder exists, failing on archives that omit
explicit directory entries.
@oceanplexian
oceanplexian force-pushed the fix/path-escape-and-misc branch from a68b864 to c2b3547 Compare July 16, 2026 20:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens extraction against output-directory escapes (especially via pre-existing symlinks) and applies several correctness fixes across extractors and supporting utilities, with regression tests covering the newly addressed cases.

Changes:

  • Prevent writes escaping OutputDir via pre-existing symlinks by adding resolved-path containment checks and replacing final-component symlinks at write targets.
  • Apply consistent path validation for ISO9660/UDF directory entries and restrict CUE FILE references to remain within the CUE sheet’s folder.
  • Improve correctness and robustness: preserve XFile config across RAR/7z password retries, introduce a shared parallel worker dispatcher, fix signature detection reads, and patch smaller extraction edge cases (e.g., cpio link parent dirs, Rename fallback behaviors).

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
zip.go Switch ZIP parallel extraction to shared dispatchWorkers helper.
7z.go Switch 7z parallel extraction to shared dispatchWorkers helper; preserve config across password retries.
files.go Add dispatchWorkers, resolved-path containment helpers, symlink-safe mkDir/writeFile, logging tweak, and Rename fallback fixes.
udf.go Validate and guard UDF directory entry paths via clean() + pathWithinOutput.
iso.go Validate and guard ISO directory entry paths via clean() + pathWithinOutput.
cue.go Reject CUE FILE references that escape the cue directory; remove now-unneeded lint suppression.
cpio.go Ensure parent directory exists for cpio link entries before creating the link.
magic.go Use io.ReadFull to avoid short-read signature misses.
symlink_test.go Add regression tests for pre-existing symlink directory escape and final-component symlink replacement.
parallel_test.go Add coverage for worker-phase error propagation in parallel ZIP extraction.
rar.go Preserve full caller configuration across password retries by copying XFile per attempt.
rar_test.go Add regression test ensuring password retries keep caller logger/config.
cue_internal_test.go Add internal tests for CUE traversal rejection and valid nested audio paths.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread files.go
Comment on lines +844 to +849
// 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))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants