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 {