From e5c63508dd6488b7f7b18a6e8b9e0056d57c2053 Mon Sep 17 00:00:00 2001 From: Nick Cooke <36927374+ncooke3@users.noreply.github.com> Date: Sun, 15 Sep 2024 14:20:46 -0400 Subject: [PATCH 1/4] [Auth] Handle corrupt keychain value resulting from incomplete v11 port --- .../SystemService/AuthStoredUserManager.swift | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/FirebaseAuth/Sources/Swift/SystemService/AuthStoredUserManager.swift b/FirebaseAuth/Sources/Swift/SystemService/AuthStoredUserManager.swift index b9471dc891f..e6d6bb82753 100644 --- a/FirebaseAuth/Sources/Swift/SystemService/AuthStoredUserManager.swift +++ b/FirebaseAuth/Sources/Swift/SystemService/AuthStoredUserManager.swift @@ -119,7 +119,19 @@ class AuthStoredUserManager { archiver.encode(user, forKey: Self.storedUserCoderKey) archiver.finishEncoding() - try keychainServices.setItem(archiver.encodedData, withQuery: query) + do { + try keychainServices.setItem(archiver.encodedData, withQuery: query) + } catch let error as NSError { + // 11.0.0 ≤ version < 11.3.0 + guard !shareAuthStateAcrossDevices, + error.localizedFailureReason == "SecItemAdd (-25299)" else { + throw error + } + let corruptQuery = query + .merging([kSecAttrAccessible as String: kSecAttrAccessibleAfterFirstUnlock]) { $1 } + try keychainServices.removeItem(query: corruptQuery) + try keychainServices.setItem(archiver.encodedData, withQuery: query) + } } /// Remove the user that stored locally. From 89f7b67976a61b1f9795fc67ac978a17e31f81fd Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Wed, 18 Sep 2024 12:16:08 -0400 Subject: [PATCH 2/4] Add documentation for the issue --- .../SystemService/AuthStoredUserManager.swift | 71 ++++++++++++++++++- 1 file changed, 69 insertions(+), 2 deletions(-) diff --git a/FirebaseAuth/Sources/Swift/SystemService/AuthStoredUserManager.swift b/FirebaseAuth/Sources/Swift/SystemService/AuthStoredUserManager.swift index e6d6bb82753..f9a177b596a 100644 --- a/FirebaseAuth/Sources/Swift/SystemService/AuthStoredUserManager.swift +++ b/FirebaseAuth/Sources/Swift/SystemService/AuthStoredUserManager.swift @@ -119,14 +119,81 @@ class AuthStoredUserManager { archiver.encode(user, forKey: Self.storedUserCoderKey) archiver.finishEncoding() + // In Firebase 10, the below query contained the `kSecAttrSynchronizable` + // key set to `true` when `shareAuthStateAcrossDevices == true`. This + // allows a user entry to be shared across devices via the iCloud keychain. + // For the purpose of this discussion, such a user entry will be referred + // to as a "iCloud entry". Conversely, a "non-iCloud entry" will refer to a + // user entry stored when `shareAuthStateAcrossDevices == false`. Keep in + // mind that this class exclusively manages user entries stored in + // device-specific keychain access groups, so both iCloud and non-iCloud + // entries are implicitly available at the device level to apps that + // have access rights to the specific keychain access group used. + // + // The iCloud/non-iCloud distinction is important because entries stored + // with `kSecAttrSynchronizable == true` can only be retrieved when the + // search query includes `kSecAttrSynchronizable == true`. Likewise, + // entries stored without the `kSecAttrSynchronizable` key (or + // `kSecAttrSynchronizable == false`) can only be retrieved when + // the search query omits `kSecAttrSynchronizable` or sets it to `false`. + // + // So for each access group, the SDK manages up to two buckets in the + // keychain, one for iCloud entries and one for non-iCloud entries. + // + // From Firebase 11.0.0 up to but not including 11.3.0, the + // `kSecAttrSynchronizable` key was *not* included in the query when + // `shareAuthStateAcrossDevices == true`. This had the effect of the iCloud + // bucket being inaccessible, and iCloud and non-iCloud entries attempting + // to be written to the same bucket. This was problematic because the + // two types of entries use another flag, the `kSecAttrAccessible` flag, + // with different values. If two queries are identical apart from different + // values for their `kSecAttrAccessible` key, whichever query written to + // the keychain first won't be accessible for reading or updating via the + // other query (resulting in a OSStatus of -25300 indicating the queried + // item cannot be found). And worse, attempting to write the other query to + // the keychain won't work because the write will conflict with the + // previously written query (resulting in a OSStatus of -25299 indicating a + // duplicate item already exists in the keychain). This formed the basis + // for the issues this bug caused. + // + // The missing key was added back in 11.3, but adding back the key + // introduced a new issue. If the buggy version succeeded at writing an + // iCloud entry to the non-iCloud bucket (e.g. keychain was empty before + // iCloud entry was written), then all future non-iCloud writes would fail + // due to the mismatching `kSecAttrAccessible` flag and throw an + // unrecoverable error. To address this the below error handling is used to + // detect such cases,remove the "corrupt" iCloud entry stored by the buggy + // version in the non-iCloud bucket, and retry writing the current + // non-iCloud entry again. do { try keychainServices.setItem(archiver.encodedData, withQuery: query) } catch let error as NSError { - // 11.0.0 ≤ version < 11.3.0 - guard !shareAuthStateAcrossDevices, + guard shareAuthStateAcrossDevices == false, error.localizedFailureReason == "SecItemAdd (-25299)" else { + // The error is not related to the 11.0 - 11.2, and should be rethrown. throw error } + // We are trying to write a non-iCloud entry but a corrupt iCloud entry + // is likely preventing it from happening. + // + // The corrupt query was supposed to contain the following keys: + // { + // kSecAttrSynchronizable: true, + // kSecAttrAccessible: kSecAttrAccessibleAfterFirstUnlock + // } + // Instead, it contained: + // { + // kSecAttrAccessible: kSecAttrAccessibleAfterFirstUnlock + // } + // + // Excluding `kSecAttrSynchronizable` treats the query as if it's false + // and the entry won't be shared in iCloud across devices. It is instead + // written to the non-iCloud bucket. This query is corrupting the + // non-iCloud bucket because its `kSecAttrAccessible` value is not + // compatible with the value used for non-iCloud entries. To delete it, + // a compatible query is formed by swapping the accessibility flag + // out for `kSecAttrAccessibleAfterFirstUnlock`. This frees up the bucket + // so the non-iCloud entry can attempt to be written again. let corruptQuery = query .merging([kSecAttrAccessible as String: kSecAttrAccessibleAfterFirstUnlock]) { $1 } try keychainServices.removeItem(query: corruptQuery) From d114ee0af85296b68e815f20285b4868380a412c Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Wed, 18 Sep 2024 12:21:16 -0400 Subject: [PATCH 3/4] fix typo --- .../Sources/Swift/SystemService/AuthStoredUserManager.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/FirebaseAuth/Sources/Swift/SystemService/AuthStoredUserManager.swift b/FirebaseAuth/Sources/Swift/SystemService/AuthStoredUserManager.swift index f9a177b596a..8f9c58d0547 100644 --- a/FirebaseAuth/Sources/Swift/SystemService/AuthStoredUserManager.swift +++ b/FirebaseAuth/Sources/Swift/SystemService/AuthStoredUserManager.swift @@ -162,7 +162,7 @@ class AuthStoredUserManager { // iCloud entry was written), then all future non-iCloud writes would fail // due to the mismatching `kSecAttrAccessible` flag and throw an // unrecoverable error. To address this the below error handling is used to - // detect such cases,remove the "corrupt" iCloud entry stored by the buggy + // detect such cases, remove the "corrupt" iCloud entry stored by the buggy // version in the non-iCloud bucket, and retry writing the current // non-iCloud entry again. do { From d2b096d151a17a8600648c931b961504e91cd86c Mon Sep 17 00:00:00 2001 From: Nick Cooke <36927374+ncooke3@users.noreply.github.com> Date: Wed, 18 Sep 2024 18:41:25 -0400 Subject: [PATCH 4/4] Review --- .../Sources/Swift/SystemService/AuthStoredUserManager.swift | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/FirebaseAuth/Sources/Swift/SystemService/AuthStoredUserManager.swift b/FirebaseAuth/Sources/Swift/SystemService/AuthStoredUserManager.swift index 8f9c58d0547..5517ddc3fa1 100644 --- a/FirebaseAuth/Sources/Swift/SystemService/AuthStoredUserManager.swift +++ b/FirebaseAuth/Sources/Swift/SystemService/AuthStoredUserManager.swift @@ -170,7 +170,8 @@ class AuthStoredUserManager { } catch let error as NSError { guard shareAuthStateAcrossDevices == false, error.localizedFailureReason == "SecItemAdd (-25299)" else { - // The error is not related to the 11.0 - 11.2, and should be rethrown. + // The error is not related to the 11.0 - 11.2 issue described above, + // and should be rethrown. throw error } // We are trying to write a non-iCloud entry but a corrupt iCloud entry