From 48145ac7fb177d9fb14e015a0bdea4c642b36729 Mon Sep 17 00:00:00 2001 From: Kathryn Baldauf Date: Tue, 28 Jul 2026 10:13:10 -0700 Subject: [PATCH] Fix image env vars, build context checks, TCP/UDP port forward buffer, and validate plugin name (#2027) Signed-off-by: Kathryn Baldauf Co-authored-by: John Logan Co-authored-by: Raj Aryan Singh --- Package.resolved | 2 +- Package.swift | 1 + Sources/ContainerBuild/BuildFSSync.swift | 85 +++- .../ContainerBuild/BuildImageResolver.swift | 7 + .../ContainerBuild/BuildPipelineHandler.swift | 50 +++ .../BuildRemoteContentProxy.swift | 5 + Sources/ContainerBuild/BuildStdio.swift | 5 + Sources/ContainerBuild/Globber.swift | 83 ++-- Sources/ContainerBuild/URL+Extensions.swift | 19 +- Sources/ContainerPlugin/PluginFactory.swift | 17 +- .../ContainerAPIService/Client/Parser.swift | 4 +- Sources/SocketForwarder/ConnectHandler.swift | 36 +- Sources/SocketForwarder/TCPForwarder.swift | 9 +- Sources/SocketForwarder/UDPForwarder.swift | 10 +- .../ContainerAPIClientTests/ParserTest.swift | 12 + .../BuildFSSyncTests.swift | 397 ++++++++++++++++++ Tests/ContainerBuildTests/GlobberTests.swift | 116 ++++- .../PluginFactoryTest.swift | 75 ++++ .../Build/TestCLIBuilder.swift | 30 ++ 19 files changed, 882 insertions(+), 81 deletions(-) create mode 100644 Tests/ContainerBuildTests/BuildFSSyncTests.swift diff --git a/Package.resolved b/Package.resolved index eb751d5a..b636b3b1 100644 --- a/Package.resolved +++ b/Package.resolved @@ -1,5 +1,5 @@ { - "originHash" : "8277777f28690a67b811704b389423d888c06888b2f727cda0d627dab2202c99", + "originHash" : "17a1adf6ab79fe79a1a88954948cebaf96738c8c0b96c822dec9fba1b820453d", "pins" : [ { "identity" : "async-http-client", diff --git a/Package.swift b/Package.swift index f100555f..9a1cf821 100644 --- a/Package.swift +++ b/Package.swift @@ -143,6 +143,7 @@ let package = Package( .product(name: "Containerization", package: "containerization"), .product(name: "ContainerizationArchive", package: "containerization"), .product(name: "ContainerizationOCI", package: "containerization"), + .product(name: "ContainerizationOS", package: "containerization"), .product(name: "ArgumentParser", package: "swift-argument-parser"), .product(name: "GRPCCore", package: "grpc-swift-2"), .product(name: "GRPCNIOTransportHTTP2", package: "grpc-swift-nio-transport"), diff --git a/Sources/ContainerBuild/BuildFSSync.swift b/Sources/ContainerBuild/BuildFSSync.swift index 7b4fc440..c5a5288f 100644 --- a/Sources/ContainerBuild/BuildFSSync.swift +++ b/Sources/ContainerBuild/BuildFSSync.swift @@ -22,18 +22,49 @@ import CryptoKit import Foundation import GRPCCore +/// Handles the `fssync` stage of the build protocol. +/// +/// When BuildKit needs build-context files it sends `Walk`, `Read`, and `Info` +/// requests to the shim, which proxies them over the gRPC stream to this actor. +/// +/// ## Primary path: Walk (tar mode) +/// +/// `Walk` is the primary data path. The host packs all requested context paths +/// into a tar archive and streams it to the shim. The shim unpacks the tar to a +/// local cache and presents the files to BuildKit via `DiffCopy`. BuildKit then +/// issues `PACKET_REQ` for regular files it needs; the shim serves those from +/// the local cache without any further calls to the host. +/// +/// When a context path is a symlink whose target lies within the context root, +/// ``walk(_:_:_:)`` adds the target to the archive alongside the symlink so +/// BuildKit can dereference it during `COPY`/`ADD` processing. +/// +/// ## Fallback path: Info + Read +/// +/// `FS.Open()` in the shim falls back to `Info` followed by `Read` calls when +/// its local checksum cache is unpopulated (a narrow race window at the start of +/// a build). These paths are not exercised during a normal build. +/// +/// ## Symlink safety +/// +/// 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. +/// Dockerignore filtering is **not** applied here; the shim applies it after +/// unpacking the tar. actor BuildFSSync: BuildPipelineHandler { let contextDir: URL init(_ contextDir: URL) throws { + let resolved = contextDir.resolvingSymlinksInPath() guard FileManager.default.fileExists(atPath: contextDir.cleanPath) else { throw Error.contextNotFound(contextDir.cleanPath) } - guard try contextDir.isDir() else { + guard resolved.isDirectory else { throw Error.contextIsNotDirectory(contextDir.cleanPath) } - self.contextDir = contextDir + self.contextDir = resolved } nonisolated func accept(_ packet: ServerStream) throws -> Bool { @@ -63,6 +94,11 @@ 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. func read(_ sender: AsyncStream.Continuation, _ packet: BuildTransfer, _ buildID: String) async throws { let offset: UInt64 = packet.offset() ?? 0 let size: Int = packet.len() ?? 0 @@ -79,6 +115,10 @@ actor BuildFSSync: BuildPipelineHandler { 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() @@ -95,6 +135,12 @@ actor BuildFSSync: BuildPipelineHandler { sender.yield(response) } + /// Returns metadata (mode, size, modification time, uid/gid) for a single + /// 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(_:_:_:)``. func info(_ sender: AsyncStream.Continuation, _ packet: BuildTransfer, _ buildID: String) async throws { let path: URL if packet.source.hasPrefix("/") { @@ -105,6 +151,10 @@ actor BuildFSSync: BuildPipelineHandler { .appendingPathComponent(packet.source) .standardizedFileURL } + 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) var response = ClientStream() response.buildID = buildID @@ -127,6 +177,23 @@ actor BuildFSSync: BuildPipelineHandler { } } + /// Packs requested context paths into a tar archive and streams it to the shim. + /// + /// This is the primary data path for build-context transfer. BuildKit sends + /// a `Walk` request whose `followpaths` field names the context paths needed + /// for the current build step (e.g. the source of a `COPY` instruction). + /// The host resolves those globs, builds an entry set, and passes it to + /// `Archiver.compress` to produce the tar. + /// + /// For any symlink in the entry set whose target lies within the context + /// root, the target is added to the entry set so BuildKit can dereference + /// the symlink during `COPY`/`ADD` processing without a separate request. + /// Symlinks whose targets lie outside the context root are included as + /// symlink entries but their targets are not; BuildKit will resolve them + /// against the shim's local filesystem on Linux, not the macOS host. + /// + /// Dockerignore filtering is the shim's responsibility and is applied after + /// the tar is unpacked; this method has no knowledge of `.dockerignore`. func walk( _ sender: AsyncStream.Continuation, _ packet: BuildTransfer, @@ -334,16 +401,10 @@ actor BuildFSSync: BuildPipelineHandler { let target: String init(path: URL, contextDir: URL) throws { - if path.isSymlink { - let target: URL = path.resolvingSymlinksInPath() - if contextDir.parentOf(target) { - self.target = target.relativePathFrom(from: path) - } else { - self.target = target.cleanPath - } - } else { - self.target = "" - } + // Always report the literal, unresolved on-disk symlink target — + // the same value tar mode provides via Archiver's use of + // destinationOfSymbolicLink — rather than a host-resolved path. + self.target = path.isSymlink ? try FileManager.default.destinationOfSymbolicLink(atPath: path.cleanPath) : "" self.name = try path.relativeChildPath(to: contextDir) self.modTime = try path.modTime() diff --git a/Sources/ContainerBuild/BuildImageResolver.swift b/Sources/ContainerBuild/BuildImageResolver.swift index 77f5b49e..5b15352d 100644 --- a/Sources/ContainerBuild/BuildImageResolver.swift +++ b/Sources/ContainerBuild/BuildImageResolver.swift @@ -23,6 +23,13 @@ import GRPCCore import Logging import TerminalProgress +/// Handles the `resolver` stage of the build protocol. +/// +/// Resolves image references on behalf of BuildKit: authenticates with +/// registries, pulls missing base-image manifests and layers, and stores +/// them in the local content store. BuildKit delegates these operations to +/// the host because registry credentials and network access live on the +/// macOS side, not inside the builder VM. struct BuildImageResolver: BuildPipelineHandler { let contentStore: ContentStore let quiet: Bool diff --git a/Sources/ContainerBuild/BuildPipelineHandler.swift b/Sources/ContainerBuild/BuildPipelineHandler.swift index 6ee36e8f..da0b0081 100644 --- a/Sources/ContainerBuild/BuildPipelineHandler.swift +++ b/Sources/ContainerBuild/BuildPipelineHandler.swift @@ -18,11 +18,61 @@ import Foundation import GRPCCore import NIO +/// A handler for one stage of the build protocol. +/// +/// The build pipeline multiplexes a single bidirectional gRPC stream between +/// the macOS host and the builder shim. Each packet carries a stage tag; +/// a handler claims packets for its stage via ``accept(_:)`` and processes +/// them via ``handle(_:_:)``. protocol BuildPipelineHandler: Sendable { func accept(_ packet: ServerStream) throws -> Bool func handle(_ sender: AsyncStream.Continuation, _ packet: ServerStream) async throws } +/// Drives a build session by routing packets from the builder shim to the +/// appropriate handler. +/// +/// ## Three-tier architecture +/// +/// Builds involve three components with distinct responsibilities: +/// +/// **macOS host (`BuildPipeline` / its handlers)** +/// Serves resources to the builder shim over a bidirectional gRPC stream. +/// Responsibilities include: +/// - Packing requested build-context files into a tar archive (``BuildFSSync``). +/// - Proxying image-layer blobs from the local content store (``BuildRemoteContentProxy``). +/// - Resolving and pulling base images (``BuildImageResolver``). +/// - Relaying builder stdout/stderr to the terminal (``BuildStdio``). +/// - Enforcing the context root boundary: directory traversal uses `openat(O_NOFOLLOW)` +/// at every descent step, and every individual file request resolves symlinks to their +/// canonical path before verifying containment within the context root. +/// +/// **Builder shim (`container-builder-shim`)** +/// A Go process running inside a Linux VM that bridges the host gRPC stream +/// and BuildKit's `filesync` gRPC interface. Responsibilities include: +/// - Receiving the context tar from the host, unpacking it to a local cache, +/// and presenting the result to BuildKit via `DiffCopy`. +/// - Applying dockerignore exclusions (received from BuildKit as +/// `exclude-patterns` metadata) when walking the unpacked cache. +/// - Passing `followpaths` from BuildKit to the host so the host knows which +/// context paths to include in the tar. +/// +/// **BuildKit** +/// Parses and executes the Dockerfile. Responsibilities include: +/// - Sending `Walk` requests with `followpaths` derived from each `COPY`/`ADD` +/// source and `exclude-patterns` derived from `.dockerignore`. +/// - Dereferencing symlinks, recursing into directories, and applying all +/// other COPY/ADD transfer semantics on the unpacked context the shim provides. +/// +/// ## Packet flow +/// +/// ``` +/// BuildKit ──► shim DiffCopy ──► host Walk (tar of context files) +/// ◄── tar archive +/// ◄── PACKET_STAT per file (after shim unpacks + filters) +/// ──► PACKET_REQ for each regular file +/// ◄── PACKET_DATA (shim reads from local unpacked cache) +/// ``` public actor BuildPipeline { let handlers: [BuildPipelineHandler] public init(_ config: Builder.BuildConfig) async throws { diff --git a/Sources/ContainerBuild/BuildRemoteContentProxy.swift b/Sources/ContainerBuild/BuildRemoteContentProxy.swift index e6cb1cbc..afc1e270 100644 --- a/Sources/ContainerBuild/BuildRemoteContentProxy.swift +++ b/Sources/ContainerBuild/BuildRemoteContentProxy.swift @@ -21,6 +21,11 @@ import ContainerizationOCI import Foundation import GRPCCore +/// Handles the `content-store` stage of the build protocol. +/// +/// Proxies image-layer blob requests from BuildKit to the host's local +/// containerd content store. BuildKit issues these requests when it needs +/// base-image layers that are not already present in the builder VM. struct BuildRemoteContentProxy: BuildPipelineHandler { let local: ContentStore diff --git a/Sources/ContainerBuild/BuildStdio.swift b/Sources/ContainerBuild/BuildStdio.swift index 32429480..47b2201b 100644 --- a/Sources/ContainerBuild/BuildStdio.swift +++ b/Sources/ContainerBuild/BuildStdio.swift @@ -19,6 +19,11 @@ import Foundation import GRPCCore import NIO +/// Handles the stdio stage of the build protocol. +/// +/// Relays builder stdout/stderr from the shim to the client terminal. +/// Build output (layer download progress, `RUN` command output, etc.) flows +/// through this handler and is written directly to the configured file handle. actor BuildStdio: BuildPipelineHandler { public let quiet: Bool public let handle: FileHandle diff --git a/Sources/ContainerBuild/Globber.swift b/Sources/ContainerBuild/Globber.swift index baecf60d..9b3011b3 100644 --- a/Sources/ContainerBuild/Globber.swift +++ b/Sources/ContainerBuild/Globber.swift @@ -14,7 +14,9 @@ // limitations under the License. //===----------------------------------------------------------------------===// +import ContainerizationOS import Foundation +import SystemPackage public class Globber { let input: URL @@ -33,7 +35,7 @@ public class Globber { .replacingOccurrences(of: "[/]\\*{2,}([^/])", with: "/**/*$1", options: .regularExpression) .replacingOccurrences(of: "^\\*{2,}([^/])", with: "**/*$1", options: .regularExpression) - for child in input.children { + for child in self.children(of: input) { try self.match(input: child, components: adjustedPattern.split(separator: "/").map(String.init)) } } @@ -47,7 +49,7 @@ public class Globber { guard dir.pathComponents.count > 1 else { break } dir.deleteLastPathComponent() } - return input.childrenRecursive.forEach { results.insert($0) } + return self.childrenRecursive(of: input).forEach { results.insert($0) } } let head = components.first ?? "" @@ -59,7 +61,7 @@ public class Globber { tail = tail.tail } try self.match(input: input, components: tail) - for child in input.children { + for child in self.children(of: input) { try self.match(input: child, components: components) } return @@ -68,13 +70,66 @@ public class Globber { if try glob(input.lastPathComponent, head) { try self.match(input: input, components: tail) - for child in input.children where try glob(child.lastPathComponent, tail.first ?? "") { + for child in self.children(of: input) where try glob(child.lastPathComponent, tail.first ?? "") { try self.match(input: child, components: tail) } return } } + /// Returns the direct children of `url`, following `url` itself when it is + /// a directory symlink whose fully-resolved target stays within the match + /// root. A symlink that escapes the root is treated as having no children + /// (same as a regular file) so pattern components after it never match — + /// mirrors the containment check `BuildFSSync` applies before reading. + /// + /// Children are named by their resolved (physical) path, not by `url`, so + /// that `walk(root:includePatterns:)`'s later filter — which is driven by + /// `Archiver.compress`'s own physical directory walk — reliably finds a + /// matching entry regardless of whether that walk itself follows `url`'s + /// symlink. `url` is separately inserted into `results` so the symlink + /// entry is still present in the tar for the builder to resolve the + /// original path against. + private func children(of url: URL) -> [URL] { + // TODO: modifying object state and returning results is odd, rework + guard let dir = self.resolvedDirectory(of: url) else { return [] } + if url.isSymlink { self.results.insert(url) } + return (try? FileManager.default.contentsOfDirectory(at: dir, includingPropertiesForKeys: nil)) + ?? [] + } + + /// Recursive form of ``children(of:)``, used once a full pattern (or `**`) + /// has matched `url` and every descendant needs to be collected. Nested + /// directory symlinks are resolved and boundary-checked the same way, one + /// level at a time, via ``FileDescriptorOps/enumerate`` which never follows + /// symlinks it encounters mid-traversal — only the top-level `url` passed + /// in here gets the resolve-and-check treatment. + private func childrenRecursive(of url: URL) -> [URL] { + guard let dir = self.resolvedDirectory(of: url) else { return [url] } + if url.isSymlink { self.results.insert(url) } + guard let fd = try? FileDescriptor.open(FilePath(dir.path), .readOnly, options: .directory) else { + return [dir] + } + defer { try? fd.close() } + var found: [URL] = [dir] + try? FileDescriptorOps.enumerate(fd) { relPath, _, _ in + found.append(dir.appendingPathComponent(relPath.string)) + } + return found + } + + /// Resolves `url` to the real directory whose contents should be listed in + /// its place. Non-symlinks resolve to themselves. A directory symlink + /// resolves to its target only if the fully-resolved target is still + /// within `self.input` (the match root); otherwise `nil`, so callers treat + /// it as a leaf rather than descending outside the context. + private func resolvedDirectory(of url: URL) -> URL? { + guard url.isSymlink else { return url } + let resolved = url.resolvingSymlinksInPath() + guard resolved.isDirectory, self.input.parentOf(resolved) else { return nil } + return resolved + } + func glob(_ input: String, _ pattern: String) throws -> Bool { let regexPattern = "^" @@ -91,26 +146,6 @@ public class Globber { } } -extension URL { - var children: [URL] { - - (try? FileManager.default.contentsOfDirectory(at: self, includingPropertiesForKeys: nil)) - ?? [] - } - - var childrenRecursive: [URL] { - var results: [URL] = [] - if let enumerator = FileManager.default.enumerator( - at: self, includingPropertiesForKeys: [.isDirectoryKey, .isSymbolicLinkKey]) - { - while let child = enumerator.nextObject() as? URL { - results.append(child) - } - } - return [self] + results - } -} - extension [String] { var tail: [String] { if self.count <= 1 { diff --git a/Sources/ContainerBuild/URL+Extensions.swift b/Sources/ContainerBuild/URL+Extensions.swift index 4f0bd7ec..81818b20 100644 --- a/Sources/ContainerBuild/URL+Extensions.swift +++ b/Sources/ContainerBuild/URL+Extensions.swift @@ -54,6 +54,13 @@ extension URL { self.path.fs_cleaned } + /// Returns true if `url` is lexically a descendant of `self`. + /// + /// This is a **string-prefix check** on the normalised path components; it + /// does not call `realpath` or resolve symlinks. A URL that is lexically + /// inside the context root but reachable through an intermediate symlink + /// that points outside it will still pass this check. Callers that need + /// physical containment must resolve symlinks before calling this method. func parentOf(_ url: URL) -> Bool { let parentPath = self.absoluteURL.cleanPath let childPath = url.absoluteURL.cleanPath @@ -80,18 +87,6 @@ extension URL { return selfParts.dropFirst(ctxParts.count).joined(separator: "/") } - func relativePathFrom(from base: URL) -> String { - let destParts = cleanPath.fs_components - let baseParts = base.cleanPath.fs_components - - let common = zip(destParts, baseParts).prefix { $0 == $1 }.count - guard common > 0 else { return cleanPath } - - let ups = Array(repeating: "..", count: baseParts.count - common) - let remainder = destParts.dropFirst(common) - return (ups + remainder).joined(separator: "/") - } - func zeroCopyReader( chunk: Int = 1024 * 1024, buffer: AsyncStream.Continuation.BufferingPolicy = .unbounded diff --git a/Sources/ContainerPlugin/PluginFactory.swift b/Sources/ContainerPlugin/PluginFactory.swift index 2a9998b6..04d756f0 100644 --- a/Sources/ContainerPlugin/PluginFactory.swift +++ b/Sources/ContainerPlugin/PluginFactory.swift @@ -14,8 +14,10 @@ // limitations under the License. //===----------------------------------------------------------------------===// +import ContainerizationError import Foundation import Logging +import SystemPackage /// Describes the configuration and binary file locations for a plugin. public protocol PluginFactory: Sendable { @@ -25,6 +27,15 @@ public protocol PluginFactory: Sendable { func create(parentURL: URL, name: String) throws -> Plugin? } +/// Requires `name` to be a single regular path component that round-trips +/// exactly, so it can't escape the plugin directory via `/`, `..`, or a NUL +/// that `FilePath.Component` would silently truncate. +private func validatePluginName(_ name: String) throws { + guard let component = FilePath.Component(name), component.kind == .regular, component.string == name else { + throw ContainerizationError(.invalidArgument, message: "invalid plugin name \(name)") + } +} + /// Default layout which uses a Unix-like structure. public struct DefaultPluginFactory: PluginFactory { // Order matters: earlier entries take priority during config file discovery. @@ -78,7 +89,8 @@ public struct DefaultPluginFactory: PluginFactory { } public func create(parentURL: URL, name: String) throws -> Plugin? { - try create(installURL: parentURL.appendingPathComponent(name)) + try validatePluginName(name) + return try create(installURL: parentURL.appendingPathComponent(name)) } } @@ -130,6 +142,7 @@ public struct AppBundlePluginFactory: PluginFactory { } public func create(parentURL: URL, name: String) throws -> Plugin? { - try create(installURL: parentURL.appendingPathComponent("\(name)\(Self.appSuffix)")) + try validatePluginName(name) + return try create(installURL: parentURL.appendingPathComponent("\(name)\(Self.appSuffix)")) } } diff --git a/Sources/Services/ContainerAPIService/Client/Parser.swift b/Sources/Services/ContainerAPIService/Client/Parser.swift index ef209df5..e4516d7f 100644 --- a/Sources/Services/ContainerAPIService/Client/Parser.swift +++ b/Sources/Services/ContainerAPIService/Client/Parser.swift @@ -125,7 +125,9 @@ public struct Parser { public static func allEnv(imageEnvs: [String], envFiles: [String], envs: [String]) throws -> [String] { var combined: [String] = [] - combined.append(contentsOf: Parser.env(envList: imageEnvs)) + // Image config is untrusted. Bare env var names here must not be expanded from the host + // process's environment. + combined.append(contentsOf: imageEnvs.filter { $0.contains("=") }) for envFile in envFiles { let content = try Parser.envFile(path: envFile) combined.append(contentsOf: content) diff --git a/Sources/SocketForwarder/ConnectHandler.swift b/Sources/SocketForwarder/ConnectHandler.swift index 5f98e805..c55e3a53 100644 --- a/Sources/SocketForwarder/ConnectHandler.swift +++ b/Sources/SocketForwarder/ConnectHandler.swift @@ -19,13 +19,13 @@ import NIOCore import NIOPosix final class ConnectHandler { - private var pendingBytes: [NIOAny] private let serverAddress: SocketAddress + private let connectTimeout: TimeAmount private var log: Logger? = nil - init(serverAddress: SocketAddress, log: Logger?) { - self.pendingBytes = [] + init(serverAddress: SocketAddress, connectTimeout: TimeAmount, log: Logger?) { self.serverAddress = serverAddress + self.connectTimeout = connectTimeout self.log = log } } @@ -34,10 +34,6 @@ extension ConnectHandler: ChannelInboundHandler { typealias InboundIn = ByteBuffer typealias OutboundOut = ByteBuffer - func channelRead(context: ChannelHandlerContext, data: NIOAny) { - self.pendingBytes.append(data) - } - func handlerAdded(context: ChannelHandlerContext) { // Add logger metadata. self.log?[metadataKey: "proxy"] = "\(context.channel.localAddress?.description ?? "none")" @@ -51,31 +47,14 @@ extension ConnectHandler: ChannelInboundHandler { } } -extension ConnectHandler: RemovableChannelHandler { - func removeHandler(context: ChannelHandlerContext, removalToken: ChannelHandlerContext.RemovalToken) { - var didRead = false - - // We are being removed, and need to deliver any pending bytes we may have if we're upgrading. - while self.pendingBytes.count > 0 { - let data = self.pendingBytes.removeFirst() - context.fireChannelRead(data) - didRead = true - } - - if didRead { - context.fireChannelReadComplete() - } - - self.log?.trace("backend - removing connect handler from pipeline") - context.leavePipeline(removalToken: removalToken) - } -} +extension ConnectHandler: RemovableChannelHandler {} extension ConnectHandler { private func connectToServer(context: ChannelHandlerContext) { self.log?.trace("backend - connecting") ClientBootstrap(group: context.eventLoop) + .connectTimeout(self.connectTimeout) .connect(to: serverAddress) .assumeIsolatedUnsafeUnchecked() .whenComplete { result in @@ -105,6 +84,11 @@ extension ConnectHandler { try context.channel.pipeline.syncOperations.addHandler(localGlue) try peerChannel.pipeline.syncOperations.addHandler(peerGlue) context.pipeline.syncOperations.removeHandler(self, promise: nil) + + // Reads were paused on the frontend channel while we waited for the backend to + // connect. Resume both sides now that GlueHandler owns steady-state flow control. + try context.channel.syncOptions?.setOption(ChannelOptions.autoRead, value: true) + try peerChannel.syncOptions?.setOption(ChannelOptions.autoRead, value: true) } catch { // Close connected peer channel before closing our channel. peerChannel.close(mode: .all, promise: nil) diff --git a/Sources/SocketForwarder/TCPForwarder.swift b/Sources/SocketForwarder/TCPForwarder.swift index e5103360..0f616419 100644 --- a/Sources/SocketForwarder/TCPForwarder.swift +++ b/Sources/SocketForwarder/TCPForwarder.swift @@ -26,17 +26,21 @@ public struct TCPForwarder: SocketForwarder { private let eventLoopGroup: any EventLoopGroup + private let connectTimeout: TimeAmount + private let log: Logger? public init( proxyAddress: SocketAddress, serverAddress: SocketAddress, eventLoopGroup: any EventLoopGroup, + connectTimeout: TimeAmount = .seconds(10), log: Logger? = nil ) throws { self.proxyAddress = proxyAddress self.serverAddress = serverAddress self.eventLoopGroup = eventLoopGroup + self.connectTimeout = connectTimeout self.log = log } @@ -46,10 +50,13 @@ public struct TCPForwarder: SocketForwarder { let bootstrap = ServerBootstrap(group: self.eventLoopGroup) .serverChannelOption(ChannelOptions.socket(.init(SOL_SOCKET), .init(SO_REUSEADDR)), value: 1) .childChannelOption(ChannelOptions.socket(.init(SOL_SOCKET), .init(SO_REUSEADDR)), value: 1) + // Reads are paused until the backend connects; the client's bytes are held in the + // kernel receive buffer instead of an app-level buffer while we wait. + .childChannelOption(ChannelOptions.autoRead, value: false) .childChannelInitializer { channel in channel.eventLoop.makeCompletedFuture { try channel.pipeline.syncOperations.addHandler( - ConnectHandler(serverAddress: self.serverAddress, log: log) + ConnectHandler(serverAddress: self.serverAddress, connectTimeout: self.connectTimeout, log: log) ) } } diff --git a/Sources/SocketForwarder/UDPForwarder.swift b/Sources/SocketForwarder/UDPForwarder.swift index 54d472e1..031e8dc1 100644 --- a/Sources/SocketForwarder/UDPForwarder.swift +++ b/Sources/SocketForwarder/UDPForwarder.swift @@ -26,6 +26,12 @@ private final class UDPProxyBackend: ChannelInboundHandler { typealias InboundIn = AddressedEnvelope typealias OutboundOut = AddressedEnvelope + // Datagrams sent by the client before the outbound backend channel finishes binding are + // queued here. The window is normally tiny (a local bind, not a network round-trip), so this + // cap is a safety valve, not the primary defense. UDP has no delivery guarantee, so dropping + // the newest datagram once full is an acceptable, protocol-consistent fallback. + private static let maxQueuedPayloads = 8 + private struct State { var queuedPayloads: Deque var channel: (any Channel)? @@ -72,10 +78,12 @@ private final class UDPProxyBackend: ChannelInboundHandler { self.log?.trace("backend - writing datagram to server") let outbound: UDPProxyBackend.OutboundOut = OutboundOut(remoteAddress: self.serverAddress, data: data) channel.writeAndFlush(outbound, promise: nil) - } else { + } else if state.queuedPayloads.count < Self.maxQueuedPayloads { // channel is initializing, queue self.log?.trace("backend - queuing datagram") state.queuedPayloads.append(data) + } else { + self.log?.trace("backend - queue full, dropping datagram") } } diff --git a/Tests/ContainerAPIClientTests/ParserTest.swift b/Tests/ContainerAPIClientTests/ParserTest.swift index 0dcc6f7c..3e39698b 100644 --- a/Tests/ContainerAPIClientTests/ParserTest.swift +++ b/Tests/ContainerAPIClientTests/ParserTest.swift @@ -633,6 +633,18 @@ struct ParserTest { #expect(Set(result) == Set(["FOO=fromuser", "BAR=fromimage", "BAZ=fromfile"])) } + @Test + func testAllEnvRejectsBareNameFromImage() throws { + // Image config is untrusted: a bare name (no "=") must be dropped rather + // than expanded from the host process's environment. + let result = try Parser.allEnv( + imageEnvs: ["PATH", "FOO=fromimage"], + envFiles: [], + envs: [] + ) + #expect(Set(result) == Set(["FOO=fromimage"])) + } + private func tmpFileWithContent(_ content: String) throws -> URL { let tempDir = FileManager.default.temporaryDirectory let tempFile = tempDir.appendingPathComponent("envfile-test-\(UUID().uuidString)") diff --git a/Tests/ContainerBuildTests/BuildFSSyncTests.swift b/Tests/ContainerBuildTests/BuildFSSyncTests.swift new file mode 100644 index 00000000..5d6d607d --- /dev/null +++ b/Tests/ContainerBuildTests/BuildFSSyncTests.swift @@ -0,0 +1,397 @@ +//===----------------------------------------------------------------------===// +// Copyright © 2026 Apple Inc. and the container project authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +//===----------------------------------------------------------------------===// + +import ContainerizationOS +import Foundation +import SystemPackage +import Testing + +@testable import ContainerBuild + +// Tests for BuildFSSync — the handler that serves build-context files to the +// builder over the FSSync protocol. The suite creates real files on disk and +// calls the actor methods directly, bypassing the Walk/cache layer in the shim +// so that the macOS-side boundary enforcement is exercised in isolation. +@Suite class BuildFSSyncTests { + let fm = FileManager.default + let base: URL + let contextDir: URL + let outsideDir: URL + + init() throws { + base = FileManager.default.temporaryDirectory.appendingPathComponent(UUID().uuidString) + contextDir = base.appendingPathComponent("context") + outsideDir = base.appendingPathComponent("outside") + try fm.createDirectory(at: URL(fileURLWithPath: contextDir.path(percentEncoded: false)), withIntermediateDirectories: true) + try fm.createDirectory(at: URL(fileURLWithPath: outsideDir.path(percentEncoded: false)), withIntermediateDirectories: true) + } + + deinit { + try? fm.removeItem(at: base) + } + + // MARK: - Helpers + + private func write(_ content: String, to url: URL) throws { + try content.data(using: .utf8)!.write(to: url) + } + + /// Returns a minimal BuildTransfer packet for the given context-relative source path. + private func readPacket(source: String) -> BuildTransfer { + var p = BuildTransfer() + p.id = UUID().uuidString + p.source = source + return p + } + + // MARK: - read(): symlink boundary enforcement + // + // The tests below call read() directly and expect it to throw when the + // requested path resolves outside the context directory. + // + // Final-component symlink tests (testReadRejectsAbsoluteSymlinkOutsideContext, + // testReadRejectsRelativeSymlinkOutsideContext): the source path is itself a + // symlink that resolves outside the context. + // + // Intermediate-component symlink test (testReadRejectsIntermediateDirectorySymlinkOutsideContext): + // the source path looks like a normal relative path ("subdir/secret.txt"), but + // an intermediate directory component is a symlink that escapes the context. + // read() unconditionally resolves the full path and checks parentOf, so this + // case is caught as well. + + @Test func testReadRejectsAbsoluteSymlinkOutsideContext() async throws { + let secretFile = outsideDir.appendingPathComponent("secret.txt") + try write("supersecret", to: secretFile) + + // Symlink inside context → absolute path outside context. + try fm.createSymbolicLink( + atPath: contextDir.appendingPathComponent("leak").path(percentEncoded: false), + withDestinationPath: secretFile.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: "leak"), "build-0") + Issue.record("read() should throw BuildFSSync.Error for a symlink that resolves outside the context") + } catch is BuildFSSync.Error { + // expected + } + } + + @Test func testReadRejectsRelativeSymlinkOutsideContext() async throws { + let secretFile = outsideDir.appendingPathComponent("secret.txt") + try write("supersecret", to: secretFile) + + // Symlink inside context → relative path that traverses above the context root. + // contextDir = base/context, outsideDir = base/outside, so the relative + // path from base/context/leak to base/outside/secret.txt is ../outside/secret.txt. + try fm.createSymbolicLink( + atPath: contextDir.appendingPathComponent("leak").path(percentEncoded: false), + withDestinationPath: "../outside/secret.txt" + ) + + let fssync = try BuildFSSync(contextDir) + var continuation: AsyncStream.Continuation! + _ = AsyncStream { continuation = $0 } + defer { continuation.finish() } + + do { + try await fssync.read(continuation, readPacket(source: "leak"), "build-0") + Issue.record("read() should throw BuildFSSync.Error for a relative symlink that resolves outside the context") + } catch is BuildFSSync.Error { + // expected + } + } + + @Test func testReadRejectsIntermediateDirectorySymlinkOutsideContext() async throws { + let secretFile = outsideDir.appendingPathComponent("secret.txt") + try write("supersecret", to: secretFile) + + // Directory symlink inside context → directory outside context. + // The requested source "subdir/secret.txt" has a plain file as its final + // component, but read() resolves the full path unconditionally, so the + // intermediate symlink escape is caught by the parentOf check. + try fm.createSymbolicLink( + atPath: contextDir.appendingPathComponent("subdir").path(percentEncoded: false), + withDestinationPath: outsideDir.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: "subdir/secret.txt"), "build-0") + Issue.record("read() should throw BuildFSSync.Error when an intermediate path component is a symlink outside the context") + } catch is BuildFSSync.Error { + // expected + } + } + + // MARK: - info(): symlink boundary enforcement + // + // info() is the metadata-only half of the shim's FS.Open() fallback path. + // It must enforce the same context boundary as read(). All three tests + // below should pass: the fix unconditionally resolves the full path and + // checks parentOf before serving any metadata. + + @Test func testInfoRejectsAbsoluteSymlinkOutsideContext() async throws { + let secretFile = outsideDir.appendingPathComponent("secret.txt") + try write("supersecret", to: secretFile) + + try fm.createSymbolicLink( + atPath: contextDir.appendingPathComponent("leak").path(percentEncoded: false), + withDestinationPath: secretFile.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: "leak"), "build-0") + Issue.record("info() should throw BuildFSSync.Error for a symlink that resolves outside the context") + } catch is BuildFSSync.Error { + // expected + } + } + + @Test func testInfoRejectsRelativeSymlinkOutsideContext() async throws { + let secretFile = outsideDir.appendingPathComponent("secret.txt") + try write("supersecret", to: secretFile) + + try fm.createSymbolicLink( + atPath: contextDir.appendingPathComponent("leak").path(percentEncoded: false), + withDestinationPath: "../outside/secret.txt" + ) + + let fssync = try BuildFSSync(contextDir) + var continuation: AsyncStream.Continuation! + _ = AsyncStream { continuation = $0 } + defer { continuation.finish() } + + do { + try await fssync.info(continuation, readPacket(source: "leak"), "build-0") + Issue.record("info() should throw BuildFSSync.Error for a relative symlink that resolves outside the context") + } catch is BuildFSSync.Error { + // expected + } + } + + @Test func testInfoRejectsIntermediateDirectorySymlinkOutsideContext() async throws { + let secretFile = outsideDir.appendingPathComponent("secret.txt") + try write("supersecret", to: secretFile) + + try fm.createSymbolicLink( + atPath: contextDir.appendingPathComponent("subdir").path(percentEncoded: false), + withDestinationPath: outsideDir.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: "subdir/secret.txt"), "build-0") + Issue.record("info() should throw BuildFSSync.Error when an intermediate path component is a symlink outside the context") + } catch is BuildFSSync.Error { + // expected + } + } + + // MARK: - walk(): directory symlink boundary enforcement + // + // walk() is the primary data path — every build goes through it, and its + // results are what gets packed into the tar sent to the builder. A + // directory symlink inside the context must not let anything physically + // outside the context root end up in those results. The test below + // asserts that directly. + + @Test func testWalkDoesNotFollowDirectorySymlinkOutsideContext() async throws { + let secretFile = outsideDir.appendingPathComponent("secret.txt") + try write("supersecret", to: secretFile) + + // Directory symlink inside context → directory outside context. + try fm.createSymbolicLink( + atPath: contextDir.appendingPathComponent("subdir").path(percentEncoded: false), + withDestinationPath: outsideDir.path(percentEncoded: false) + ) + + let fssync = try BuildFSSync(contextDir) + // Request the symlinked directory. Globber.childrenRecursive follows it + // and currently returns the external file as if it were in the context. + let urls = try await fssync.walk(root: contextDir, includePatterns: ["subdir"]) + + // The directory symlink itself is expected in results (Rule 2: it appears + // as a symlink entry in the tar). What must not appear are non-symlink + // entries — regular files or directories — that physically reside outside + // the context root. + let leaked = urls.filter { url in + guard !url.isSymlink else { return false } + let resolved = url.resolvingSymlinksInPath() + return !self.contextDir.parentOf(resolved) + && resolved.cleanPath != self.contextDir.cleanPath + } + + #expect(leaked.isEmpty, "walk() returned non-symlink URLs that physically resolve outside the context: \(leaked)") + } + + // MARK: - walk(): JSON/FileInfo metadata paths + // + // walk() has two response formats: a tar stream (the primary data path, + // handled above) and a JSON array of FileInfo, used for metadata-only + // walks. FileInfo.target reports the same literal, unresolved on-disk + // symlink destination for every symlink — in-context or not — that tar + // mode already exposes via Archiver's use of destinationOfSymbolicLink. + // It must never be an absolute or canonicalized path derived from + // resolvingSymlinksInPath(), which would disclose a host-resolved path + // for a symlink that escapes the context. + + /// Returns a minimal BuildTransfer packet requesting a JSON-mode walk. + private func walkJSONPacket(followPaths: [String]) -> BuildTransfer { + var p = BuildTransfer() + p.id = UUID().uuidString + p.source = "." + p.metadata = [ + "followpaths": followPaths.joined(separator: ","), + "mode": "json", + ] + return p + } + + /// Drives the actor's real walk(_:_:_:) method in JSON mode and decodes + /// the resulting FileInfo array. + private func walkJSON(_ fssync: BuildFSSync, followPaths: [String] = ["*"]) async throws -> [BuildFSSync.FileInfo] { + var continuation: AsyncStream.Continuation! + let stream = AsyncStream { continuation = $0 } + try await fssync.walk(continuation, walkJSONPacket(followPaths: followPaths), "build-0") + continuation.finish() + + var fileInfos: [BuildFSSync.FileInfo] = [] + for await resp in stream { + let data = resp.buildTransfer.data + if !data.isEmpty { + fileInfos += try JSONDecoder().decode([BuildFSSync.FileInfo].self, from: data) + } + } + return fileInfos + } + + @Test func testWalkJSONReportsEmptyTargetForRegularFile() async throws { + try write("hello", to: contextDir.appendingPathComponent("plain.txt")) + + let fssync = try BuildFSSync(contextDir) + let infos = try await walkJSON(fssync) + + let plain = infos.first { $0.name == "plain.txt" } + #expect(plain != nil, "the regular file should appear in walk() results") + #expect(plain?.target == "", "a regular file should report an empty target") + } + + @Test func testWalkJSONReportsLiteralTargetForInContextSymlink() async throws { + try write("hello", to: contextDir.appendingPathComponent("real.txt")) + try fm.createSymbolicLink( + atPath: contextDir.appendingPathComponent("alias").path(percentEncoded: false), + withDestinationPath: "real.txt" + ) + + let fssync = try BuildFSSync(contextDir) + let infos = try await walkJSON(fssync) + + let alias = infos.first { $0.name == "alias" } + #expect(alias != nil, "the in-context symlink should appear in walk() results") + #expect(alias?.target == "real.txt", "in-context symlink target should be the literal on-disk value, got \(alias?.target ?? "nil")") + } + + @Test func testWalkJSONReportsLiteralTargetForAbsoluteSymlinkOutsideContext() async throws { + let secretFile = outsideDir.appendingPathComponent("secret.txt") + try write("supersecret", to: secretFile) + + let literalDestination = secretFile.path(percentEncoded: false) + try fm.createSymbolicLink( + atPath: contextDir.appendingPathComponent("leak").path(percentEncoded: false), + withDestinationPath: literalDestination + ) + + let fssync = try BuildFSSync(contextDir) + let infos = try await walkJSON(fssync) + + let leak = infos.first { $0.name == "leak" } + #expect(leak != nil, "the escaping symlink should still appear in walk() results") + #expect( + leak?.target == literalDestination, + "target should be the literal symlink destination, not a resolved/canonicalized path: \(leak?.target ?? "nil")") + } + + @Test func testWalkJSONReportsLiteralTargetForRelativeSymlinkOutsideContext() async throws { + let secretFile = outsideDir.appendingPathComponent("secret.txt") + try write("supersecret", to: secretFile) + + // contextDir = base/context, outsideDir = base/outside, so the + // relative destination from context/leak to outside/secret.txt is + // ../outside/secret.txt. Resolving this always yields an absolute + // path, so a literal-vs-resolved mismatch here is deterministic and + // does not depend on any symlink quirks in the host's temp dir. + let literalDestination = "../outside/secret.txt" + try fm.createSymbolicLink( + atPath: contextDir.appendingPathComponent("leak").path(percentEncoded: false), + withDestinationPath: literalDestination + ) + + let fssync = try BuildFSSync(contextDir) + let infos = try await walkJSON(fssync) + + let leak = infos.first { $0.name == "leak" } + #expect(leak != nil, "the escaping symlink should still appear in walk() results") + #expect( + leak?.target == literalDestination, + "target should be the literal (relative) symlink destination, never a resolved absolute host path: \(leak?.target ?? "nil")" + ) + #expect(leak?.target.hasPrefix("/") == false, "target must not be an absolute host path for an out-of-context symlink") + } + + @Test func testWalkJSONReportsLiteralTargetForDirectorySymlinkOutsideContext() async throws { + let secretFile = outsideDir.appendingPathComponent("secret.txt") + try write("supersecret", to: secretFile) + + let literalDestination = outsideDir.path(percentEncoded: false) + try fm.createSymbolicLink( + atPath: contextDir.appendingPathComponent("subdir").path(percentEncoded: false), + withDestinationPath: literalDestination + ) + + let fssync = try BuildFSSync(contextDir) + let infos = try await walkJSON(fssync, followPaths: ["subdir"]) + + let leak = infos.first { $0.name == "subdir" } + #expect(leak != nil, "the escaping directory symlink should still appear in walk() results") + #expect( + leak?.target == literalDestination, + "target should be the literal symlink destination, not the resolved external directory path: \(leak?.target ?? "nil")") + + // Nothing from inside the external directory should have leaked in as + // its own entry — the fixed Globber must not have descended into it. + let secretLeak = infos.first { $0.name.hasSuffix("secret.txt") } + #expect(secretLeak == nil, "no entry for the external file should appear in walk() results: \(infos.map { $0.name })") + } +} diff --git a/Tests/ContainerBuildTests/GlobberTests.swift b/Tests/ContainerBuildTests/GlobberTests.swift index fff0fc2d..b2d2a2c5 100644 --- a/Tests/ContainerBuildTests/GlobberTests.swift +++ b/Tests/ContainerBuildTests/GlobberTests.swift @@ -14,6 +14,7 @@ // limitations under the License. //===----------------------------------------------------------------------===// +import ContainerizationOS import Foundation import Testing @@ -179,7 +180,8 @@ let testCases = [ #expect(throws: Never.self) { try globber.match(test.pattern) let found: Bool = !globber.results.isEmpty - #expect(found == test.expectSuccess, "expected match to be \(test.expectSuccess), instead got \(found) \(tempDir.childrenRecursive)") + let onDisk = FileManager.default.enumerator(at: tempDir, includingPropertiesForKeys: nil)?.allObjects ?? [] + #expect(found == test.expectSuccess, "expected match to be \(test.expectSuccess), instead got \(found) \(onDisk)") } } @@ -202,4 +204,116 @@ let testCases = [ #expect(globber.results.isEmpty, "expected to find no matches, instead found \(globber.results)") } } + + // MARK: - Directory symlink traversal + // + // Globber must be able to descend through a directory symlink whose fully + // resolved target is still inside the match root (the same containment + // check BuildFSSync.read()/info() apply before serving content), while + // continuing to treat a symlink that escapes the root as a leaf with no + // children. See the discussion on + // https://github.com/apple/container-ghsa-2v2q-4q35-h585/pull/2. + + @Test("Match descends through an in-context directory symlink") + func testMatchFollowsInContextDirectorySymlink() throws { + let tempDir = FileManager.default.temporaryDirectory.appendingPathComponent(UUID().uuidString) + let real = tempDir.appendingPathComponent("real") + let link = tempDir.appendingPathComponent("link") + let file = real.appendingPathComponent("file.txt") + + try FileManager.default.createDirectory(at: real, withIntermediateDirectories: true) + try "hello".write(to: file, atomically: true, encoding: .utf8) + try FileManager.default.createSymbolicLink(at: link, withDestinationURL: real) + + defer { try? FileManager.default.removeItem(at: tempDir) } + + let globber = Globber(tempDir) + try globber.match("link/file.txt") + + // Compare by name rather than exact path: on macOS /var is itself a + // symlink to /private/var, and FileManager.contentsOfDirectory vs. + // URL.resolvingSymlinksInPath() normalize that inconsistently, so a + // temp-dir-rooted URL built by hand won't reliably string-match a URL + // Globber discovered through the real filesystem APIs. + #expect( + globber.results.contains { $0.isSymlink && $0.lastPathComponent == "link" }, + "expected the symlink itself to be preserved in results, got \(globber.results)" + ) + #expect( + globber.results.contains { $0.lastPathComponent == "file.txt" }, + "expected the resolved physical file to be present in results, got \(globber.results)" + ) + } + + @Test("Recursive glob descends through an in-context directory symlink") + func testMatchRecursiveGlobFollowsInContextDirectorySymlink() throws { + let tempDir = FileManager.default.temporaryDirectory.appendingPathComponent(UUID().uuidString) + let real = tempDir.appendingPathComponent("real") + let link = tempDir.appendingPathComponent("link") + let file = real.appendingPathComponent("file.txt") + + try FileManager.default.createDirectory(at: real, withIntermediateDirectories: true) + try "hello".write(to: file, atomically: true, encoding: .utf8) + try FileManager.default.createSymbolicLink(at: link, withDestinationURL: real) + + defer { try? FileManager.default.removeItem(at: tempDir) } + + let globber = Globber(tempDir) + try globber.match("link/**") + + #expect(globber.results.contains { $0.isSymlink && $0.lastPathComponent == "link" }) + #expect(globber.results.contains { $0.lastPathComponent == "real" }) + #expect(globber.results.contains { $0.lastPathComponent == "file.txt" }) + } + + @Test("Match does not descend through a directory symlink that escapes the root") + func testMatchDoesNotDescendThroughSymlinkEscapingRoot() throws { + let tempDir = FileManager.default.temporaryDirectory.appendingPathComponent(UUID().uuidString) + let outsideDir = FileManager.default.temporaryDirectory.appendingPathComponent(UUID().uuidString) + let link = tempDir.appendingPathComponent("link") + let secret = outsideDir.appendingPathComponent("secret.txt") + + try FileManager.default.createDirectory(at: tempDir, withIntermediateDirectories: true) + try FileManager.default.createDirectory(at: outsideDir, withIntermediateDirectories: true) + try "supersecret".write(to: secret, atomically: true, encoding: .utf8) + try FileManager.default.createSymbolicLink(at: link, withDestinationURL: outsideDir) + + defer { + try? FileManager.default.removeItem(at: tempDir) + try? FileManager.default.removeItem(at: outsideDir) + } + + let globber = Globber(tempDir) + try globber.match("link/secret.txt") + + #expect(globber.results.isEmpty, "expected no match through a symlink escaping the root, got \(globber.results)") + } + + @Test("Recursive glob through a symlink escaping the root never leaks out-of-root files") + func testMatchRecursiveGlobDoesNotLeakOutsideRoot() throws { + let tempDir = FileManager.default.temporaryDirectory.appendingPathComponent(UUID().uuidString) + let outsideDir = FileManager.default.temporaryDirectory.appendingPathComponent(UUID().uuidString) + let link = tempDir.appendingPathComponent("link") + let secret = outsideDir.appendingPathComponent("secret.txt") + + try FileManager.default.createDirectory(at: tempDir, withIntermediateDirectories: true) + try FileManager.default.createDirectory(at: outsideDir, withIntermediateDirectories: true) + try "supersecret".write(to: secret, atomically: true, encoding: .utf8) + try FileManager.default.createSymbolicLink(at: link, withDestinationURL: outsideDir) + + defer { + try? FileManager.default.removeItem(at: tempDir) + try? FileManager.default.removeItem(at: outsideDir) + } + + let globber = Globber(tempDir) + try globber.match("link/**") + + let leaked = globber.results.filter { url in + guard !url.isSymlink else { return false } + let resolved = url.resolvingSymlinksInPath() + return !tempDir.parentOf(resolved) && resolved.cleanPath != tempDir.cleanPath + } + #expect(leaked.isEmpty, "match() produced non-symlink results that physically resolve outside the root: \(leaked)") + } } diff --git a/Tests/ContainerPluginTests/PluginFactoryTest.swift b/Tests/ContainerPluginTests/PluginFactoryTest.swift index cc154560..20f53643 100644 --- a/Tests/ContainerPluginTests/PluginFactoryTest.swift +++ b/Tests/ContainerPluginTests/PluginFactoryTest.swift @@ -14,6 +14,7 @@ // limitations under the License. //===----------------------------------------------------------------------===// +import ContainerizationError import Foundation import Logging import Testing @@ -328,4 +329,78 @@ struct PluginFactoryTest { #expect(plugin.name == name) #expect(plugin.config.abstract == "TOML service") } + + @Test + func testDefaultFactoryRejectsTraversalName() async throws { + let fm = FileManager.default + let tempURL = try fm.url( + for: .itemReplacementDirectory, + in: .userDomainMask, + appropriateFor: .temporaryDirectory, + create: true + ) + defer { try? FileManager.default.removeItem(at: tempURL) } + + // A complete, loadable plugin layout planted outside the plugin parent directory. + let outsideURL = tempURL.appending(path: "outside") + let binDirURL = outsideURL.appending(path: "bin") + try fm.createDirectory(at: binDirURL, withIntermediateDirectories: true) + try "abstract = \"payload\"\nauthor = \"Apple\"" + .write(to: outsideURL.appending(path: "config.toml"), atomically: true, encoding: .utf8) + try "".write(to: binDirURL.appending(path: "outside"), atomically: true, encoding: .utf8) + + let pluginParent = tempURL.appending(path: "plugins") + try fm.createDirectory(at: pluginParent, withIntermediateDirectories: true) + + let factory = DefaultPluginFactory(logger: Logger(label: "test")) + #expect(throws: ContainerizationError.self) { + try factory.create(parentURL: pluginParent, name: "../outside") + } + } + + @Test + func testDefaultFactoryRejectsParentDirectoryName() async throws { + let factory = DefaultPluginFactory(logger: Logger(label: "test")) + #expect(throws: ContainerizationError.self) { + try factory.create(parentURL: URL(fileURLWithPath: "/tmp"), name: "..") + } + } + + @Test + func testDefaultFactoryRejectsNullByteTraversalName() async throws { + let factory = DefaultPluginFactory(logger: Logger(label: "test")) + #expect(throws: ContainerizationError.self) { + try factory.create(parentURL: URL(fileURLWithPath: "/tmp"), name: "evil\u{0}/../../../../etc") + } + } + + @Test + func testAppBundleFactoryRejectsTraversalName() async throws { + let fm = FileManager.default + let tempURL = try fm.url( + for: .itemReplacementDirectory, + in: .userDomainMask, + appropriateFor: .temporaryDirectory, + create: true + ) + defer { try? FileManager.default.removeItem(at: tempURL) } + + // A complete, loadable app-bundle plugin layout planted outside the plugin parent directory. + let outsideURL = tempURL.appending(path: "outside.app") + let resourcesURL = outsideURL.appending(path: "Contents").appending(path: "Resources") + try fm.createDirectory(at: resourcesURL, withIntermediateDirectories: true) + try "abstract = \"payload\"\nauthor = \"Apple\"" + .write(to: resourcesURL.appending(path: "config.toml"), atomically: true, encoding: .utf8) + let macosURL = outsideURL.appending(path: "Contents").appending(path: "MacOS") + try fm.createDirectory(at: macosURL, withIntermediateDirectories: true) + try "".write(to: macosURL.appending(path: "outside"), atomically: true, encoding: .utf8) + + let pluginParent = tempURL.appending(path: "plugins") + try fm.createDirectory(at: pluginParent, withIntermediateDirectories: true) + + let factory = AppBundlePluginFactory(logger: Logger(label: "test")) + #expect(throws: ContainerizationError.self) { + try factory.create(parentURL: pluginParent, name: "../outside") + } + } } diff --git a/Tests/IntegrationTests/Build/TestCLIBuilder.swift b/Tests/IntegrationTests/Build/TestCLIBuilder.swift index 7fc250f1..95d4f74f 100644 --- a/Tests/IntegrationTests/Build/TestCLIBuilder.swift +++ b/Tests/IntegrationTests/Build/TestCLIBuilder.swift @@ -1000,4 +1000,34 @@ struct TestCLIBuilder { #expect(result.status != 0, "build should fail when source image does not exist") } } + + /// Regression test: the context *root* itself (not an entry inside it) is a + /// symlink to a sibling directory, e.g. `context -> real-context`. The build + /// must resolve the symlink and use the real directory's contents. + @Test func testBuildContextRootSymlink() async throws { + try await ContainerFixture.with { f in + let dir = try f.createTempDir() + let dockerfile = """ + FROM ghcr.io/linuxcontainers/alpine:3.20 + COPY hello.txt hello.txt + RUN cat hello.txt + """ + try f.createContext( + dir: dir, + dockerfile: dockerfile, + context: [ + .file("hello.txt", content: .data(Data("hello from real-context".utf8))) + ]) + + let contextPath = dir.appending("context") + let realContextPath = dir.appending("real-context") + try FileManager.default.moveItem(atPath: contextPath.string, toPath: realContextPath.string) + try FileManager.default.createSymbolicLink( + atPath: contextPath.string, withDestinationPath: "real-context") + + let image = "registry.local/build-context-root-symlink:\(UUID().uuidString)" + try f.build(tag: image, contextDir: dir) + try f.assertImageBuilt(image) + } + } }