diff --git a/ios/MullvadSettings/KeychainSettingsStore.swift b/ios/MullvadSettings/KeychainSettingsStore.swift index ba0612de0d3a..d9a058cbcdd9 100644 --- a/ios/MullvadSettings/KeychainSettingsStore.swift +++ b/ios/MullvadSettings/KeychainSettingsStore.swift @@ -7,33 +7,68 @@ // import Foundation +import MullvadLogging import MullvadTypes import Security public class KeychainSettingsStore: SettingsStore { public let serviceName: String public let accessGroup: String + private let logger = Logger(label: "KeychainSettingsStore") + private let cacheDirectory: URL - public init(serviceName: String, accessGroup: String) { + public init(serviceName: String, accessGroup: String, cacheDirectory: URL) { self.serviceName = serviceName self.accessGroup = accessGroup + self.cacheDirectory = cacheDirectory.appendingPathComponent("keychainLock.json") } public func read(key: SettingsKey) throws -> Data { - try readItemData(key) + try coordinate(Data(), try readItemData(key)) } public func write(_ data: Data, for key: SettingsKey) throws { - try addOrUpdateItem(key, data: data) + try coordinate((), try addOrUpdateItem(key, data: data)) } public func delete(key: SettingsKey) throws { - try deleteItem(key) + try coordinate((), try deleteItem(key)) + } + + /// Prevents all items in `keys` from backup inclusion + /// + /// This method uses the `coordinate` helper function to guarantee atomicity + /// of the keychain exclusion process so that a pre-running VPN process cannot + /// accidentally read or write to the keychain when the exclusion happens. + /// It will be blocked temporarily and automatically resume when the migration is done. + /// + /// Likewise, the exclusion process will also be forced to wait until it can access the keychain + /// if the VPN process is operating on it. + /// + /// - Important: Do not call `read`, `write`, or `delete` from this method, + /// the coordinator is *not reentrant* and will deadlock if you do so. + /// Only call methods that do not call `coordinate`. + /// + /// - Parameter keys: The keys to exclude from backup + public func excludeFromBackup(keys: [SettingsKey]) { + let coordination = { [unowned self] in + for key in keys { + do { + let data = try readItemData(key) + try deleteItem(key) + try addItem(key, data: data) + } catch { + logger.error("Could not exclude \(key) from backups. \(error)") + } + } + } + + try? coordinate((), coordination()) } private func addItem(_ item: SettingsKey, data: Data) throws { var query = createDefaultAttributes(item: item) - query.merge(createAccessAttributes()) { current, _ in + query.merge(createAccessAttributesThisDeviceOnly()) { current, _ in current } query[kSecValueData] = data @@ -96,10 +131,38 @@ public class KeychainSettingsStore: SettingsStore { ] } - private func createAccessAttributes() -> [CFString: Any] { + private func createAccessAttributesThisDeviceOnly() -> [CFString: Any] { [ kSecAttrAccessGroup: accessGroup, - kSecAttrAccessible: kSecAttrAccessibleAfterFirstUnlock, + kSecAttrAccessible: kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly, ] } + + /// Runs `action` in a cross process synchronized way + /// + /// This enables doing CRUD operations on the keychain items in a cross process safe way. + /// This does not prevent TOCTOU issues. + /// - Parameters: + /// - initial: Dummy value used for the returned value, if any. + /// - action: The CRUD operation to run on the keychain. + /// - Returns: The result of the keychain operation, if any. + private func coordinate(_ initial: T, _ action: @autoclosure () throws -> T) throws -> T { + let fileCoordinator = NSFileCoordinator(filePresenter: nil) + var error: NSError? + var thrownError: Error? + var returnedValue: T = initial + fileCoordinator.coordinate(writingItemAt: cacheDirectory, error: &error) { _ in + do { + returnedValue = try action() + } catch { + thrownError = error + } + } + + if let thrownError { + throw thrownError + } + + return returnedValue + } } diff --git a/ios/MullvadSettings/KeychainSettingsStoreMigration.swift b/ios/MullvadSettings/KeychainSettingsStoreMigration.swift new file mode 100644 index 000000000000..e6dc38f33b45 --- /dev/null +++ b/ios/MullvadSettings/KeychainSettingsStoreMigration.swift @@ -0,0 +1,63 @@ +// +// KeychainSettingsStoreMigration.swift +// MullvadSettings +// +// Created by Marco Nikic on 2025-01-07. +// Copyright © 2025 Mullvad VPN AB. All rights reserved. +// + +import Foundation + +public struct KeychainSettingsStoreMigration { + private let serviceName: String + private let accessGroup: String + private let store: SettingsStore + + init(serviceName: String, accessGroup: String, store: SettingsStore) { + self.serviceName = serviceName + self.accessGroup = accessGroup + self.store = store + } + + /// Creates a keychain query matching old keychain settings that would be included in backups. + private func createQueryAttributes(item: SettingsKey) -> [CFString: Any] { + [ + kSecClass: kSecClassGenericPassword, + kSecAttrService: serviceName, + kSecAttrAccount: item.rawValue, + kSecAttrAccessGroup: accessGroup, + kSecAttrAccessible: kSecAttrAccessibleAfterFirstUnlock, + ] + } + + /// Whether items saved in the keychain should be migrated to be excluded from backups. + /// + /// Create a keychain item query using the old value `kSecAttrAccessibleAfterFirstUnlock` + /// for keychain items accessibility. + /// + /// If there is a match, probe whether it's a first time launch by querying whether the `SettingKey` + /// `shouldWipeSettings` has already been set. + /// + /// If both the query is successful, and `shouldWipeSettings` has already been set, + /// this means the keychain saved settings accessibility need to be upgraded to `kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly`. + /// + /// This will be done automatically by deleting and re-adding entries in the keychain. + /// - Returns: Whether keychain settings should be deleted and added again + private func shouldExcludeSettingsFromBackup() -> Bool { + var query = createQueryAttributes(item: .shouldWipeSettings) + query[kSecReturnData] = true + + var result: CFTypeRef? + let status = SecItemCopyMatching(query as CFDictionary, &result) + let shouldWipeSettingsWasSet = (try? store.read(key: .shouldWipeSettings)) != nil + + return status == errSecSuccess && shouldWipeSettingsWasSet + } + + public func excludeKeychainSettingsFromBackups() { + guard shouldExcludeSettingsFromBackup() == true else { return } + store.excludeFromBackup(keys: SettingsKey.allCases) + + precondition(shouldExcludeSettingsFromBackup() == false) + } +} diff --git a/ios/MullvadSettings/SettingsManager.swift b/ios/MullvadSettings/SettingsManager.swift index d414d7ed0aa7..ff38232b3444 100644 --- a/ios/MullvadSettings/SettingsManager.swift +++ b/ios/MullvadSettings/SettingsManager.swift @@ -20,7 +20,8 @@ public enum SettingsManager { #if DEBUG private static var _store = KeychainSettingsStore( serviceName: keychainServiceName, - accessGroup: ApplicationConfiguration.securityGroupIdentifier + accessGroup: ApplicationConfiguration.securityGroupIdentifier, + cacheDirectory: ApplicationConfiguration.containerURL ) /// Alternative store used for tests. @@ -163,6 +164,16 @@ public enum SettingsManager { } } + public static func excludeKeychainSettingsFromBackups() { + let migration = KeychainSettingsStoreMigration( + serviceName: keychainServiceName, + accessGroup: ApplicationConfiguration.securityGroupIdentifier, + store: store + ) + + migration.excludeKeychainSettingsFromBackups() + } + // MARK: - Private private static func checkLatestSettingsVersion() throws { diff --git a/ios/MullvadSettings/SettingsStore.swift b/ios/MullvadSettings/SettingsStore.swift index 2901ae2bb3b9..2e908d008ba5 100644 --- a/ios/MullvadSettings/SettingsStore.swift +++ b/ios/MullvadSettings/SettingsStore.swift @@ -22,4 +22,5 @@ public protocol SettingsStore { func read(key: SettingsKey) throws -> Data func write(_ data: Data, for key: SettingsKey) throws func delete(key: SettingsKey) throws + func excludeFromBackup(keys: [SettingsKey]) } diff --git a/ios/MullvadVPN.xcodeproj/project.pbxproj b/ios/MullvadVPN.xcodeproj/project.pbxproj index 8969294266ac..b5f13b415f86 100644 --- a/ios/MullvadVPN.xcodeproj/project.pbxproj +++ b/ios/MullvadVPN.xcodeproj/project.pbxproj @@ -744,6 +744,7 @@ A93969812CE606190032A7A0 /* Maybenot.swift in Sources */ = {isa = PBXBuildFile; fileRef = A9840BB32C69F78A0030F05E /* Maybenot.swift */; }; A94D691A2ABAD66700413DD4 /* WireGuardKitTypes in Frameworks */ = {isa = PBXBuildFile; productRef = 58FE25E22AA72AE9003D1918 /* WireGuardKitTypes */; }; A94D691B2ABAD66700413DD4 /* WireGuardKitTypes in Frameworks */ = {isa = PBXBuildFile; productRef = 58FE25E72AA7399D003D1918 /* WireGuardKitTypes */; }; + A959878D2D2D31C500B297AE /* KeychainSettingsStoreMigration.swift in Sources */ = {isa = PBXBuildFile; fileRef = A959878C2D2D31C500B297AE /* KeychainSettingsStoreMigration.swift */; }; A95EEE362B722CD600A8A39B /* TunnelMonitorState.swift in Sources */ = {isa = PBXBuildFile; fileRef = A95EEE352B722CD600A8A39B /* TunnelMonitorState.swift */; }; A95EEE382B722DFC00A8A39B /* PingStats.swift in Sources */ = {isa = PBXBuildFile; fileRef = A95EEE372B722DFC00A8A39B /* PingStats.swift */; }; A970C89D2B29E38C000A7684 /* Socks5UsernamePasswordCommand.swift in Sources */ = {isa = PBXBuildFile; fileRef = A970C89C2B29E38C000A7684 /* Socks5UsernamePasswordCommand.swift */; }; @@ -2122,6 +2123,7 @@ A944F2712B8E02E800473F4C /* libmullvad_ios.a */ = {isa = PBXFileReference; lastKnownFileType = archive.ar; name = libmullvad_ios.a; path = "../target/aarch64-apple-ios/debug/libmullvad_ios.a"; sourceTree = ""; }; A9467E7E2A29DEFE000DC21F /* RelayCacheTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RelayCacheTests.swift; sourceTree = ""; }; A948809A2BC9308D0090A44C /* EphemeralPeerExchangeActor.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = EphemeralPeerExchangeActor.swift; sourceTree = ""; }; + A959878C2D2D31C500B297AE /* KeychainSettingsStoreMigration.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = KeychainSettingsStoreMigration.swift; sourceTree = ""; }; A95EEE352B722CD600A8A39B /* TunnelMonitorState.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TunnelMonitorState.swift; sourceTree = ""; }; A95EEE372B722DFC00A8A39B /* PingStats.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PingStats.swift; sourceTree = ""; }; A970C89C2B29E38C000A7684 /* Socks5UsernamePasswordCommand.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Socks5UsernamePasswordCommand.swift; sourceTree = ""; }; @@ -3458,6 +3460,7 @@ 7A5869B22B5697AC00640D27 /* IPOverride.swift */, 7A5869BA2B56EE9500640D27 /* IPOverrideRepository.swift */, 06410DFD292CE18F00AFC18C /* KeychainSettingsStore.swift */, + A959878C2D2D31C500B297AE /* KeychainSettingsStoreMigration.swift */, 068CE5732927B7A400A068BB /* Migration.swift */, A9D96B192A8247C100A5C673 /* MigrationManager.swift */, 58B2FDD52AA71D2A003EB5C6 /* MullvadSettings.h */, @@ -5720,6 +5723,7 @@ 58B2FDE72AA71D5C003EB5C6 /* SettingsStore.swift in Sources */, 44DD7D2D2B74E44A0005F67F /* QuantumResistanceSettings.swift in Sources */, F08827872B318C840020A383 /* ShadowsocksCipherOptions.swift in Sources */, + A959878D2D2D31C500B297AE /* KeychainSettingsStoreMigration.swift in Sources */, 58B2FDE92AA71D5C003EB5C6 /* SettingsParser.swift in Sources */, F08827892B3192110020A383 /* AccessMethodRepositoryProtocol.swift in Sources */, F050AE5A2B7376F4003F4EDB /* CustomList.swift in Sources */, diff --git a/ios/MullvadVPN/AppDelegate.swift b/ios/MullvadVPN/AppDelegate.swift index 2f78e904760a..92fac4ab5f04 100644 --- a/ios/MullvadVPN/AppDelegate.swift +++ b/ios/MullvadVPN/AppDelegate.swift @@ -163,6 +163,9 @@ class AppDelegate: UIResponder, UIApplicationDelegate, UNUserNotificationCenterD startInitialization(application: application) + // Wait for `getWipeSettingsOperation` to have run before excluding keychain from backups + SettingsManager.excludeKeychainSettingsFromBackups() + return true } diff --git a/ios/MullvadVPNTests/MullvadSettings/InMemorySettingsStore.swift b/ios/MullvadVPNTests/MullvadSettings/InMemorySettingsStore.swift index 1f19b6429c21..b551689390ad 100644 --- a/ios/MullvadVPNTests/MullvadSettings/InMemorySettingsStore.swift +++ b/ios/MullvadVPNTests/MullvadSettings/InMemorySettingsStore.swift @@ -28,4 +28,6 @@ class InMemorySettingsStore: SettingsStore where ThrownError func delete(key: SettingsKey) throws { settings.removeValue(forKey: key) } + + func excludeFromBackup(keys: [MullvadSettings.SettingsKey]) {} }