Judge zip entry names, not standardized paths - #20
Merged
Conversation
Extraction containment compared two `standardizedFileURL` paths, which makes a security check depend on the filesystem and on Foundation's willingness to resolve the `/tmp` → `/private/tmp` class of symlink. It does that asymmetrically: hand it a relative destination and an ordinary `yap.app/` entry is rejected as an escape. Nothing shipped was broken — `Paths.updatesDirectory` is absolute, and that is the only caller — so this is not a fix for a live defect. It is a rule that should not rest on the caller's spelling being right forever. Judging the components is symmetric, touches no filesystem, and says what is meant: every written path is the destination plus a sequence of plain names, with `..`, `.`, absolute names and empty names refused. Symlink entries were already refused, so together that is the whole containment argument. The test pins it and was confirmed to fail on the old implementation. Also verified, because it was never covered: a *stapled* app survives the round trip. The notarization ticket for a bundle is a regular file at `Contents/CodeResources`, not an extended attribute, so dropping AppleDouble sidecars does not drop it. Checked against the published notarized 0.2.0 build rather than a local unstapled one — zipped with the release command, extracted through `ZipArchive`, then `stapler validate` passes, `spctl --assess` reports "accepted, source=Notarized Developer ID", and the tree is byte-identical to the original.
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.
Follow-up to #19, found while verifying the updater against a stapled app rather than a merely signed one.
The containment check
ZipArchive.resolvecompared twostandardizedFileURLpaths. That makes a security check depend on the filesystem and on Foundation's willingness to resolve the/tmp→/private/tmpclass of symlink — and it does so asymmetrically:So with a relative destination, an ordinary
yap.app/entry is rejected as an escape.Nothing shipped was broken.
Paths.updatesDirectoryis absolute and it is the only caller; I hit this in a throwaway harness that passed a relative path. This is not a fix for a live defect — it is a rule that should not rest on the caller's spelling staying right forever.Judging the components instead is symmetric, touches no filesystem, and says what is meant: every written path is the destination plus a sequence of plain names, with
..,., absolute names and empty names refused. Symlink entries were already refused, so together that is the whole containment argument — nothing inside the destination can point outside it.The new test was confirmed to fail against the old implementation (
escapingPath("bundle/MacOS/tool")) and pass against the new one.Stapling, which was never covered
build-release.shrunsxattr -crbefore signing, butstapler stapleruns after, andZipArchivedeliberately drops every AppleDouble/xattr entry — so "nothing needs those sidecars" did not actually cover the notarization ticket. Checked properly, against the published notarized 0.2.0 build rather than a local unstapled one:Contents/CodeResources, not an extended attribute;ZipArchive:stapler validate→ "The validate action worked!";codesign --verify --strict→ valid on disk, satisfies its Designated Requirement;spctl --assess --type execute→ accepted, source=Notarized Developer ID;diff -ragainst the original bundle → byte-identical tree, ticket included.So an in-app update installs a still-notarized app, and Gatekeeper is satisfied offline.
48 tests.