Skip to content

Commit

Permalink
Fixing issue #184 - Improve handling of potential keychain errors
Browse files Browse the repository at this point in the history
  • Loading branch information
@robbiehanson
robbiehanson committed Jul 20, 2021
1 parent c07d161 commit 5e61029
Show file tree
Hide file tree
Showing 7 changed files with 478 additions and 39 deletions.
4 changes: 4 additions & 0 deletions phoenix-ios/phoenix-ios.xcodeproj/project.pbxproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
DC142135261E72320075857A /* AboutHTML.swift in Sources */ = {isa = PBXBuildFile; fileRef = DC142134261E72320075857A /* AboutHTML.swift */; };
DC142140261E72E40075857A /* AnyHTML.swift in Sources */ = {isa = PBXBuildFile; fileRef = DC14213F261E72E40075857A /* AnyHTML.swift */; };
DC142148261F5DCE0075857A /* AdvancedSecurityHTML.swift in Sources */ = {isa = PBXBuildFile; fileRef = DC142147261F5DCE0075857A /* AdvancedSecurityHTML.swift */; };
DC1706D926A71D8E00BAFCD0 /* ErrorView.swift in Sources */ = {isa = PBXBuildFile; fileRef = DC1706D826A71D8E00BAFCD0 /* ErrorView.swift */; };
DC18C418256FE22300A2D083 /* Prefs.swift in Sources */ = {isa = PBXBuildFile; fileRef = DC18C417256FE22300A2D083 /* Prefs.swift */; };
DC18C41D256FF91100A2D083 /* Utils.swift in Sources */ = {isa = PBXBuildFile; fileRef = DC18C41C256FF91100A2D083 /* Utils.swift */; };
DC1D2B4B2593EB860036AD38 /* CurrencyUnit.swift in Sources */ = {isa = PBXBuildFile; fileRef = DC1D2B4A2593EB850036AD38 /* CurrencyUnit.swift */; };
Expand Down Expand Up @@ -179,6 +180,7 @@
DC142134261E72320075857A /* AboutHTML.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AboutHTML.swift; sourceTree = "<group>"; };
DC14213F261E72E40075857A /* AnyHTML.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AnyHTML.swift; sourceTree = "<group>"; };
DC142147261F5DCE0075857A /* AdvancedSecurityHTML.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AdvancedSecurityHTML.swift; sourceTree = "<group>"; };
DC1706D826A71D8E00BAFCD0 /* ErrorView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ErrorView.swift; sourceTree = "<group>"; };
DC18C417256FE22300A2D083 /* Prefs.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Prefs.swift; sourceTree = "<group>"; };
DC18C41C256FF91100A2D083 /* Utils.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Utils.swift; sourceTree = "<group>"; };
DC1D2B4A2593EB850036AD38 /* CurrencyUnit.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CurrencyUnit.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -325,6 +327,7 @@
53BEFBC93C11EF306B9EB33A /* ReceiveView.swift */,
53BEFCB8A24A5FAA785AFD03 /* SendView.swift */,
53BEF4DAE061532668494988 /* PaymentView.swift */,
DC1706D826A71D8E00BAFCD0 /* ErrorView.swift */,
DCFA8757260E6DFF00AE8953 /* onboarding */,
C8D7A7335040D755924F8FFC /* configuration */,
53BEF0E7A62D4973FCC99476 /* widgets */,
Expand Down Expand Up @@ -776,6 +779,7 @@
7555FF7F242A565900829871 /* AppDelegate.swift in Sources */,
DC142135261E72320075857A /* AboutHTML.swift in Sources */,
DC649FDC2577002300853C46 /* KotlinExtensions.swift in Sources */,
DC1706D926A71D8E00BAFCD0 /* ErrorView.swift in Sources */,
7555FF81242A565900829871 /* SceneDelegate.swift in Sources */,
DCE1E5FA26418183005465B8 /* Toast.swift in Sources */,
DC81B79F25BF2AA200F5A52C /* MVI.swift in Sources */,
Expand Down
43 changes: 39 additions & 4 deletions phoenix-ios/phoenix-ios/SceneDelegate.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ class SceneDelegate: UIResponder, UIWindowSceneDelegate {
var cancellables = Set<AnyCancellable>()

var lockWindow: UIWindow?
var errorWindow: UIWindow?

let lockState = LockState(isUnlocked: false)

Expand Down Expand Up @@ -193,7 +194,8 @@ class SceneDelegate: UIResponder, UIWindowSceneDelegate {
}
}.store(in: &cancellables)

