From 73ddd132353b8aeb713b496800b0bd90b928b60e Mon Sep 17 00:00:00 2001 From: Kayla Galway Date: Fri, 3 Apr 2020 17:53:42 -0600 Subject: [PATCH 1/3] Resolves database access issues if locally stored salt or key is incorrect. --- Shared/Store/BaseDataStore.swift | 62 +++++++++++++++++++++++++++----- 1 file changed, 53 insertions(+), 9 deletions(-) diff --git a/Shared/Store/BaseDataStore.swift b/Shared/Store/BaseDataStore.swift index 21110877d..bb6d0ce35 100644 --- a/Shared/Store/BaseDataStore.swift +++ b/Shared/Store/BaseDataStore.swift @@ -388,13 +388,17 @@ extension BaseDataStore { private func unlockInternal() { guard let loginsStorage = loginsStorage, let loginsKey = loginsKey, - let salt = salt else { return } + let salt = salt, + let loginsDatabasePath = loginsDatabasePath else { return } do { try loginsStorage.ensureUnlockedWithKeyAndSalt(key: loginsKey, salt: salt) self.storageStateSubject.onNext(.Unlocked) } catch let error as LoginsStoreError { pushError(error) + // If we can not access database with current salt and key, need to delete local database and migrate to replacement salt + // This only deletes the local database file, does not delete the user's sync data + _ = handleDatabaseAccessFailure(databasePath: loginsDatabasePath, encryptionKey: loginsKey) } catch let error { NSLog("Unknown error unlocking: \(error)") } @@ -468,13 +472,13 @@ extension BaseDataStore { guard let loginsDatabasePath = loginsDatabasePath, let loginsKey = loginsKey else { return nil } - let key = KeychainKey.salt.rawValue - if keychainWrapper.hasValue(forKey: key, withAccessibility: .afterFirstUnlock) { - return keychainWrapper.string(forKey: key, withAccessibility: .afterFirstUnlock) + let saltKey = KeychainKey.salt.rawValue + if keychainWrapper.hasValue(forKey: saltKey, withAccessibility: .afterFirstUnlock) { + return keychainWrapper.string(forKey: saltKey, withAccessibility: .afterFirstUnlock) } let val = setupPlaintextHeaderAndGetSalt(databasePath: loginsDatabasePath, encryptionKey: loginsKey) - keychainWrapper.set(val, forKey: key, withAccessibility: .afterFirstUnlock) + keychainWrapper.set(val, forKey: saltKey, withAccessibility: .afterFirstUnlock) return val } @@ -487,19 +491,59 @@ extension BaseDataStore { guard let db = loginsStorage as? LoginsStorage else { return createRandomSalt() } - + do { let salt = try db.getDbSaltForKey(key: encryptionKey) try db.migrateToPlaintextHeader(key: encryptionKey, salt: salt) return salt } catch { - print("setupPlaintextHeaderAndGetSalt failed with error: \(error)") self.dispatcher.dispatch(action: SentryAction(title: "setupPlaintextHeaderAndGetSalt failed", error: error, line: nil)) - // the database exists. but we didn't store the salt? return createRandomSalt() } } - + + // Closes database + // Deletes database file + // Creates new database and syncs + private func handleDatabaseAccessFailure(databasePath: String, encryptionKey: String) -> String? { + let saltKey = KeychainKey.salt.rawValue + if keychainWrapper.hasValue(forKey: saltKey, withAccessibility: .afterFirstUnlock) { + keychainWrapper.removeObject(forKey: saltKey) + } + guard let database = loginsStorage as? LoginsStorage else { return nil } + database.close() + do { + if FileManager.default.fileExists(atPath: databasePath) { + try FileManager.default.removeItem(atPath: databasePath) + loginsStorage = nil + return createNewDatabase() + } else { + loginsStorage = nil + return createNewDatabase() + } + } catch { + self.dispatcher.dispatch(action: SentryAction(title: "handleDatabaseAccessFailure failed", error: error, line: nil)) + return nil + } + } + + private func createNewDatabase() -> String? { + guard let encryptionKey = loginsKey else { return nil } + do { + initializeLoginsStorage() + guard let newDatabase = loginsStorage as? LoginsStorage else { return nil } + let salt = createRandomSalt() + try newDatabase.ensureUnlockedWithKeyAndSalt(key: encryptionKey, salt: salt) + let saltKey = KeychainKey.salt.rawValue + keychainWrapper.set(salt, forKey: saltKey, withAccessibility: .afterFirstUnlock) + self.storageStateSubject.onNext(.Unlocked) + return salt + } catch { + self.dispatcher.dispatch(action: SentryAction(title: "handleDatabaseAccessFailure failed", error: error, line: nil)) + return nil + } + } + private func createRandomSalt() -> String { return UUID().uuidString.replacingOccurrences(of: "-", with: "") } From a627322e8f24732fc21ec05065866160ee2cd0ac Mon Sep 17 00:00:00 2001 From: Kayla Galway Date: Mon, 6 Apr 2020 10:49:20 -0600 Subject: [PATCH 2/3] Add error handling to creation of new database. --- Shared/Store/BaseDataStore.swift | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/Shared/Store/BaseDataStore.swift b/Shared/Store/BaseDataStore.swift index bb6d0ce35..a9d88afc4 100644 --- a/Shared/Store/BaseDataStore.swift +++ b/Shared/Store/BaseDataStore.swift @@ -510,16 +510,19 @@ extension BaseDataStore { if keychainWrapper.hasValue(forKey: saltKey, withAccessibility: .afterFirstUnlock) { keychainWrapper.removeObject(forKey: saltKey) } - guard let database = loginsStorage as? LoginsStorage else { return nil } - database.close() do { + if let database = loginsStorage as? LoginsStorage { + database.close() + } if FileManager.default.fileExists(atPath: databasePath) { try FileManager.default.removeItem(atPath: databasePath) loginsStorage = nil - return createNewDatabase() + let newSalt = try createNewDatabase() + return newSalt } else { loginsStorage = nil - return createNewDatabase() + let newSalt = try createNewDatabase() + return newSalt } } catch { self.dispatcher.dispatch(action: SentryAction(title: "handleDatabaseAccessFailure failed", error: error, line: nil)) @@ -527,11 +530,16 @@ extension BaseDataStore { } } - private func createNewDatabase() -> String? { - guard let encryptionKey = loginsKey else { return nil } + enum DatabaseError: Error { + case issueDeletingDatabase(description: String) + case issueCreatingDatabase(description: String) + } + + private func createNewDatabase() throws -> String { + guard let encryptionKey = loginsKey else { throw DatabaseError.issueCreatingDatabase(description: "logins database key is nil") } do { initializeLoginsStorage() - guard let newDatabase = loginsStorage as? LoginsStorage else { return nil } + guard let newDatabase = loginsStorage as? LoginsStorage else { throw DatabaseError.issueCreatingDatabase(description: "initializing new database failed") } let salt = createRandomSalt() try newDatabase.ensureUnlockedWithKeyAndSalt(key: encryptionKey, salt: salt) let saltKey = KeychainKey.salt.rawValue @@ -540,7 +548,7 @@ extension BaseDataStore { return salt } catch { self.dispatcher.dispatch(action: SentryAction(title: "handleDatabaseAccessFailure failed", error: error, line: nil)) - return nil + throw DatabaseError.issueCreatingDatabase(description: "failed to unlock new database with key and salt:\(error)") } } From 411ce7957cb5a7cea56f1cd7fbea7bcd959eb2f7 Mon Sep 17 00:00:00 2001 From: Kayla Galway Date: Mon, 6 Apr 2020 11:05:54 -0600 Subject: [PATCH 3/3] Remove unnecessary return of string from createNewDatabase and handleDatabaseAccessFailure --- Shared/Store/BaseDataStore.swift | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/Shared/Store/BaseDataStore.swift b/Shared/Store/BaseDataStore.swift index a9d88afc4..dbdb4cf1f 100644 --- a/Shared/Store/BaseDataStore.swift +++ b/Shared/Store/BaseDataStore.swift @@ -398,7 +398,7 @@ extension BaseDataStore { pushError(error) // If we can not access database with current salt and key, need to delete local database and migrate to replacement salt // This only deletes the local database file, does not delete the user's sync data - _ = handleDatabaseAccessFailure(databasePath: loginsDatabasePath, encryptionKey: loginsKey) + handleDatabaseAccessFailure(databasePath: loginsDatabasePath, encryptionKey: loginsKey) } catch let error { NSLog("Unknown error unlocking: \(error)") } @@ -505,7 +505,7 @@ extension BaseDataStore { // Closes database // Deletes database file // Creates new database and syncs - private func handleDatabaseAccessFailure(databasePath: String, encryptionKey: String) -> String? { + private func handleDatabaseAccessFailure(databasePath: String, encryptionKey: String) { let saltKey = KeychainKey.salt.rawValue if keychainWrapper.hasValue(forKey: saltKey, withAccessibility: .afterFirstUnlock) { keychainWrapper.removeObject(forKey: saltKey) @@ -517,16 +517,13 @@ extension BaseDataStore { if FileManager.default.fileExists(atPath: databasePath) { try FileManager.default.removeItem(atPath: databasePath) loginsStorage = nil - let newSalt = try createNewDatabase() - return newSalt + try createNewDatabase() } else { loginsStorage = nil - let newSalt = try createNewDatabase() - return newSalt + try createNewDatabase() } } catch { self.dispatcher.dispatch(action: SentryAction(title: "handleDatabaseAccessFailure failed", error: error, line: nil)) - return nil } } @@ -535,7 +532,7 @@ extension BaseDataStore { case issueCreatingDatabase(description: String) } - private func createNewDatabase() throws -> String { + private func createNewDatabase() throws { guard let encryptionKey = loginsKey else { throw DatabaseError.issueCreatingDatabase(description: "logins database key is nil") } do { initializeLoginsStorage() @@ -545,7 +542,6 @@ extension BaseDataStore { let saltKey = KeychainKey.salt.rawValue keychainWrapper.set(salt, forKey: saltKey, withAccessibility: .afterFirstUnlock) self.storageStateSubject.onNext(.Unlocked) - return salt } catch { self.dispatcher.dispatch(action: SentryAction(title: "handleDatabaseAccessFailure failed", error: error, line: nil)) throw DatabaseError.issueCreatingDatabase(description: "failed to unlock new database with key and salt:\(error)")