Build context fix, bump to containerization 0.52.1. (#2415)

- `BuildFSSync.read()` and `info()` resolved a 
  requested path, checked that the result was
  inside the build context, and then opened the
  original path. A symlink replaced between the
  check and the open was followed to its new
  target, which could be outside the context.
-This change opens each requested file by
  walking its path one component at a time from
  a directory descriptor pinned on the context
  root, using `FileDescriptorOps` from
  `containerization`. Symlinks are followed only
  when the target is relative and stays beneath
  the context root. The metadata and the content
  returned to the builder both come from the
  same open descriptor.
- Behavior change: a symlink with an absolute
  target is now rejected by `read()` and `info()`,
  even when the target is inside the context.
  Relative symlinks that stay inside the context
  still work. `walk()`-based transfers are unchanged.
- Dependency: Requires `containerization` 0.50.0
  for its `FileDescriptorOps` API enhancements.
  This change updates the library from 0.52.0
  to 0.52.1.

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"
       }
     },
     {

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",

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<ClientStream>.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<ClientStream>.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<T>(_ 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 {

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<ClientStream>.Continuation!
+        _ = AsyncStream<ClientStream> { 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<ClientStream>.Continuation!
+        _ = AsyncStream<ClientStream> { 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<ClientStream>.Continuation!
+        let stream = AsyncStream<ClientStream> { 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 } }
+}