diff --git a/Packages/OsaurusCLI/Sources/OsaurusCLICore/Commands/Bundle/MCPBundleManager.swift b/Packages/OsaurusCLI/Sources/OsaurusCLICore/Commands/Bundle/MCPBundleManager.swift index f12d1d233..4829d1964 100644 --- a/Packages/OsaurusCLI/Sources/OsaurusCLICore/Commands/Bundle/MCPBundleManager.swift +++ b/Packages/OsaurusCLI/Sources/OsaurusCLICore/Commands/Bundle/MCPBundleManager.swift @@ -7,6 +7,7 @@ import Foundation import MCP +import OsaurusRepository class MCPBundleManager { struct BundleInfo { @@ -115,6 +116,17 @@ class MCPBundleManager { // Use unzip CLI try unzipWithCLI(bundlePath, to: extractPath) + // Defense in depth: reject ZIP-slip and symlink escape entries + // before reading the bundle manifest. Bundles are loaded from + // local paths the user picked, so they're trusted-ish, but + // validating costs almost nothing and shrinks the trust surface. + do { + try ArchiveSafety.validate(extractedRoot: URL(fileURLWithPath: extractPath)) + } catch { + try? FileManager.default.removeItem(atPath: extractPath) + throw BundleLoadError.extractionFailed("Unsafe bundle layout: \(error)") + } + let manifestPath = "\(extractPath)/manifest.json" guard FileManager.default.fileExists(atPath: manifestPath) else { try? FileManager.default.removeItem(atPath: extractPath) diff --git a/Packages/OsaurusCLI/Sources/OsaurusCLICore/Commands/Tools/ToolsInstall.swift b/Packages/OsaurusCLI/Sources/OsaurusCLICore/Commands/Tools/ToolsInstall.swift index b6ed123f2..35eb991e3 100644 --- a/Packages/OsaurusCLI/Sources/OsaurusCLICore/Commands/Tools/ToolsInstall.swift +++ b/Packages/OsaurusCLI/Sources/OsaurusCLICore/Commands/Tools/ToolsInstall.swift @@ -86,6 +86,7 @@ public struct ToolsInstall { } try data.write(to: zipFile) try unzip(zipURL: zipFile, to: tmpDir) + try ArchiveSafety.validate(extractedRoot: tmpDir) } catch { fputs("Download/Unzip error: \(error)\n", stderr) exit(EXIT_FAILURE) @@ -109,6 +110,7 @@ public struct ToolsInstall { } else if pathURL.pathExtension.lowercased() == "zip" { do { try unzip(zipURL: pathURL, to: tmpDir) + try ArchiveSafety.validate(extractedRoot: tmpDir) } catch { fputs("Unzip error: \(error)\n", stderr) exit(EXIT_FAILURE) diff --git a/Packages/OsaurusCore/Managers/Model/ModelManager.swift b/Packages/OsaurusCore/Managers/Model/ModelManager.swift index c87d515f6..dc6595695 100644 --- a/Packages/OsaurusCore/Managers/Model/ModelManager.swift +++ b/Packages/OsaurusCore/Managers/Model/ModelManager.swift @@ -188,7 +188,27 @@ final class ModelManager: NSObject, ObservableObject { // Pull the OsaurusAI HF org listing once on launch so newly published // models surface in the Recommended tab without requiring a code push. - Task { [weak self] in await self?.loadOsaurusAIOrgModels() } + // + // The unit-test runner constructs `ModelManager()` repeatedly to drive + // `applyOsaurusOrgFetch` directly. If the launch-time HF fetch races + // with those test calls, whichever finishes last wins and the merge + // result is non-deterministic — that's the regression class behind + // `ModelManagerSuggestedTests/applyOsaurusOrgFetch_*` flaking in CI. + // Skip the background fetch under XCTest; production launches still + // get it because `XCTestConfigurationFilePath` is only set inside + // a test host. + if !Self.isRunningInTestEnvironment { + Task { [weak self] in await self?.loadOsaurusAIOrgModels() } + } + } + + /// True when the current process was launched by xctest. Used to gate + /// network-touching launch-time side effects so tests can drive the + /// affected code paths deterministically. + nonisolated private static var isRunningInTestEnvironment: Bool { + ProcessInfo.processInfo.environment["XCTestConfigurationFilePath"] != nil + || ProcessInfo.processInfo.environment["XCTestBundlePath"] != nil + || ProcessInfo.processInfo.environment["XCTestSessionIdentifier"] != nil } // MARK: - Public Methods diff --git a/Packages/OsaurusCore/Managers/SkillManager.swift b/Packages/OsaurusCore/Managers/SkillManager.swift index 1a78f3b65..67705d8df 100644 --- a/Packages/OsaurusCore/Managers/SkillManager.swift +++ b/Packages/OsaurusCore/Managers/SkillManager.swift @@ -7,6 +7,7 @@ import Foundation import Observation +import OsaurusRepository import SwiftUI public enum SkillFileError: Error, LocalizedError { @@ -311,6 +312,15 @@ public final class SkillManager { try FileManager.default.createDirectory(at: tempDir, withIntermediateDirectories: true) try await FileManager.default.unzipItem(at: zipURL, to: tempDir) + // Defense in depth: skill archives are user-provided (drag-and-drop) + // and unsigned. Reject ZIP-slip and symlink-escape entries before + // any skill metadata is read or copied into the skills directory. + do { + try ArchiveSafety.validate(extractedRoot: tempDir) + } catch { + throw SkillFileError.invalidSkillArchive + } + guard let skillMdURL = findSkillMd(in: tempDir) else { throw SkillFileError.invalidSkillArchive } diff --git a/Packages/OsaurusCore/Tests/Plugin/ArchiveSafetyTests.swift b/Packages/OsaurusCore/Tests/Plugin/ArchiveSafetyTests.swift new file mode 100644 index 000000000..b1f86cee8 --- /dev/null +++ b/Packages/OsaurusCore/Tests/Plugin/ArchiveSafetyTests.swift @@ -0,0 +1,100 @@ +// +// ArchiveSafetyTests.swift +// osaurusTests +// +// Tests for the post-extraction ZIP-slip / symlink-escape validator in +// `OsaurusRepository.ArchiveSafety`. The validator runs after the extractor +// (currently /usr/bin/unzip) has written files to disk and is the last line +// of defense before the caller reads anything out of the extracted tree. +// + +import Foundation +import OsaurusRepository +import Testing + +@testable import OsaurusCore + +private struct TempDirectory { + let url: URL + + init(name: String = "osaurus-archive-tests-\(UUID().uuidString)") throws { + let dir = FileManager.default.temporaryDirectory.appendingPathComponent(name, isDirectory: true) + try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) + self.url = dir + } + + func cleanup() { + try? FileManager.default.removeItem(at: url) + } +} + +struct ArchiveSafetyTests { + + @Test func acceptsCleanTree() throws { + let root = try TempDirectory() + defer { root.cleanup() } + + let nestedDir = root.url.appendingPathComponent("src") + try FileManager.default.createDirectory(at: nestedDir, withIntermediateDirectories: true) + FileManager.default.createFile( + atPath: nestedDir.appendingPathComponent("lib.swift").path, + contents: Data("hi".utf8) + ) + FileManager.default.createFile( + atPath: root.url.appendingPathComponent("README.md").path, + contents: Data("readme".utf8) + ) + + try ArchiveSafety.validate(extractedRoot: root.url) + } + + @Test func rejectsSymlinkEscape() throws { + let root = try TempDirectory() + defer { root.cleanup() } + let outside = try TempDirectory(name: "osaurus-archive-outside-\(UUID().uuidString)") + defer { outside.cleanup() } + + // Simulate a malicious zip entry that, when extracted, became a + // symbolic link inside the destination pointing OUT of it. macOS + // Info-ZIP's own protections don't catch this case. + let link = root.url.appendingPathComponent("escape") + try FileManager.default.createSymbolicLink(at: link, withDestinationURL: outside.url) + + var threw = false + do { + try ArchiveSafety.validate(extractedRoot: root.url) + } catch ArchiveSafetyError.escapingSymlink { + threw = true + } catch { + threw = true + } + #expect(threw, "symlink to outside directory must be rejected") + } + + @Test func acceptsInternalSymlink() throws { + let root = try TempDirectory() + defer { root.cleanup() } + + let target = root.url.appendingPathComponent("real-file") + FileManager.default.createFile(atPath: target.path, contents: Data()) + + let link = root.url.appendingPathComponent("alias") + try FileManager.default.createSymbolicLink(at: link, withDestinationURL: target) + + try ArchiveSafety.validate(extractedRoot: root.url) + } + + @Test func rejectsExtractionRootMissing() { + let missing = FileManager.default.temporaryDirectory + .appendingPathComponent("osaurus-archive-missing-\(UUID().uuidString)") + var threw = false + do { + try ArchiveSafety.validate(extractedRoot: missing) + } catch ArchiveSafetyError.extractionRootMissing { + threw = true + } catch { + threw = true + } + #expect(threw) + } +} diff --git a/Packages/OsaurusRepository/ArchiveSafety.swift b/Packages/OsaurusRepository/ArchiveSafety.swift new file mode 100644 index 000000000..328d4ac15 --- /dev/null +++ b/Packages/OsaurusRepository/ArchiveSafety.swift @@ -0,0 +1,121 @@ +// +// ArchiveSafety.swift +// OsaurusRepository +// +// Post-extraction validation that catches ZIP-slip and symlink-escape +// attempts. After extracting an archive into a destination directory, +// call `ArchiveSafety.validate(extractedRoot:)` and abort the install +// if it throws. Validation is symlink-aware: every regular file, every +// directory, and every symlink target is required to resolve to a path +// inside the destination root. +// +// Why post-extraction: +// * Streaming-time validation would require a ZIP library; we currently +// shell out to /usr/bin/unzip and the same logic is duplicated across +// callers (PluginInstallManager, SkillManager, CLI ToolsInstall). +// * The destination root is a freshly-created temp directory the caller +// owns. Until validate() succeeds, the caller must not move or read +// the extracted contents. +// * Symlinks introduced by the archive (or by a sloppy build) are the +// attack vector left over once the unzip tool itself starts rejecting +// absolute / ".." entries. macOS Info-ZIP already rejects absolute +// paths and many ".." escapes; symlinks pointing outside the tree are +// what we still have to police. +// + +import Foundation + +public enum ArchiveSafetyError: Error, CustomStringConvertible { + case escapingPath(String) + case escapingSymlink(linkPath: String, target: String) + case extractionRootMissing(String) + + public var description: String { + switch self { + case .escapingPath(let p): + return "Archive entry resolves outside the extraction root: \(p)" + case .escapingSymlink(let link, let target): + return "Archive symlink \(link) points outside the extraction root: \(target)" + case .extractionRootMissing(let p): + return "Extraction root does not exist or is not a directory: \(p)" + } + } +} + +public enum ArchiveSafety { + + /// Walk `extractedRoot` and verify every entry is contained — both + /// lexically (no `..` escapes) and after symlink resolution. + /// + /// Throws `ArchiveSafetyError` on the first escape encountered. + /// Callers should treat the entire extracted tree as untrusted and + /// remove it on failure. + /// + /// Containment is compared by *path components* (after both sides go + /// through `resolvingSymlinksInPath().standardized`) rather than by + /// string prefix. On macOS the TemporaryDirectory and the URLs the + /// `FileManager.enumerator` returns are canonicalized inconsistently + /// (`/var` vs `/private/var` depending on the API), and a string + /// `hasPrefix` check produced false positives whenever one side was + /// resolved and the other wasn't. `Array.starts(with:)` on + /// `pathComponents` avoids that mismatch and is also immune to the + /// "shared name prefix" foot-gun (e.g. `/work/foo-baz` vs + /// `/work/foo`). + public static func validate(extractedRoot: URL) throws { + let fm = FileManager.default + var isDir: ObjCBool = false + guard fm.fileExists(atPath: extractedRoot.path, isDirectory: &isDir), isDir.boolValue else { + throw ArchiveSafetyError.extractionRootMissing(extractedRoot.path) + } + + // Canonicalize the root once. We compare candidate paths against + // BOTH the symlink-resolved form (catches a symlink whose target + // exits the root) AND the lexically-standardized form (catches a + // candidate whose own enumerator URL was returned with a different + // /var ↔ /private/var canonicalization than the root). + let canonicalRoot = extractedRoot.resolvingSymlinksInPath().standardized + let lexicalRoot = extractedRoot.standardized + let canonicalRootComponents = canonicalRoot.pathComponents + let lexicalRootComponents = lexicalRoot.pathComponents + + guard + let enumerator = fm.enumerator( + at: extractedRoot, + includingPropertiesForKeys: [.isSymbolicLinkKey], + options: [] + ) + else { + return + } + + for case let item as URL in enumerator { + // 1. Lexical containment — catches '..' / absolute-path entries + // that slipped past unzip. Compared against the lexically- + // standardized root. + let lexicalCandidate = item.standardized.pathComponents + let passesLexical = lexicalCandidate.starts(with: lexicalRootComponents) + + // 2. Symlink-aware containment — catches a symlink whose target + // exits the root. Compared against the symlink-resolved root. + let canonicalCandidate = item.resolvingSymlinksInPath().standardized.pathComponents + let passesCanonical = canonicalCandidate.starts(with: canonicalRootComponents) + + // Either canonicalization being inside is enough; macOS's + // /var ↔ /private/var inconsistency means one form or the + // other may match while the other doesn't, and both forms + // refer to the same real file. + guard passesLexical || passesCanonical else { + let isSymlink = + (try? item.resourceValues(forKeys: [.isSymbolicLinkKey]))? + .isSymbolicLink ?? false + if isSymlink { + let target = + (try? fm.destinationOfSymbolicLink(atPath: item.path)) + ?? item.resolvingSymlinksInPath().path + throw ArchiveSafetyError.escapingSymlink(linkPath: item.path, target: target) + } + throw ArchiveSafetyError.escapingPath(item.path) + } + } + } +} diff --git a/Packages/OsaurusRepository/PluginInstallManager.swift b/Packages/OsaurusRepository/PluginInstallManager.swift index 4e29f30d6..70727d45a 100644 --- a/Packages/OsaurusRepository/PluginInstallManager.swift +++ b/Packages/OsaurusRepository/PluginInstallManager.swift @@ -128,6 +128,19 @@ public final class PluginInstallManager: @unchecked Sendable { defer { try? FileManager.default.removeItem(at: tmpDir) } try unzip(zipURL: tmpZip, to: tmpDir) + // Defense in depth against ZIP-slip and symlink escape. macOS + // Info-ZIP already rejects absolute and ".."-laden entries by + // default, but symlinks inside the archive whose targets exit the + // extraction root still extract. The bundle is signature-verified + // above, so the threat surface here is mostly "publisher key + // compromise" — but the cost of validating is trivial relative to + // the cost of recovering from a successful escape. + do { + try ArchiveSafety.validate(extractedRoot: tmpDir) + } catch { + throw PluginInstallError.layoutInvalid("Archive contains escaping entry: \(error)") + } + guard let dylibURL = findFirstDylib(in: tmpDir) else { throw PluginInstallError.layoutInvalid("No .dylib found in archive") }