download: reject ZIP entries escaping the .utm folder - #7844
Merged
Conversation
A downloaded VM archive is attacker-controlled, so an entry named `some.utm/../../elsewhere` must not be able to write outside the package it claims to belong to. The extraction path was built by substituting the archive's .utm prefix out of the entry path and appending the remainder, and `appendingPathComponent()` does not resolve `..` -- the traversal was only resolved by the filesystem at write time, landing anywhere in the app container the depth reached. Derive the relative path by dropping the prefix, and resolve every destination through a containment check before extracting. Separators are collapsed in a loop because `fopen()` and `URL.standardized` disagree on a path such as `/../elsewhere`, and a single collapse pass still lets `///../elsewhere` through. Also bump ZIPFoundation to 0.9.20 for its accumulated fixes. This file is the only call site in the tree and the deprecated failable initializer it uses is still present, so the bump is source compatible. Reported-by: Vo Duc Thang (ugvxb) Assisted-by: Claude:claude-fable-5-1
The three error types in this file declare `errorDescription` but conform only to `Error`, so the description is never consulted. `downloadUTMZip()` alerts with `error.localizedDescription`, which for a plain `Error` falls back to "The operation couldn't be completed. (UTM.UTMDownloadVMTask... error 1.)" -- the localized strings here have never reached a user. Conform them to `LocalizedError` instead, matching every other error type in the codebase. No other type has this defect. `CreateUTMFailed` is currently unthrown; it is included so the three stay consistent rather than leaving one behind. Assisted-by: Claude:claude-fable-5-1
Bumping ZIPFoundation to 0.9.20 brought in its symlink containment guard, but that guard is not sufficient here. `URL.isContained(in:)` collapses repeated separators in a single pass, so a link target ending in `<package>///../elsewhere` is accepted while the filesystem resolves it to `<package>/../elsewhere`. A crafted archive could pair such a link with a later regular file underneath it, whose own path passes the containment check, and write outside the package again. Nothing in UTM ever stores a symlink inside a package, so reject them outright instead of trying to validate the target. Also remove the destination on failure. Extraction created the .utm directory up front and a mid-loop error left the partial contents in place, where the library would list them as a real virtual machine that survives a relaunch. Assisted-by: Claude:claude-fable-5-1
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.
utm://downloadVMhands the downloaded ZIP topartialUnzipOnlyUtmVM(), which built each entry's destination by substituting the archive's.utmprefix out of the entry path and appending the remainder.appendingPathComponent()does not resolve.., so an entry namedsome.utm/../../elsewherewas resolved only by the filesystem at write time and could write outside the package, anywhere in the app container the traversal depth reached. The download prompt is still shown first, so this needs the user to accept a download, but the archive contents are never shown to them.Every destination now goes through a containment check before extraction. Symlink entries are rejected outright: ZIPFoundation 0.9.20 gained its own symlink containment guard, but that guard collapses repeated separators in a single pass and accepts a target ending
<package>///../elsewherethat the filesystem resolves outside the package. Nothing in UTM stores a symlink inside a package, so there is nothing legitimate to lose.Two problems found while fixing this are in their own commits. A rejected archive used to leave its partially extracted package behind, where the library listed it as a real VM that survived a relaunch. And the error types in this file declared
errorDescriptionwithout conforming toLocalizedError, so the alert never showed why a download failed.ZIPFoundation moves from 0.9.17 to 0.9.20. This file is its only call site in the tree.
Reported by Vo Duc Thang (ugvxb).
Testing: Tested by a human on macOS 15.6, Mac mini (M4 Pro). The author acknowledges that this change has been tested and/or reviewed by a human in accordance with UTM's AI contribution guidelines.