From de06669966950c116db0fc5d1415aa7151f0d6bc Mon Sep 17 00:00:00 2001 From: TerrifiedBug Date: Fri, 4 Sep 2026 08:42:09 +0100 Subject: [PATCH] Judge zip entry names, not standardized paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- Sources/yap/ZipArchive.swift | 29 ++++++++++++++++++++-------- Tests/yapTests/ZipArchiveTests.swift | 24 +++++++++++++++++++++++ 2 files changed, 45 insertions(+), 8 deletions(-) diff --git a/Sources/yap/ZipArchive.swift b/Sources/yap/ZipArchive.swift index 8679fd1..e1e7ec1 100644 --- a/Sources/yap/ZipArchive.swift +++ b/Sources/yap/ZipArchive.swift @@ -98,15 +98,28 @@ enum ZipArchive { } /// Entry names are attacker-controlled the moment a feed is: an archive - /// saying `../../bin/launchd` must not write there. Resolving and then - /// checking containment covers `..` and absolute names together. + /// saying `../../bin/launchd` must not write there. + /// + /// The name is judged, not the joined path. The previous version compared + /// two `standardizedFileURL` paths, which is a security check that depends + /// on the filesystem and on Foundation's willingness to resolve the + /// `/tmp` → `/private/tmp` class of symlink — and it does so + /// asymmetrically: hand it a *relative* destination and an ordinary + /// `yap.app/` entry is rejected as an escape. The updater only ever passes + /// an absolute path, so nothing shipped was broken; a rule this important + /// should not rest on that being true forever. + /// + /// Judging the components instead is symmetric, touches no filesystem, and + /// says exactly what is meant: every written path is the destination plus + /// a sequence of plain names. With symlink entries refused as well, that + /// is the whole containment argument — nothing inside the destination can + /// point outside it. private static func resolve(_ name: String, under destination: URL) throws -> URL { - let root = destination.standardizedFileURL.path - let target = destination.appendingPathComponent(name).standardizedFileURL - guard target.path == root || target.path.hasPrefix(root + "/") else { - throw ZipError.escapingPath(name) - } - return target + let components = name.split(separator: "/", omittingEmptySubsequences: true).map(String.init) + guard !name.hasPrefix("/"), !components.isEmpty, + !components.contains(".."), !components.contains(".") + else { throw ZipError.escapingPath(name) } + return components.reduce(destination) { $0.appendingPathComponent($1) } } private static func centralDirectory(in data: Data) throws -> [Entry] { diff --git a/Tests/yapTests/ZipArchiveTests.swift b/Tests/yapTests/ZipArchiveTests.swift index 1c02269..5e81ca4 100644 --- a/Tests/yapTests/ZipArchiveTests.swift +++ b/Tests/yapTests/ZipArchiveTests.swift @@ -30,6 +30,30 @@ final class ZipArchiveTests: XCTestCase { return destination } + /// The containment rule must not depend on how the caller spelled the + /// destination. The first version compared two standardized paths, which + /// resolves symlinks off the filesystem and does it asymmetrically: with a + /// relative destination it rejected an ordinary `yap.app/` entry. The + /// updater always passes an absolute path, so nothing shipped was broken — + /// this pins it so nothing has to keep being true by luck. + func testDestinationSpellingDoesNotAffectExtraction() throws { + let previous = FileManager.default.currentDirectoryPath + defer { FileManager.default.changeCurrentDirectoryPath(previous) } + XCTAssertTrue(FileManager.default.changeCurrentDirectoryPath(directory.path)) + + let archive = directory.appendingPathComponent("fixture.zip") + try XCTUnwrap(Data(base64Encoded: Self.fixture, options: .ignoreUnknownCharacters)) + .write(to: archive) + + // Relative, resolved against the working directory set above. + try ZipArchive.extract(URL(fileURLWithPath: "fixture.zip"), to: URL(fileURLWithPath: "out")) + XCTAssertEqual( + try String( + contentsOf: directory.appendingPathComponent("out/bundle/Info.plist"), + encoding: .utf8), + "") + } + /// Deflated, and 0755: a Mach-O written 0644 is a bundle macOS refuses to /// launch, and nothing else in the update path would notice. func testDeflatedEntryKeepsItsContentsAndExecutableBit() throws {