Exclude keychain items from backups

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<T>(_ 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
+    }
 }

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)
+    }
+}

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 {

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])
 }

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 = "<group>"; };
 		A9467E7E2A29DEFE000DC21F /* RelayCacheTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RelayCacheTests.swift; sourceTree = "<group>"; };
 		A948809A2BC9308D0090A44C /* EphemeralPeerExchangeActor.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = EphemeralPeerExchangeActor.swift; sourceTree = "<group>"; };
+		A959878C2D2D31C500B297AE /* KeychainSettingsStoreMigration.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = KeychainSettingsStoreMigration.swift; sourceTree = "<group>"; };
 		A95EEE352B722CD600A8A39B /* TunnelMonitorState.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TunnelMonitorState.swift; sourceTree = "<group>"; };
 		A95EEE372B722DFC00A8A39B /* PingStats.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PingStats.swift; sourceTree = "<group>"; };
 		A970C89C2B29E38C000A7684 /* Socks5UsernamePasswordCommand.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Socks5UsernamePasswordCommand.swift; sourceTree = "<group>"; };
@@ -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 */,

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
     }
 

ios/MullvadVPNTests/MullvadSettings/InMemorySettingsStore.swift

@@ -28,4 +28,6 @@ class InMemorySettingsStore<ThrownError: Error>: SettingsStore where ThrownError
     func delete(key: SettingsKey) throws {
         settings.removeValue(forKey: key)
     }
+
+    func excludeFromBackup(keys: [MullvadSettings.SettingsKey]) {}
 }