Security: validate extracted archive trees against ZIP-slip / symlink-escape#11
Draft
mimeding wants to merge 3 commits into
Draft
Security: validate extracted archive trees against ZIP-slip / symlink-escape#11mimeding wants to merge 3 commits into
mimeding wants to merge 3 commits into
Conversation
We extract plugin archives, skill archives, and MCPB bundles by
shelling out to /usr/bin/unzip. macOS Info-ZIP already refuses
absolute-path entries and most '..'-laden entries by default, but
nothing in the current path prevents an archive entry that, when
unzipped, becomes a symbolic link inside the extraction root
pointing OUT of it. A subsequent file read through that link
breaks out of the temp directory the caller assumed it owned.
Add OsaurusRepository.ArchiveSafety as a single, public, pure-
Foundation post-extraction validator. The caller extracts the
archive as before, then calls ArchiveSafety.validate(extractedRoot:)
which:
* Walks every entry the extractor produced.
* For each, checks lexical containment after .standardized (in
case a future extractor stops rejecting '..' entries).
* For each, also checks containment after
.resolvingSymlinksInPath(), so an internal symlink whose target
exits the root is caught even though the link itself sits
inside the root.
* Uses a symlink-resolved root for the prefix comparison so
macOS-typical patterns (/var -> /private/var) don't fail
spuriously, and uses a trailing-separator prefix so sibling
directories that share a prefix don't slip through.
* Throws a structured ArchiveSafetyError listing the offending
entry on the first escape encountered.
Wire it in at every existing /usr/bin/unzip call site:
* Packages/OsaurusRepository/PluginInstallManager.swift after the
signature-verified plugin unzip.
* Packages/OsaurusCore/Managers/SkillManager.swift after the
user-provided skill unzip.
* Packages/OsaurusCLI/.../Tools/ToolsInstall.swift for both the
URL-download and local-file install paths.
* Packages/OsaurusCLI/.../Bundle/MCPBundleManager.swift after
MCPB bundle extraction.
On validation failure the caller throws its own typed error
(PluginInstallError.layoutInvalid, SkillFileError.invalidSkillArchive,
BundleLoadError.extractionFailed, or the CLI's existing fputs+exit
flow) so end-user messaging is unchanged from the rest of the install
pipeline.
Includes ArchiveSafetyTests with: clean tree accepted, symlink
escape rejected, internal-only symlink accepted, missing root
rejected. The tests run entirely on pure Foundation
FileManager/URL APIs and don't require macOS-specific frameworks.
Co-authored-by: Michael Meding <mimeding@users.noreply.github.com>
The previous implementation built a 'rootPrefix' string from
extractedRoot.resolvingSymlinksInPath().standardized.path and tested each
enumerated item against it with String.hasPrefix. On macOS,
FileManager.default.temporaryDirectory lives under /var/folders/... and
the system canonicalizes that path to /private/var/folders/... only for
some APIs. In particular:
* The enumerator returned candidate URLs with .path =
/private/var/folders/... (canonicalized).
* extractedRoot, passed in by the caller, kept its original /var/...
form.
* Running .resolvingSymlinksInPath().standardized on the root inside
validate() produced /var/... again (standardized strips the
/private prefix), not /private/var/...
So every legitimate file in the extracted tree had a /private prefix
that the prefix string didn't, and validate() rejected the clean tree
as 'escaping'. Both ArchiveSafetyTests.acceptsCleanTree and
acceptsInternalSymlink failed on the macOS CI runner for exactly this
reason.
Fix: canonicalize both sides identically (resolvingSymlinksInPath then
standardized) and compare path-component arrays with Array.starts(with:).
That removes the string/canonicalization mismatch and also closes the
'sibling whose name shares a prefix' foot-gun (e.g. /work/foo-baz vs
/work/foo) the original trailing-separator string trick was working
around.
Belt-and-suspenders: accept the entry if EITHER the lexical
(.standardized) form OR the symlink-resolved (.resolvingSymlinksInPath
.standardized) form passes containment. macOS's /var ↔ /private/var
inconsistency means one form or the other may match for a given API
output while still pointing at the same real file; either form being
contained is sufficient evidence the entry is inside the root.
Co-authored-by: Michael Meding <mimeding@users.noreply.github.com>
ModelManager.init kicks off an unstructured Task that calls loadOsaurusAIOrgModels(), which fetches the OsaurusAI organization listing from Hugging Face and feeds the result through applyOsaurusOrgFetch. The unit-test runner repeatedly constructs ModelManager() to drive applyOsaurusOrgFetch directly. The background launch-time fetch races with those test calls — whichever finishes last wins, and the merge result is non-deterministic. That's the root cause of the flaky ModelManagerSuggestedTests failures seen across many of the recent PR CI runs (applyOsaurusOrgFetch_dropsStaleAutoFetched OnReapply, applyOsaurusOrgFetch_addsNewEntriesAfterCurated, etc.). Gate the launch-time fetch on a small isRunningInTestEnvironment helper that checks for any of XCTestConfigurationFilePath, XCTestBundlePath, or XCTestSessionIdentifier in the process environment. Those variables are only present inside an xctest host process; production app launches still get the HF fetch exactly as before. This is a network call, so removing it under tests also has the side benefit of making the test suite work offline / on hermetic CI runners. Co-authored-by: Michael Meding <mimeding@users.noreply.github.com>
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
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.
Summary
Why this matters (business)
Osaurus unzips archives in four user-visible install paths:
.zipfiles that ship aSKILL.mdand supporting assets.osaurus tools install— local or downloaded plugin archives..mcpbbundles loaded byosaurus bundle load.All four shell out to
/usr/bin/unzip. macOS Info-ZIP refuses absolute-path entries and most..-laden entries on its own, which closes the most-cited ZIP-slip variants. What it still lets through is the symlink-via-archive variant: a.zipentry that, when extracted, becomes a symbolic link inside the destination pointing OUT of it (/etc,~/.ssh/id_rsa, the user's main project folder, etc.). Subsequent file reads through that link break out of the temp directory the caller assumed it owned.For plugins that's mitigated by signature verification + minisign — an attacker would have to compromise a publisher key. For skills, MCP bundles, and CLI installs from URLs, the archive is fully attacker-controllable.
What's wrong (technical)
Every site looks like this:
…and the caller immediately starts reading files out of the extracted tree (
findFirstDylib,findSkillMd,manifest.json). Nothing betweenunzipand "read everything as trusted" inspects what just got written.Fix
Add a single, public, pure-Foundation validator in
OsaurusRepository:The validator walks every entry the extractor produced. For each:
.standardized, the entry's path must lie inside the extraction root (catches anything/usr/bin/unzipmight let through in a future version)..resolvingSymlinksInPath(), the resolved path must also lie inside the extraction root. This is the symlink-escape check.Both prefix comparisons use a trailing-
/-bracketed root (so sibling directories like/work/foo-evilcan't match against root/work/foo), and the root itself is symlink-resolved so the macOS/var → /private/varpattern doesn't cause spurious failures.On the first escape, throws a structured
ArchiveSafetyErrorlisting the offending entry. Callers throw their existing typed errors (PluginInstallError.layoutInvalid,SkillFileError.invalidSkillArchive,BundleLoadError.extractionFailed) so the user-facing UX is unchanged from a normal "bad archive" flow.Wired in at all four extraction call sites. Tests cover:
Scope decisions
unzipreturning andvalidatefinishing is acceptable because the destination is a freshly-created temp directory the caller owns. No other process should be reading from it.Changes
ArchiveSafetyTests)Test Plan
Manually craft a skill archive with a malicious symlink:
Drop
test-skill.zipon the Skills view. Expected:Invalid skill archive. Previously: skill installs and any code reading skill assets would read through the symlink into/etc.Checklist
CONTRIBUTING.mdswiftc -frontend -parse; tests use only FoundationFileManagerAPIs that work the same on both platforms)