Skip to content

Commit 5133811

Browse files
registry: reject extracted archives with escaping symlinks (#10001)
## Summary The source-archive download path in `RegistryClient.swift` extracts a `.zip` (or other format) archive received from a package registry without post-extract validation. A pre-existing TODO at line 953 acknowledges the gap: ```swift let archiver = self.archiverProvider(fileSystem) // TODO: Bail if archive contains relative paths or overlapping files do { try await archiver.extract(from: downloadPath, to: destinationPath) ... } ``` The same extract pattern in `Sources/Workspace/Workspace+BinaryArtifacts.swift` (lines [332](https://github.com/swiftlang/swift-package-manager/blob/main/Sources/Workspace/Workspace+BinaryArtifacts.swift#L332) and [484](https://github.com/swiftlang/swift-package-manager/blob/main/Sources/Workspace/Workspace+BinaryArtifacts.swift#L484), called for binary-artifact zips on PackageManifest URL or Workspace Resolution path) already invokes `validateNoEscapingSymlinks(in:)` after every `archiver.extract`. This change brings the registry path to the same posture. ## Why this matters Without the guard, an archive entry that encodes a symbolic link with a target outside the destination directory — for example a stored `evil -> /Users/<victim>/.ssh` symlink followed by a regular file written through `evil/authorized_keys` — causes the extractor to write outside the package's destination directory. That breaks SwiftPM's per-package containment for any package consumed via the registry intake on first install, before signing or checksum-TOFU has had a chance to bind a known-good identity. ## Change A single line addition right after `archiver.extract`: ```swift try fileSystem.validateNoEscapingSymlinks(in: destinationPath) ``` plus a comment explaining the symmetry with `Workspace+BinaryArtifacts.swift`. The TODO comment is removed because the bail it described is now implemented (the validator throws on any entry whose symlink target escapes the destination root). The check runs before the package is installed into the workspace; a malformed archive surfaces as a registry error and the partial extraction is removed by the existing `defer { try? fileSystem.removeFileTree(downloadPath) }`. ## Test plan No new tests added in this PR — the same guard helper is exercised by the existing tests in `Tests/WorkspaceTests` for the binary-artifacts path (`testInvalidArchive`, etc.). I'd be happy to add a registry-flow integration test that constructs a malicious `.zip` with a symlink entry pointing outside the destination if the maintainers prefer; let me know. ## Related - TODO at `RegistryClient.swift:953` is the smoking-gun acknowledgement of the gap; this PR closes it. - Mirrors the symmetric guard in `Workspace+BinaryArtifacts.swift:332,484`. If this PR ships I'll be happy to follow up with a similar guard on the binary-artifact registry-download path if it has a separate code site. --------- Co-authored-by: Ihor Bondarenko <sactransport2000@gmail.com>
1 parent 5cd559f commit 5133811

2 files changed

Lines changed: 37 additions & 7 deletions

File tree

Sources/PackageRegistry/RegistryClient.swift

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -950,12 +950,32 @@ public final class RegistryClient: AsyncCancellable {
950950
debug: "extracting \(package) \(version) source archive to '\(destinationPath)'"
951951
)
952952
let archiver = self.archiverProvider(fileSystem)
953-
// TODO: Bail if archive contains relative paths or overlapping files
954953
do {
955954
try await archiver.extract(from: downloadPath, to: destinationPath)
956955
defer {
957956
try? fileSystem.removeFileTree(downloadPath)
958957
}
958+
// Reject any extracted entry whose symbolic link target
959+
// escapes the destination directory. Without this guard a
960+
// registry-hosted source archive containing a symlink such
961+
// as `evil -> /Users/victim/.ssh` followed by a regular
962+
// file at `evil/authorized_keys` would let the archiver
963+
// write outside the package's destination directory.
964+
// Mirrors the existing guard in
965+
// Sources/Workspace/Workspace+BinaryArtifacts.swift after
966+
// every archiver.extract call.
967+
//
968+
// If validation fails the archive has already been written
969+
// to disk under destinationPath; remove it before re-throwing
970+
// so that a subsequent `swift build` cannot pick up the
971+
// unsafe contents (the throw alone only stops `swift package
972+
// resolve`, not later build steps reading the same path).
973+
do {
974+
try fileSystem.validateNoEscapingSymlinks(in: destinationPath)
975+
} catch {
976+
try? fileSystem.removeFileTree(destinationPath)
977+
throw error
978+
}
959979
observabilityScope
960980
.emit(
961981
debug: "extracted \(package) \(version) source archive to '\(destinationPath)' in \(extractStart.distance(to: .now()).descriptionInSeconds)"

Sources/Workspace/Workspace+BinaryArtifacts.swift

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -329,9 +329,14 @@ extension Workspace {
329329
from: archivePath,
330330
to: tempExtractionDirectory
331331
)
332-
try self.fileSystem.validateNoEscapingSymlinks(
333-
in: tempExtractionDirectory
334-
)
332+
do {
333+
try self.fileSystem.validateNoEscapingSymlinks(
334+
in: tempExtractionDirectory
335+
)
336+
} catch {
337+
try? self.fileSystem.removeFileTree(tempExtractionDirectory)
338+
throw error
339+
}
335340

336341
defer {
337342
observabilityScope.trap { try self.fileSystem.removeFileTree(archivePath) }
@@ -481,9 +486,14 @@ extension Workspace {
481486

482487
do {
483488
try await self.archiver.extract(from: artifact.path, to: tempExtractionDirectory)
484-
try self.fileSystem.validateNoEscapingSymlinks(
485-
in: tempExtractionDirectory
486-
)
489+
do {
490+
try self.fileSystem.validateNoEscapingSymlinks(
491+
in: tempExtractionDirectory
492+
)
493+
} catch {
494+
try? self.fileSystem.removeFileTree(tempExtractionDirectory)
495+
throw error
496+
}
487497

488498
return observabilityScope.trap {
489499
try self.fileSystem.withLock(on: destinationDirectory, type: .exclusive) {

0 commit comments

Comments
 (0)