fix(security): reject unsafe entries during archive extraction#91
Merged
Conversation
Unarchiver now scans an archive's table of contents before extracting and refuses any archive containing a symbolic/hard link, an absolute path, or a '..' traversal component. This blocks the Zip Slip / tar symlink-escape class of attacks where a crafted archive writes files outside the installation directory (e.g. ~/.ssh, shell rc files). Adds malicious fixtures (symlink zip/tar.gz, parent-traversal tar.gz) and tests asserting extraction is refused and nothing is written.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…nsafeArchiveEntries override Mirrors the --ignore-arch-check / ignoreArchCheck pattern so users can opt out of the Zip Slip / symlink-escape safety check when they trust the archive source. - Tool gains ignoreUnsafeArchiveEntries: Bool? (nil falls back to the global flag) - Unarchiver.init accepts ignoreUnsafeArchiveEntries: Bool and skips validateArchiveEntries when set - ToolInstaller computes the effective flag (per-tool ?? global) and passes it to Unarchiver - Installer threads ignoreUnsafeArchiveEntries through both public and internal inits - InstallCommand exposes --ignore-unsafe-entries CLI flag - Adds fixture Lucafile_mock_ignoreUnsafeArchiveEntries_true.yml and eight new tests
…staller init - Add MockAbsolutePath.zip fixture (zip with a /evil.txt entry) and a test that verifies extraction is refused with unsafeArchiveEntry - Refactor validateArchiveEntries to use if/else instead of switch, removing the dead case .executable: return branch that was never reachable from the call site - Add test_init_publicInit_ignoreUnsafeArchiveEntries_threadsThrough to exercise Installer's public init (which delegates ignoreUnsafeArchiveEntries through to the internal init), using a home-directory file manager for an early-exit path
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
.zipor.tar.gz, it shells out tounzip/tarto extract the archive into the tool's installation directory. The archive is fetched from a remote URL and, when no checksum is configured, is otherwise unverified.unzip(andtaron some platforms) will create a symbolic link and then write through it, so a crafted archive can drop files into arbitrary locations such as~/.ssh/authorized_keysor shell start-up files — the "Zip Slip" / tar symlink-escape class of attacks.Unarchivernow inspects the archive's table of contents before extracting anything and refuses the archive if any entry is a symbolic or hard link, uses an absolute path, or contains a..traversal component. A malicious archive therefore never reaches the extraction step.--ignore-unsafe-entriesCLI flag and a per-toolignoreUnsafeArchiveEntriesLucafile key let users opt out of the check for trusted archives — mirroring the existing--ignore-arch-check/ignoreArchCheckpattern exactly.unzip -Z/tar -tlistings per archive. Hard links are rejected alongside symlinks because they can also point outside the destination; legitimate tool archives do not rely on link entries.Type of Change
How Has This Been Tested?
swift buildandswift testlocally on macOS (arm64).New fixtures and tests:
MockSymlink.zip,MockSymlink.tar.gz— archives containing a symlink entry; extraction is refused withunsafeArchiveEntryand nothing is written.MockTraversal.tar.gz— archive with a../escape.txtentry; extraction is refused.test_unarchive_archiveContainingSymlink_ignoringUnsafeEntries_succeeds— verifies symlink archives extract when the flag is set.test_unarchive_archiveWithParentTraversal_ignoringUnsafeEntries_throwsFailedToUnarchiveNotUnsafeEntry— verifies our check is bypassed (tar itself still rejects../paths, surfaced asfailedToUnarchive).ToolInstallerTestscovering global flag true/false, per-tool override true/false, and nil fallback.Checklist
CI Considerations
Breaking Changes?
Additional Notes
The
--ignore-unsafe-entriesflag follows the identical layering as--ignore-arch-check: the CLI flag is the global default, the per-toolignoreUnsafeArchiveEntrieskey in the Lucafile overrides it (truealways skips,falsealways validates,nilfalls back to the CLI flag).