diff --git a/Package.resolved b/Package.resolved index 26a7fce4..2741320b 100644 --- a/Package.resolved +++ b/Package.resolved @@ -1,5 +1,5 @@ { - "originHash" : "823bd101889f099d386f6437c6d48629379ae716d34181656b0ede5caa58b43e", + "originHash" : "259150ed43b4de53af8834d1b5d8163b8c8c2212b2490cb4215008ed34987953", "pins" : [ { "identity" : "async-http-client", @@ -15,8 +15,8 @@ "kind" : "remoteSourceControl", "location" : "https://github.com/apple/containerization.git", "state" : { - "revision" : "6adea5663427b4068108e9268e6125fc139b6045", - "version" : "0.52.0" + "revision" : "836080dd4b8090372035882116a264053a22ff04", + "version" : "0.52.1" } }, { diff --git a/Package.swift b/Package.swift index cb555569..047651db 100644 --- a/Package.swift +++ b/Package.swift @@ -23,7 +23,7 @@ import PackageDescription let releaseVersion = ProcessInfo.processInfo.environment["RELEASE_VERSION"] ?? "0.0.0" let gitCommit = ProcessInfo.processInfo.environment["GIT_COMMIT"] ?? "unspecified" let builderShimVersion = "0.13.2" -let scVersion = "0.52.0" +let scVersion = "0.52.1" let package = Package( name: "container", diff --git a/Sources/ContainerBuild/BuildFSSync.swift b/Sources/ContainerBuild/BuildFSSync.swift index c5a5288f..5d228ae9 100644 --- a/Sources/ContainerBuild/BuildFSSync.swift +++ b/Sources/ContainerBuild/BuildFSSync.swift @@ -18,9 +18,11 @@ import Collections import ContainerAPIClient import ContainerizationArchive import ContainerizationOCI +import ContainerizationOS import CryptoKit import Foundation import GRPCCore +import SystemPackage /// Handles the `fssync` stage of the build protocol. /// @@ -50,10 +52,19 @@ import GRPCCore /// The host enforces that no file served to the builder resolves to a path /// outside the context root. If any component of a requested path is a symlink /// whose target lies outside the context root the request is rejected. +/// +/// ``read(_:_:_:)`` and ``info(_:_:_:)`` enforce this by walking the path +/// component by component from a directory descriptor pinned on the context +/// root (``FileDescriptorOps``), so there is no window between checking a path +/// and using it in which a symlink can be swapped in. Symlinks are followed +/// only to relative targets that stay beneath the context root. /// Dockerignore filtering is **not** applied here; the shim applies it after /// unpacking the tar. actor BuildFSSync: BuildPipelineHandler { let contextDir: URL + /// Descriptor for `contextDir`, opened once so every request is resolved + /// relative to the same directory regardless of later changes to its path. + private let contextFD: FileDescriptor init(_ contextDir: URL) throws { let resolved = contextDir.resolvingSymlinksInPath() @@ -65,6 +76,12 @@ actor BuildFSSync: BuildPipelineHandler { } self.contextDir = resolved + self.contextFD = try FileDescriptor.open( + FilePath(resolved.cleanPath), .readOnly, options: [.directory, .closeOnExec]) + } + + deinit { + try? contextFD.close() } nonisolated func accept(_ packet: ServerStream) throws -> Bool { @@ -97,37 +114,31 @@ actor BuildFSSync: BuildPipelineHandler { /// Serves the content of a single context file to the shim. /// /// Called only via the shim's `FS.Open()` fallback path, not during a - /// normal `Walk`-based build. Rejects any path whose symlink chain resolves - /// outside the context root. + /// normal `Walk`-based build. The file is opened by walking its path from + /// the pinned context descriptor, following only symlinks that stay beneath + /// the context root; anything else is rejected with ``Error/pathIsNotChild``. func read(_ sender: AsyncStream.Continuation, _ packet: BuildTransfer, _ buildID: String) async throws { let offset: UInt64 = packet.offset() ?? 0 let size: Int = packet.len() ?? 0 - var path: URL - if packet.source.hasPrefix("/") { - path = URL(fileURLWithPath: packet.source).standardizedFileURL - } else { - path = - contextDir - .appendingPathComponent(packet.source) - .standardizedFileURL - } - if !FileManager.default.fileExists(atPath: path.cleanPath) { - path = URL(filePath: self.contextDir.cleanPath) - path.append(components: packet.source.cleanPathComponent) - } - let resolved = path.resolvingSymlinksInPath() - guard self.contextDir.parentOf(resolved) else { - throw Error.pathIsNotChild(resolved.cleanPath, self.contextDir.cleanPath) - } - let data = try { - if try path.isDir() { - return Data() - } - let file = try LocalContent(path: path.standardizedFileURL) - return try file.data(offset: offset, length: size) ?? Data() - }() + let relative = try self.relativePath(for: packet.source) - let transfer = try path.buildTransfer(id: packet.id, contextDir: self.contextDir, complete: true, data: data) + let (data, status) = try self.confined(packet.source) { () -> (Data, FileDescriptorOps.FileStatus) in + do { + let fd = try FileDescriptorOps.openFile(self.contextFD, relativePath: relative, symlinks: .followBeneath) + defer { try? fd.close() } + // Metadata and content both come from the descriptor, so they + // describe the same inode. + let status = try FileDescriptorOps.status(of: fd) + let handle = FileHandle(fileDescriptor: fd.rawValue, closeOnDealloc: false) + try handle.seek(toOffset: offset) + let data = size == 0 ? try handle.readToEnd() : try handle.read(upToCount: size) + return (data ?? Data(), status) + } catch FileDescriptorOps.Error.conflict(.directory) { + return (Data(), try self.entryStatus(relative)) + } + } + + let transfer = BuildTransfer(id: packet.id, source: relative.string, status: status, complete: true, data: data) var response = ClientStream() response.buildID = buildID response.buildTransfer = transfer @@ -139,23 +150,26 @@ actor BuildFSSync: BuildPipelineHandler { /// context path. /// /// Called only via the shim's `FS.Open()` fallback path, not during a - /// normal `Walk`-based build. Must reject paths that escape the context root - /// via symlinks for the same reasons as ``read(_:_:_:)``. + /// normal `Walk`-based build. Applies the same descriptor-relative + /// confinement as ``read(_:_:_:)``. The metadata is that of the entry + /// itself (a symlink is not dereferenced), but a symlink is rejected unless + /// it also resolves beneath the context root. func info(_ sender: AsyncStream.Continuation, _ packet: BuildTransfer, _ buildID: String) async throws { - let path: URL - if packet.source.hasPrefix("/") { - path = URL(fileURLWithPath: packet.source).standardizedFileURL - } else { - path = - contextDir - .appendingPathComponent(packet.source) - .standardizedFileURL + let relative = try self.relativePath(for: packet.source) + + let status = try self.confined(packet.source) { () -> FileDescriptorOps.FileStatus in + let status = try self.entryStatus(relative) + if status.type == .symlink { + do { + try FileDescriptorOps.openFile(self.contextFD, relativePath: relative, symlinks: .followBeneath).close() + } catch FileDescriptorOps.Error.conflict, FileDescriptorOps.Error.notFound { + // The link stays beneath the context; its target is just not a regular file. + } + } + return status } - let resolved = path.resolvingSymlinksInPath() - guard self.contextDir.parentOf(resolved) else { - throw Error.pathIsNotChild(resolved.cleanPath, self.contextDir.cleanPath) - } - let transfer = try path.buildTransfer(id: packet.id, contextDir: self.contextDir, complete: true) + + let transfer = BuildTransfer(id: packet.id, source: relative.string, status: status, complete: true) var response = ClientStream() response.buildID = buildID response.buildTransfer = transfer @@ -163,6 +177,43 @@ actor BuildFSSync: BuildPipelineHandler { sender.yield(response) } + /// Converts a request `source` into a path relative to the context root. + /// + /// Absolute sources inside the context are made relative to it; other + /// absolute sources are taken to be relative to the context root, as the + /// shim sometimes sends them. `..` is collapsed lexically and a result + /// that leaves the context is rejected. This is only normalisation: the + /// real confinement is done by the descriptor walk. + private func relativePath(for source: String) throws -> FilePath { + if source.hasPrefix("/") { + let url = URL(fileURLWithPath: source).standardizedFileURL + if let relative = try? url.relativeChildPath(to: self.contextDir) { + return FilePath(relative) + } + let fallback = self.contextDir.appending(components: source.cleanPathComponent).standardizedFileURL + return FilePath(try fallback.relativeChildPath(to: self.contextDir)) + } + let url = self.contextDir.appendingPathComponent(source).standardizedFileURL + return FilePath(try url.relativeChildPath(to: self.contextDir)) + } + + /// Metadata of the entry at `relative` without dereferencing it. + private func entryStatus(_ relative: FilePath) throws -> FileDescriptorOps.FileStatus { + guard let status = try FileDescriptorOps.status(self.contextFD, relativePath: relative, symlinks: .followBeneath) else { + throw FileDescriptorOps.Error.notFound + } + return status + } + + /// Runs `body`, reporting a refused symlink or path as ``Error/pathIsNotChild``. + private func confined(_ source: String, _ body: () throws -> T) throws -> T { + do { + return try body() + } catch FileDescriptorOps.Error.cannotFollowSymlink, FileDescriptorOps.Error.invalidRelativePath { + throw Error.pathIsNotChild(source, self.contextDir.cleanPath) + } + } + private struct DirEntry: Hashable { let url: URL let isDirectory: Bool @@ -497,6 +548,27 @@ extension BuildTransfer { } } +extension BuildTransfer { + fileprivate init(id: String, source: String, status: FileDescriptorOps.FileStatus, complete: Bool, data: Data = Data()) { + self.init( + id: id, + source: source, + complete: complete, + isDir: status.type == .directory, + metadata: [ + "os": "linux", + "stage": "fssync", + "mode": String(status.permissions.rawValue), + "size": String(status.size), + "modified_at": Date(timeIntervalSince1970: TimeInterval(status.modificationSeconds)).rfc3339(), + "uid": String(status.userID), + "gid": String(status.groupID), + ], + data: data + ) + } +} + extension URL { fileprivate func size() throws -> UInt64 { let attrs = try FileManager.default.attributesOfItem(atPath: self.cleanPath) @@ -514,14 +586,6 @@ extension URL { throw BuildFSSync.Error.couldNotDetermineModTime(self.cleanPath) } - fileprivate func isDir() throws -> Bool { - let attrs = try FileManager.default.attributesOfItem(atPath: self.cleanPath) - guard let t = attrs[.type] as? FileAttributeType, t == .typeDirectory else { - return false - } - return true - } - fileprivate func mode() throws -> UInt32 { let attrs = try FileManager.default.attributesOfItem(atPath: self.cleanPath) if let mode = attrs[FileAttributeKey.posixPermissions] as? NSNumber { @@ -529,50 +593,6 @@ extension URL { } throw BuildFSSync.Error.couldNotDetermineFileMode(self.cleanPath) } - - fileprivate func uid() throws -> UInt32 { - let attrs = try FileManager.default.attributesOfItem(atPath: self.cleanPath) - if let uid = attrs[.ownerAccountID] as? UInt32 { - return uid - } - throw BuildFSSync.Error.couldNotDetermineUID(self.cleanPath) - } - - fileprivate func gid() throws -> UInt32 { - let attrs = try FileManager.default.attributesOfItem(atPath: self.cleanPath) - if let gid = attrs[.groupOwnerAccountID] as? UInt32 { - return gid - } - throw BuildFSSync.Error.couldNotDetermineGID(self.cleanPath) - } - - fileprivate func buildTransfer( - id: String, - contextDir: URL? = nil, - complete: Bool = false, - data: Data = Data() - ) throws -> BuildTransfer { - let p = try { - if let contextDir { return try self.relativeChildPath(to: contextDir) } - return self.cleanPath - }() - return BuildTransfer( - id: id, - source: String(p), - complete: complete, - isDir: try self.isDir(), - metadata: [ - "os": "linux", - "stage": "fssync", - "mode": String(try self.mode()), - "size": String(try self.size()), - "modified_at": try self.modTime(), - "uid": String(try self.uid()), - "gid": String(try self.gid()), - ], - data: data - ) - } } extension Date { diff --git a/Tests/ContainerBuildTests/BuildFSSyncTests.swift b/Tests/ContainerBuildTests/BuildFSSyncTests.swift index e48195ff..61e5069f 100644 --- a/Tests/ContainerBuildTests/BuildFSSyncTests.swift +++ b/Tests/ContainerBuildTests/BuildFSSyncTests.swift @@ -146,6 +146,118 @@ import Testing } } + // Symlink following is limited to relative targets beneath the context + // root. A link with an absolute target is rejected even when that target is + // inside the context, because the descriptor walk cannot follow an absolute + // path without leaving the pinned directory. + + @Test func testReadRejectsAbsoluteSymlinkInsideContext() async throws { + let realFile = contextDir.appendingPathComponent("real.txt") + try write("hello", to: realFile) + try fm.createSymbolicLink( + atPath: contextDir.appendingPathComponent("alias").path(percentEncoded: false), + withDestinationPath: realFile.path(percentEncoded: false) + ) + + let fssync = try BuildFSSync(contextDir) + var continuation: AsyncStream.Continuation! + _ = AsyncStream { continuation = $0 } + defer { continuation.finish() } + + do { + try await fssync.read(continuation, readPacket(source: "alias"), "build-0") + Issue.record("read() should throw BuildFSSync.Error for a symlink with an absolute target, even inside the context") + } catch is BuildFSSync.Error { + // expected + } + } + + @Test func testInfoRejectsAbsoluteSymlinkInsideContext() async throws { + let realFile = contextDir.appendingPathComponent("real.txt") + try write("hello", to: realFile) + try fm.createSymbolicLink( + atPath: contextDir.appendingPathComponent("alias").path(percentEncoded: false), + withDestinationPath: realFile.path(percentEncoded: false) + ) + + let fssync = try BuildFSSync(contextDir) + var continuation: AsyncStream.Continuation! + _ = AsyncStream { continuation = $0 } + defer { continuation.finish() } + + do { + try await fssync.info(continuation, readPacket(source: "alias"), "build-0") + Issue.record("info() should throw BuildFSSync.Error for a symlink with an absolute target, even inside the context") + } catch is BuildFSSync.Error { + // expected + } + } + + // MARK: - read(): served content + + private func readResponse(_ fssync: BuildFSSync, source: String) async throws -> BuildTransfer { + var continuation: AsyncStream.Continuation! + let stream = AsyncStream { continuation = $0 } + defer { continuation.finish() } + try await fssync.read(continuation, readPacket(source: source), "build-0") + continuation.finish() + for await packet in stream { return packet.buildTransfer } + throw BuildFSSync.Error.buildTransferMissing + } + + @Test func testReadServesRegularFileAndRelativeInContextSymlink() async throws { + try write("hello", to: contextDir.appendingPathComponent("plain.txt")) + try fm.createSymbolicLink( + atPath: contextDir.appendingPathComponent("alias").path(percentEncoded: false), + withDestinationPath: "plain.txt") + + let fssync = try BuildFSSync(contextDir) + for source in ["plain.txt", "alias", "./plain.txt", contextDir.appendingPathComponent("plain.txt").path(percentEncoded: false)] { + let transfer = try await readResponse(fssync, source: source) + #expect(String(data: transfer.data, encoding: .utf8) == "hello", "source: \(source)") + } + } + + @Test func testReadOfDirectoryReturnsNoData() async throws { + try fm.createDirectory(at: contextDir.appendingPathComponent("sub"), withIntermediateDirectories: true) + let transfer = try await readResponse(try BuildFSSync(contextDir), source: "sub") + #expect(transfer.data.isEmpty) + #expect(transfer.isDirectory) + } + + /// The scenario from GHSA-fqxg-rrf3-358v: a link is swapped between an + /// in-context and an out-of-context target while reads are in flight. Every + /// read must either return the in-context content or fail; the secret must + /// never be served. + @Test func testReadNeverServesSwappedSymlinkTarget() async throws { + try write("safe", to: contextDir.appendingPathComponent("safe.txt")) + try write("supersecret", to: outsideDir.appendingPathComponent("secret.txt")) + let bait = contextDir.appendingPathComponent("bait").path(percentEncoded: false) + let tmp = contextDir.appendingPathComponent("bait.tmp").path(percentEncoded: false) + let targets = ["safe.txt", outsideDir.appendingPathComponent("secret.txt").path(percentEncoded: false)] + try fm.createSymbolicLink(atPath: bait, withDestinationPath: targets[0]) + + let stop = ManagedAtomicFlag() + let swapper = Thread { + var i = 0 + while !stop.isSet { + unlink(tmp) + symlink(targets[i % 2], tmp) + rename(tmp, bait) + i += 1 + } + } + swapper.start() + defer { stop.set() } + + let fssync = try BuildFSSync(contextDir) + for _ in 0..<2000 { + if let transfer = try? await readResponse(fssync, source: "bait") { + #expect(String(data: transfer.data, encoding: .utf8) != "supersecret") + } + } + } + // MARK: - info(): symlink boundary enforcement // // info() is the metadata-only half of the shim's FS.Open() fallback path. @@ -423,3 +535,10 @@ import Testing #expect(secretLeak == nil, "no entry for the external file should appear in walk() results: \(infos.map { $0.name })") } } + +private final class ManagedAtomicFlag: @unchecked Sendable { + private let lock = NSLock() + private var value = false + var isSet: Bool { lock.withLock { value } } + func set() { lock.withLock { value = true } } +}