diff --git a/Signal.xcodeproj/project.pbxproj b/Signal.xcodeproj/project.pbxproj index 5d5bb125505..ce2e6c1913e 100644 --- a/Signal.xcodeproj/project.pbxproj +++ b/Signal.xcodeproj/project.pbxproj @@ -734,6 +734,7 @@ 505C2ED42997015800C23FB2 /* LinkDeviceViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 505C2ED32997015800C23FB2 /* LinkDeviceViewController.swift */; }; 505C2ED629971D4E00C23FB2 /* DeviceLimitExceededError.swift in Sources */ = {isa = PBXBuildFile; fileRef = 505C2ED529971D4E00C23FB2 /* DeviceLimitExceededError.swift */; }; 505C2ED92997422D00C23FB2 /* SelfSignedIdentityTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 505C2ED82997422D00C23FB2 /* SelfSignedIdentityTest.swift */; }; + 6218A0022F00000000000001 /* DeviceTransferPathTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6218A0012F00000000000001 /* DeviceTransferPathTest.swift */; }; 505CC1672F22BC7A00D311CD /* OutgoingPaymentActivationRequestMessage.swift in Sources */ = {isa = PBXBuildFile; fileRef = 505CC1662F22BC7A00D311CD /* OutgoingPaymentActivationRequestMessage.swift */; }; 505CC1692F22C2EC00D311CD /* OutgoingPaymentActivationRequestFinishedMessage.swift in Sources */ = {isa = PBXBuildFile; fileRef = 505CC1682F22C2EC00D311CD /* OutgoingPaymentActivationRequestFinishedMessage.swift */; }; 505F76332BC45C0700B1B51C /* BuildFlags+Generated.swift in Sources */ = {isa = PBXBuildFile; fileRef = 505F76322BC45C0700B1B51C /* BuildFlags+Generated.swift */; }; @@ -5000,6 +5001,7 @@ 505C2ED32997015800C23FB2 /* LinkDeviceViewController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = LinkDeviceViewController.swift; sourceTree = ""; }; 505C2ED529971D4E00C23FB2 /* DeviceLimitExceededError.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DeviceLimitExceededError.swift; sourceTree = ""; }; 505C2ED82997422D00C23FB2 /* SelfSignedIdentityTest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SelfSignedIdentityTest.swift; sourceTree = ""; }; + 6218A0012F00000000000001 /* DeviceTransferPathTest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DeviceTransferPathTest.swift; sourceTree = ""; }; 505C2EDA29974D2000C23FB2 /* StorageServiceContactTest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = StorageServiceContactTest.swift; sourceTree = ""; }; 505CC1662F22BC7A00D311CD /* OutgoingPaymentActivationRequestMessage.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = OutgoingPaymentActivationRequestMessage.swift; sourceTree = ""; }; 505CC1682F22C2EC00D311CD /* OutgoingPaymentActivationRequestFinishedMessage.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = OutgoingPaymentActivationRequestFinishedMessage.swift; sourceTree = ""; }; @@ -9767,6 +9769,7 @@ 505C2ED72997421E00C23FB2 /* DeviceTransfer */ = { isa = PBXGroup; children = ( + 6218A0012F00000000000001 /* DeviceTransferPathTest.swift */, 505C2ED82997422D00C23FB2 /* SelfSignedIdentityTest.swift */, ); path = DeviceTransfer; @@ -18811,6 +18814,7 @@ F93999F628C81F2100E34899 /* DataMessagePaddingTests.swift in Sources */, 3494BBE026E66FC30079B11B /* DateUtilTest.swift in Sources */, C17A54962D7B3D9E00E1D267 /* DeviceProvisioningURLTest.swift in Sources */, + 6218A0022F00000000000001 /* DeviceTransferPathTest.swift in Sources */, D92812E22FA95C1400667DCF /* DisplayableAccountEntropyPoolTest.swift in Sources */, 45E7A6A81E71CA7E00D44FB5 /* DisplayableTextFilterTest.swift in Sources */, F90B7BC12912B90100F50A59 /* DonateViewControllerTest.swift in Sources */, diff --git a/Signal/DeviceTransfer/DeviceTransferService+MultipeerDelegates.swift b/Signal/DeviceTransfer/DeviceTransferService+MultipeerDelegates.swift index 8590562fb39..cd2951ceca1 100644 --- a/Signal/DeviceTransfer/DeviceTransferService+MultipeerDelegates.swift +++ b/Signal/DeviceTransfer/DeviceTransferService+MultipeerDelegates.swift @@ -316,12 +316,13 @@ extension DeviceTransferService: MCSessionDelegate { } do { + let pendingFilePath = try DeviceTransferService.validatedPath( + for: file.identifier, + within: DeviceTransferService.pendingTransferFilesDirectory, + ) try OWSFileSystem.moveFilePath( localURL.path, - toFilePath: URL( - fileURLWithPath: file.identifier, - relativeTo: DeviceTransferService.pendingTransferFilesDirectory, - ).path, + toFilePath: pendingFilePath, ) } catch { Logger.warn("Couldn't move file: \(error.shortDescription)") diff --git a/Signal/DeviceTransfer/DeviceTransferService+Restore.swift b/Signal/DeviceTransfer/DeviceTransferService+Restore.swift index 9abb5ccc4ec..ff2ed39deb4 100644 --- a/Signal/DeviceTransfer/DeviceTransferService+Restore.swift +++ b/Signal/DeviceTransfer/DeviceTransferService+Restore.swift @@ -68,14 +68,17 @@ extension DeviceTransferService { owsFailDebug("did not receive file \(file.identifier)") return false } - guard - OWSFileSystem.fileOrFolderExists( - atPath: URL( - fileURLWithPath: file.identifier, - relativeTo: DeviceTransferService.pendingTransferFilesDirectory, - ).path, + let filePath: String + do { + filePath = try DeviceTransferService.validatedPath( + for: file.identifier, + within: DeviceTransferService.pendingTransferFilesDirectory, ) - else { + } catch { + owsFailDebug("Invalid file identifier in manifest") + return false + } + guard OWSFileSystem.fileOrFolderExists(atPath: filePath) else { owsFailDebug("Missing file \(file.identifier)") return false } @@ -162,10 +165,16 @@ extension DeviceTransferService { } for file in manifest.files + [database.database, database.wal] { - let pendingFilePath = URL( - fileURLWithPath: file.identifier, - relativeTo: DeviceTransferService.pendingTransferFilesDirectory, - ).path + let pendingFilePath: String + do { + pendingFilePath = try DeviceTransferService.validatedPath( + for: file.identifier, + within: DeviceTransferService.pendingTransferFilesDirectory, + ) + } catch { + owsFailDebug("Invalid file identifier in manifest") + return false + } // We could be receiving a database in any of the directory modes, // so we force the restore path to be the "primary" database since @@ -177,10 +186,15 @@ extension DeviceTransferService { } else if DeviceTransferService.databaseWALIdentifier == file.identifier { newFilePath = GRDBDatabaseStorageAdapter.databaseWalUrl(directoryMode: .primary).path } else { - newFilePath = URL( - fileURLWithPath: file.relativePath, - relativeTo: DeviceTransferService.appSharedDataDirectory, - ).path + do { + newFilePath = try DeviceTransferService.validatedPath( + for: file.relativePath, + within: DeviceTransferService.appSharedDataDirectory, + ) + } catch { + owsFailDebug("Invalid file relative path in manifest") + return false + } } // If we're hot swapping the database, we move the database @@ -452,11 +466,11 @@ extension DeviceTransferService { let destDir = DeviceTransferService.appSharedDataDirectory try manifest.files.forEach { file in - let sourceUrl = URL(fileURLWithPath: file.identifier, relativeTo: sourceDir) - let destUrl = URL(fileURLWithPath: file.relativePath, relativeTo: destDir) + let sourcePath = try DeviceTransferService.validatedPath(for: file.identifier, within: sourceDir) + let destPath = try DeviceTransferService.validatedPath(for: file.relativePath, within: destDir) do { - try move(pendingFilePath: sourceUrl.path, to: destUrl.path) + try move(pendingFilePath: sourcePath, to: destPath) } catch CocoaError.fileWriteFileExists { Logger.info("Skipping restoration of file that was already restored: \(file.identifier)") } catch CocoaError.fileNoSuchFile, CocoaError.fileReadNoSuchFile, POSIXError.ENOENT { diff --git a/Signal/DeviceTransfer/DeviceTransferService.swift b/Signal/DeviceTransfer/DeviceTransferService.swift index 73545572b4a..0dce0a071d0 100644 --- a/Signal/DeviceTransfer/DeviceTransferService.swift +++ b/Signal/DeviceTransfer/DeviceTransferService.swift @@ -89,6 +89,35 @@ class DeviceTransferService: NSObject, DeviceTransferServiceProtocol { static let missingFileData = Data("Missing File".utf8) static let missingFileHash = Data(SHA256.hash(data: missingFileData)) + static func validatedPath(for relativePath: String, within baseDirectory: URL) throws -> String { + guard !relativePath.isEmpty, !relativePath.hasPrefix("/") else { + throw OWSAssertionError("Received invalid transfer file path") + } + + let resolvedBase = baseDirectory + .standardizedFileURL + .resolvingSymlinksInPath() + let resolvedUrl = URL(fileURLWithPath: relativePath, relativeTo: resolvedBase) + .standardizedFileURL + + let basePathComponents = resolvedBase.pathComponents + guard + resolvedUrl.pathComponents.count > basePathComponents.count, + resolvedUrl.pathComponents.starts(with: basePathComponents) + else { + throw OWSAssertionError("Received transfer file path that escapes its base directory") + } + + var currentUrl = resolvedBase + for pathComponent in resolvedUrl.pathComponents.dropFirst(basePathComponents.count) { + currentUrl.appendPathComponent(pathComponent) + if (try? currentUrl.resourceValues(forKeys: [.isSymbolicLinkKey]).isSymbolicLink) == true { + throw OWSAssertionError("Received transfer file path containing a symbolic link") + } + } + return resolvedUrl.path + } + // This must also be updated in the info.plist private static let newDeviceServiceIdentifier = "sgnl-new-device" diff --git a/Signal/test/util/DeviceTransfer/DeviceTransferPathTest.swift b/Signal/test/util/DeviceTransfer/DeviceTransferPathTest.swift new file mode 100644 index 00000000000..079914edb36 --- /dev/null +++ b/Signal/test/util/DeviceTransfer/DeviceTransferPathTest.swift @@ -0,0 +1,83 @@ +// +// Copyright 2026 Signal Messenger, LLC +// SPDX-License-Identifier: AGPL-3.0-only +// + +import XCTest + +@testable import Signal + +final class DeviceTransferPathTest: XCTestCase { + private var temporaryDirectory: URL! + private var baseDirectory: URL { + temporaryDirectory.appendingPathComponent("device-transfer", isDirectory: true) + } + + override func setUpWithError() throws { + temporaryDirectory = FileManager.default.temporaryDirectory + .appendingPathComponent(UUID().uuidString, isDirectory: true) + try FileManager.default.createDirectory( + at: baseDirectory, + withIntermediateDirectories: true, + ) + } + + override func tearDownWithError() throws { + try FileManager.default.removeItem(at: temporaryDirectory) + temporaryDirectory = nil + } + + func testValidRelativePath() throws { + XCTAssertEqual( + try DeviceTransferService.validatedPath( + for: "Attachments/uuid/file.dat", + within: baseDirectory, + ), + baseDirectory + .appendingPathComponent("Attachments/uuid/file.dat") + .resolvingSymlinksInPath() + .path, + ) + } + + func testRejectsInvalidPaths() { + for path in ["", ".", "..", "../outside", "directory/../../outside", "/tmp/outside"] { + XCTAssertThrowsError( + try DeviceTransferService.validatedPath(for: path, within: baseDirectory), + "Expected path to be rejected: \(path)", + ) + } + } + + func testAllowsNormalizedPathWithinBaseDirectory() throws { + XCTAssertEqual( + try DeviceTransferService.validatedPath( + for: "Attachments/../file.dat", + within: baseDirectory, + ), + baseDirectory + .appendingPathComponent("file.dat") + .resolvingSymlinksInPath() + .path, + ) + } + + func testRejectsPathThroughSymlinkOutsideBaseDirectory() throws { + let outsideDirectory = temporaryDirectory.appendingPathComponent("outside", isDirectory: true) + try FileManager.default.createDirectory( + at: outsideDirectory, + withIntermediateDirectories: true, + ) + try FileManager.default.createSymbolicLink( + at: baseDirectory.appendingPathComponent("link"), + withDestinationURL: outsideDirectory, + ) + + XCTAssertThrowsError( + try DeviceTransferService.validatedPath( + for: "link/file.dat", + within: baseDirectory, + ), + ) + } +}