diff --git a/clients/swift/AgentHostProtocol/Sources/AgentHostProtocolClient/Hosts/ClientIdStore.swift b/clients/swift/AgentHostProtocol/Sources/AgentHostProtocolClient/Hosts/ClientIdStore.swift index 859968b3..96a0f21c 100644 --- a/clients/swift/AgentHostProtocol/Sources/AgentHostProtocolClient/Hosts/ClientIdStore.swift +++ b/clients/swift/AgentHostProtocol/Sources/AgentHostProtocolClient/Hosts/ClientIdStore.swift @@ -57,10 +57,10 @@ public final class InMemoryClientIdStore: ClientIdStore { /// Filesystem-backed `ClientIdStore` that survives process restarts. /// -/// Stores one file per host id under a configurable directory; writes are -/// atomic (`.atomic` Data option) and best-effort restrict file permissions -/// to owner-read/write on POSIX platforms so the persisted ids aren't -/// world-readable. Per-store mutations are serialised through an internal +/// Stores one file per host id under a configurable directory. Writes go +/// through a temp file opened with owner-read/write permissions and are then +/// renamed into place, so the persisted ids are atomic on the same volume and +/// are never present on disk at world-readable permissions. Per-store mutations are serialised through an internal /// actor so concurrent `load`/`store` calls from different hosts don't race /// on the directory's contents. /// @@ -118,15 +118,70 @@ public final class FileClientIdStore: ClientIdStore { ensureDirectory() let url = fileURL(for: hostId) guard let data = clientId.data(using: .utf8) else { return } - do { - try data.write(to: url, options: [.atomic]) - // Best-effort restrict permissions to owner-only on POSIX - // platforms. Silently ignore on platforms where this - // attribute isn't applicable. - try? fm.setAttributes([.posixPermissions: 0o600], ofItemAtPath: url.path) - } catch { + + // Create the temp file with owner-only permissions BEFORE any bytes + // land in it, then rename(2) it into place. + // + // The previous shape — `data.write(to:options:.atomic)` followed by a + // `setAttributes(.posixPermissions)` — put the id on disk at the + // umask default (0644 under a typical umask) and only tightened it + // afterwards, so the first store for a host id was briefly world + // readable at a predictable path. The 0700 on `directory` does not + // cover that: `ensureDirectory()` only applies it on the branch where + // it CREATES the directory, so a pre-existing Application Support or + // XDG path keeps its own mode. + // + // `open(2)` with an explicit mode has no such window, and rename(2) is + // atomic and carries the 0600 across — including over a destination + // that was already left at looser permissions by an older build. + let tempURL = directory.appendingPathComponent(".\(UUID().uuidString).tmp") + let fd = tempURL.withUnsafeFileSystemRepresentation { path -> Int32 in + guard let path else { return -1 } + return open(path, O_WRONLY | O_CREAT | O_EXCL | O_CLOEXEC, 0o600) + } + guard fd >= 0 else { + #if DEBUG + print("[FileClientIdStore] failed to create temp file for \(hostId)") + #endif + return + } + // open(2)'s mode argument is masked by the process umask, so a umask + // carrying 0o200 would leave the file read-only (0o400) and a stricter one + // could strip owner access entirely. fchmod is not masked, so it pins the + // mode to exactly 0o600 — the guarantee the previous chmod-after-write + // shape did provide, kept here without reopening the window it left. This + // mirrors the netstandard2.0 leg of the .NET fix in #411. + _ = fchmod(fd, 0o600) + + var written = 0 + data.withUnsafeBytes { buffer in + guard let base = buffer.baseAddress else { return } + while written < buffer.count { + let n = write(fd, base + written, buffer.count - written) + if n <= 0 { break } + written += n + } + } + close(fd) + + guard written == data.count else { + try? fm.removeItem(at: tempURL) + #if DEBUG + print("[FileClientIdStore] short write persisting id for \(hostId)") + #endif + return + } + + let renamed = tempURL.withUnsafeFileSystemRepresentation { src -> Bool in + url.withUnsafeFileSystemRepresentation { dst -> Bool in + guard let src, let dst else { return false } + return rename(src, dst) == 0 + } + } + if !renamed { + try? fm.removeItem(at: tempURL) #if DEBUG - print("[FileClientIdStore] failed to persist id for \(hostId): \(error)") + print("[FileClientIdStore] failed to persist id for \(hostId)") #endif } } diff --git a/clients/swift/AgentHostProtocol/Tests/AgentHostProtocolClientTests/FileClientIdStoreTests.swift b/clients/swift/AgentHostProtocol/Tests/AgentHostProtocolClientTests/FileClientIdStoreTests.swift index 5554f701..a306a173 100644 --- a/clients/swift/AgentHostProtocol/Tests/AgentHostProtocolClientTests/FileClientIdStoreTests.swift +++ b/clients/swift/AgentHostProtocol/Tests/AgentHostProtocolClientTests/FileClientIdStoreTests.swift @@ -89,18 +89,141 @@ final class FileClientIdStoreTests: XCTestCase { } func testFileIsRestrictedToOwnerWhenPossible() async throws { - // Smoke test for the perm-restriction code path on POSIX - // platforms. We don't assert on non-POSIX file systems where - // this is a no-op. let store = FileClientIdStore(directory: tempDir) await store.store("h", clientId: "value") let url = tempDir.appendingPathComponent("h.clientid") - if let attrs = try? FileManager.default.attributesOfItem(atPath: url.path), - let perms = attrs[.posixPermissions] as? NSNumber { - // 0o600 = 384 - XCTAssertEqual(perms.intValue & 0o777, 0o600, - "expected owner-only permissions on the persisted file") + // Assert unconditionally. This used to be wrapped in `if let attrs = try?` + // with no `else`, so a failure to read the attributes passed the test + // silently rather than failing it. + let attrs = try FileManager.default.attributesOfItem(atPath: url.path) + let perms = try XCTUnwrap(attrs[.posixPermissions] as? NSNumber) + XCTAssertEqual(perms.intValue & 0o777, 0o600, + "expected owner-only permissions on the persisted file") + // This runs after `store()` returns, so on its own it cannot tell a file + // created 0600 from one created loose and chmod'd afterwards. The two tests + // below cover that difference directly. + } + + /// The window itself. `Data.write(to:options:.atomic)` writes a temp file in the + /// destination directory at the umask default and only chmods after the rename, so + /// the client id is briefly readable by any local user. This watches the directory + /// while a store runs and fails if ANY file in it is ever observed carrying group or + /// other permission bits. + /// + /// Measured against the pre-fix implementation this observes the leak in 19 of 20 + /// runs; against the current one, 0 of 20. The asymmetry is deliberate — the + /// assertion is "nothing was ever loose", which the current implementation satisfies + /// by construction, so the test cannot fail spuriously. Only a real regression fails + /// it, and the repeat count makes missing one vanishingly unlikely. + func testClientIdIsNeverObservableAtLoosePermissionsDuringStore() async throws { + for attempt in 0..<5 { + let dir = tempDir.appendingPathComponent("attempt-\(attempt)") + try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) + // 0755, and pre-existing: `ensureDirectory()` only applies 0700 on the branch + // where it CREATES the directory, so this is the reachable configuration. + try FileManager.default.setAttributes([.posixPermissions: 0o755], ofItemAtPath: dir.path) + + let observer = LoosePermissionObserver() + let stop = DispatchSemaphore(value: 0) + DispatchQueue.global().async { + let fm = FileManager.default + while stop.wait(timeout: .now()) == .timedOut { + guard let entries = try? fm.contentsOfDirectory(atPath: dir.path) else { continue } + for entry in entries { + let path = dir.appendingPathComponent(entry).path + guard let attrs = try? fm.attributesOfItem(atPath: path), + let perms = attrs[.posixPermissions] as? NSNumber, + perms.intValue & 0o077 != 0 + else { continue } + observer.record("\(entry) was 0\(String(perms.intValue & 0o777, radix: 8))") + } + } + } + + // A payload large enough that the write is not instantaneous. 1 MB detects the + // pre-fix leak in 20 of 20 trials; the value is about widening the window, not + // about any realistic client-id length. + let store = FileClientIdStore(directory: dir) + await store.store("h", clientId: String(repeating: "s", count: 1_000_000)) + + stop.signal() + try await Task.sleep(nanoseconds: 20_000_000) + + XCTAssertNil(observer.first, + "client id was observable at loose permissions during store(): \(observer.first ?? "")") + } + } + + /// `open(2)`'s mode argument is masked by the process umask, so opening with 0600 + /// under a umask carrying 0o200 yields a read-only 0400 file. The explicit `fchmod` + /// is what pins it to exactly 0600. Without that call this test fails with 0400. + func testPersistedModeIsExactlyOwnerOnlyRegardlessOfUmask() async throws { + try FileManager.default.createDirectory(at: tempDir, withIntermediateDirectories: true) + try FileManager.default.setAttributes([.posixPermissions: 0o700], ofItemAtPath: tempDir.path) + + // umask is process-wide; restore it immediately so no sibling test sees it. + let saved = umask(0o277) + let store = FileClientIdStore(directory: tempDir) + await store.store("h", clientId: "value") + umask(saved) + + let url = tempDir.appendingPathComponent("h.clientid") + let attrs = try FileManager.default.attributesOfItem(atPath: url.path) + let perms = try XCTUnwrap(attrs[.posixPermissions] as? NSNumber) + XCTAssertEqual(perms.intValue & 0o777, 0o600, + "expected exactly 0600 under a restrictive umask, got 0\(String(perms.intValue & 0o777, radix: 8))") + let value = await store.load("h") + XCTAssertEqual(value, "value", "a file the owner cannot read back is not a fix") + } + + /// `rename(2)` carries the temp file's mode onto the destination, so a file + /// left at loose permissions by an older build is repaired by the next store + /// rather than kept. + func testStoreRepairsPreExistingLoosePermissions() async throws { + try FileManager.default.createDirectory(at: tempDir, withIntermediateDirectories: true) + let url = tempDir.appendingPathComponent("h.clientid") + XCTAssertTrue(FileManager.default.createFile( + atPath: url.path, + contents: Data("stale".utf8), + attributes: [.posixPermissions: 0o644])) + + let store = FileClientIdStore(directory: tempDir) + await store.store("h", clientId: "fresh") + + let attrs = try FileManager.default.attributesOfItem(atPath: url.path) + let perms = try XCTUnwrap(attrs[.posixPermissions] as? NSNumber) + XCTAssertEqual(perms.intValue & 0o777, 0o600, + "a pre-existing 0644 client-id file should be repaired to 0600") + let value = await store.load("h") + XCTAssertEqual(value, "fresh") + } + + /// A failed or interrupted store must not leave its temp file behind. + func testNoTempFilesLeftBehind() async throws { + let store = FileClientIdStore(directory: tempDir) + for i in 0..<8 { + await store.store(HostId("h-\(i)"), clientId: "id-\(i)") } + let entries = try FileManager.default.contentsOfDirectory(atPath: tempDir.path) + let temps = entries.filter { $0.hasSuffix(".tmp") } + XCTAssertTrue(temps.isEmpty, "left temp files behind: \(temps)") + } +} + +/// Records the first loose-permission observation. A class with a lock rather than an +/// actor so the polling closure can write to it without an await. +private final class LoosePermissionObserver: @unchecked Sendable { + private let lock = NSLock() + private var _first: String? + + var first: String? { + lock.lock(); defer { lock.unlock() } + return _first + } + + func record(_ description: String) { + lock.lock(); defer { lock.unlock() } + if _first == nil { _first = description } } } diff --git a/docs/.changes/20260824-swift-client-id-permissions.json b/docs/.changes/20260824-swift-client-id-permissions.json new file mode 100644 index 00000000..882d97d3 --- /dev/null +++ b/docs/.changes/20260824-swift-client-id-permissions.json @@ -0,0 +1,5 @@ +{ + "type": "security", + "message": "Swift `FileClientIdStore` creates its client-id file with owner-only permissions before writing, instead of writing at the umask default and restricting afterwards, so the persisted id is never briefly readable by other local users.", + "targets": ["swift"] +}