AppSecurity.shared.tryUnlockWithKeychain {(mnemonics: [String]?, enabledSecurity: EnabledSecurity) in
AppSecurity.shared.tryUnlockWithKeychain {
(mnemonics: [String]?, enabledSecurity: EnabledSecurity, danger: LossOfSeedDanger?) in

// There are multiple potential configurations:
//
Expand All @@ -202,15 +204,17 @@ class SceneDelegate: UIResponder, UIWindowSceneDelegate {
// - advanced security => mnemonics are not available, enabledSecurity is non-empty
//
// Another way to think about it:
// - standard security => touchID only protects the UI, wallet can immediately be loaded
// - advanced security => touchID required to unlock both the UI and the seed
// - standard security => biometrics only protect the UI, wallet can immediately be loaded
// - advanced security => biometrics required to unlock both the UI and the seed

if let mnemonics = mnemonics {
// unlock & load wallet
AppDelegate.get().loadWallet(mnemonics: mnemonics)
}

if enabledSecurity.isEmpty {
if let danger = danger {
self.showErrorWindow(danger)
} else if enabledSecurity.isEmpty {
self.lockState.isUnlocked = true
} else {
self.showLockWindow()
Expand All @@ -225,6 +229,9 @@ class SceneDelegate: UIResponder, UIWindowSceneDelegate {
private func showLockWindow() {
log.trace("showLockWindow()")

guard errorWindow == nil else {
return
}
guard let windowScene = self.window?.windowScene else {
return
}
Expand All @@ -249,6 +256,10 @@ class SceneDelegate: UIResponder, UIWindowSceneDelegate {
private func hideLockWindow() {
log.trace("hideLockWindow()")

guard errorWindow == nil else {
return
}

// Make window the responder for touch events & other input.
window?.makeKey()

Expand All @@ -270,5 +281,29 @@ class SceneDelegate: UIResponder, UIWindowSceneDelegate {
}
}
}

private func showErrorWindow(_ danger: LossOfSeedDanger) {
log.trace("showErrorWindow()")

guard let windowScene = self.window?.windowScene else {
return
}

if errorWindow == nil {

let errorView = ErrorView(danger: danger)

let controller = UIHostingController(rootView: errorView)
controller.view.backgroundColor = .clear

errorWindow = UIWindow(windowScene: windowScene)
errorWindow?.rootViewController = controller
errorWindow?.tintColor = UIColor.appAccent
errorWindow?.overrideUserInterfaceStyle = Prefs.shared.theme.toInterfaceStyle()
errorWindow?.windowLevel = .alert + 1
}

errorWindow?.makeKeyAndVisible()
}
}

178 changes: 147 additions & 31 deletions phoenix-ios/phoenix-ios/security/AppSecurity.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,31 @@ enum BiometricSupport {
}
}

enum ReadSecurityFileError: Error {
case fileNotFound
case errorReadingFile(underlying: Error)
case errorDecodingFile(underlying: Error)
}

enum ReadKeychainError: Error {
case keychainOptionNotEnabled
case keychainBoxCorrupted(underlying: Error)
case errorReadingKey(underlying: Error)
case keyNotFound
case errorOpeningBox(underlying: Error)
case invalidMnemonics
}

struct LossOfSeedDanger {
let readSecurityFileError: ReadSecurityFileError?
let readKeychainError: ReadKeychainError?

init(_ readSecurityFileError: ReadSecurityFileError?, _ readKeychainError: ReadKeychainError?) {
self.readSecurityFileError = readSecurityFileError
self.readKeychainError = readKeychainError
}
}

// Names of entries stored within the OS keychain:
private let keychain_accountName_keychain = "securityFile_keychain"
private let keychain_accountName_biometrics = "securityFile_biometrics"
Expand Down Expand Up @@ -79,17 +104,41 @@ class AppSecurity {

/// Performs disk IO - prefer use in background thread.
///
private func readFromDisk() -> SecurityFile {
private func safeReadFromDisk() -> Result<SecurityFile, ReadSecurityFileError> {

let fileUrl = self.securityJsonUrl

if !FileManager.default.fileExists(atPath: fileUrl.path) {
return .failure(.fileNotFound)
}

var result: SecurityFile? = nil
let data: Data
do {
data = try Data(contentsOf: fileUrl)
} catch {
log.error("safeReadFromDisk(): error reading file: \(String(describing: error))")
return .failure(.errorReadingFile(underlying: error))
}

let result: SecurityFile
do {
let data = try Data(contentsOf: self.securityJsonUrl)
result = try JSONDecoder().decode(SecurityFile.self, from: data)
} catch {
// NB: in the event of various failures, we rely on the `createBackup` system.
log.error("safeReadFromDisk(): error decoding file: \(String(describing: error))")
return .failure(.errorDecodingFile(underlying: error))
}

return result ?? SecurityFile()
return .success(result)
}

/// Performs disk IO - prefer use in background thread.
///
private func readFromDisk() -> SecurityFile {

let result = safeReadFromDisk()
let securityFile = try? result.get()

return securityFile ?? SecurityFile()
}

/// Performs disk IO - prefer use in background thread.
Expand Down Expand Up @@ -241,13 +290,38 @@ class AppSecurity {
/// Otherwise it will fail, and the completion closure will specify the additional security in place.
///
public func tryUnlockWithKeychain(
completion: @escaping (_ mnemonics: [String]?, _ configuration: EnabledSecurity) -> Void
completion: @escaping (
_ mnemonics: [String]?,
_ configuration: EnabledSecurity,
_ danger: LossOfSeedDanger?
) -> Void
) {

let finish = {(_ mnemonics: [String]?, _ configuration: EnabledSecurity) -> Void in
let finish = {(mnemonics: [String]?, configuration: EnabledSecurity) -> Void in

DispatchQueue.main.async {
self.enabledSecurity.send(configuration)
completion(mnemonics, configuration)
completion(mnemonics, configuration, nil)
}
}

let dangerZone1 = {(error: ReadSecurityFileError, configuration: EnabledSecurity) -> Void in

let danger = LossOfSeedDanger(error, nil)

DispatchQueue.main.async {
self.enabledSecurity.send(configuration)
completion(nil, configuration, danger)
}
}

let dangerZone2 = {(error: ReadKeychainError, configuration: EnabledSecurity) -> Void in

let danger = LossOfSeedDanger(nil, error)

DispatchQueue.main.async {
self.enabledSecurity.send(configuration)
completion(nil, configuration, danger)
}
}

Expand All @@ -257,25 +331,64 @@ class AppSecurity {

// Fetch the "security.json" file.
// If the file doesn't exist, an empty SecurityFile is returned.
let securityFile = self.readFromDisk()
let diskResult = self.safeReadFromDisk()

let result = self.readKeychainEntry(securityFile)
let enabledSecurity = self.calculateEnabledSecurity(securityFile)

let mnemonics = try? result.get()
finish(mnemonics, enabledSecurity)
switch diskResult {
case .failure(let reason):

let securityFile = SecurityFile()
let configuration = self.calculateEnabledSecurity(securityFile)

switch reason {
case .fileNotFound:
return finish(nil, configuration)

case .errorReadingFile: fallthrough
case .errorDecodingFile:
return dangerZone1(reason, configuration)
}

case .success(let securityFile):

let configuration = self.calculateEnabledSecurity(securityFile)

let keychainResult = self.readKeychainEntry(securityFile)
switch keychainResult {
case .failure(let reason):

switch reason {
case .keychainOptionNotEnabled:
return finish(nil, configuration)

case .keychainBoxCorrupted: fallthrough
case .errorReadingKey: fallthrough
case .keyNotFound: fallthrough
case .errorOpeningBox: fallthrough
case .invalidMnemonics:
return dangerZone2(reason, configuration)
}

case .success(let mnemonics):
return finish(mnemonics, configuration)
}
}
}
}

private func readKeychainEntry(_ securityFile: SecurityFile) -> Result<[String], Error> {
private func readKeychainEntry(_ securityFile: SecurityFile) -> Result<[String], ReadKeychainError> {

// The securityFile tells us which security options have been enabled.
// If there isn't a keychain entry, then we cannot unlock the seed.
guard
let keyInfo = securityFile.keychain as? KeyInfo_ChaChaPoly,
let sealedBox = try? keyInfo.toSealedBox()
else {
return .failure(genericError(401, "SecurityFile doesn't have keychain entry"))
guard let keyInfo = securityFile.keychain as? KeyInfo_ChaChaPoly else {
return .failure(.keychainOptionNotEnabled)
}

let sealedBox: ChaChaPoly.SealedBox
do {
sealedBox = try keyInfo.toSealedBox()
} catch {
log.error("readKeychainEntry(): error: keychainBoxCorrupted: \(String(describing: error))")
return .failure(.keychainBoxCorrupted(underlying: error))
}

let keychain = GenericPasswordStore()
Expand All @@ -285,24 +398,27 @@ class AppSecurity {
do {
fetchedKey = try keychain.readKey(account: keychain_accountName_keychain)
} catch {
log.error("keychain.readKey(account: keychain): error: \(String(describing: error))")
return .failure(error)
log.error("readKeychainEntry(): error: readingKey: \(String(describing: error))")
return .failure(.errorReadingKey(underlying: error))
}

guard let lockingKey = fetchedKey else {
return .failure(genericError(401, "Keychain entry missing"))
log.error("readKeychainEntry(): error: keyNotFound")
return .failure(.keyNotFound)
}

// Decrypt the databaseKey using the lockingKey
let mnemonicsData: Data
do {
mnemonicsData = try ChaChaPoly.open(sealedBox, using: lockingKey)
} catch {
return .failure(error)
log.error("readKeychainEntry(): error: openingBox: \(String(describing: error))")
return .failure(.errorOpeningBox(underlying: error))
}

guard let mnemonicsString = String(data: mnemonicsData, encoding: .utf8) else {
return .failure(genericError(500, "Keychain data is invalid"))
log.error("readKeychainEntry(): error: invalidMnemonics")
return .failure(.invalidMnemonics)
}

let mnemonics = mnemonicsString.split(separator: " ").map { String($0) }
Expand All @@ -318,8 +434,8 @@ class AppSecurity {
/// - the user is explicitly disabling existing security options
///
public func addKeychainEntry(
mnemonics : [String],
completion : @escaping (_ error: Error?) -> Void
mnemonics : [String],
completion : @escaping (_ error: Error?) -> Void
) {
let mnemonicsData = validateParameter(mnemonics: mnemonics)

Expand Down Expand Up @@ -385,9 +501,9 @@ class AppSecurity {
//
// So we're careful to to perform operations in a particular order here:
//
// - add new entry to OS keychain
// - add new entry to OS keychain (account=keychain)
// - write security.json file to disk
// - then we can safely remove the old entries from the OS keychain
// - then we can safely remove the old entries from the OS keychain (account=biometrics)

let keychain = GenericPasswordStore()

Expand Down Expand Up @@ -686,11 +802,11 @@ class AppSecurity {
return fail(error)
}

let keyInfo_touchID = KeyInfo_ChaChaPoly(sealedBox: sealedBox)
let keyInfo_biometrics = KeyInfo_ChaChaPoly(sealedBox: sealedBox)

let oldSecurityFile = self.readFromDisk()
let securityFile = SecurityFile(
touchID : keyInfo_touchID,
biometrics : keyInfo_biometrics,
passphrase : oldSecurityFile.passphrase // maintain existing option
)

Expand Down
Loading

0 comments on commit 5e61029

Please sign in to comment.