Skip to content

Commit 7ae00b8

Browse files
QuantumExplorerclaude
QuantumExplorer
and
claude
committed
fix(swift-sdk): guard legacy keychain sweep by metadata.walletId
`deleteIdentityPrivateKey(walletId:derivationPath:)` was deleting `identity_privkey.<path>` (the legacy, non-namespaced account) unconditionally. The legacy format collided across wallets — that is the bug we are fixing — so an unconditional delete could clobber a different wallet's row at the same DIP-9 path. Promoted `WalletKeyHealthSheet.cleanupLegacyKeychainEntry` (which already did the right thing — lookup metadata, decode, match walletId, delete) into a public `KeychainManager` helper `deleteLegacyKeychainEntryIfOwnedByWallet(walletIdHex:derivationPath:)`. Both `deleteIdentityPrivateKey` and the post-rederive cleanup in `WalletKeyHealthSheet` now route through it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 68f98ad commit 7ae00b8

2 files changed

Lines changed: 108 additions & 65 deletions

File tree

‎packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift‎

Lines changed: 100 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -795,37 +795,117 @@ extension KeychainManager {
795795
/// `identity_privkey.<walletId>.<path>` account scheme).
796796
///
797797
/// Idempotent; returns true on success or "not found". A
798-
/// best-effort sweep of the legacy
799-
/// (`identity_privkey.<path>` — no walletId) account is
800-
/// included so callers don't end up with a half-migrated state
801-
/// where the new-format row is gone but a legacy row at the
802-
/// same path lingers.
798+
/// guarded sweep of the legacy (`identity_privkey.<path>` —
799+
/// no walletId) account is also included so callers don't end
800+
/// up with a half-migrated state where the new-format row is
801+
/// gone but a legacy row at the same path lingers.
802+
///
803+
/// SAFETY: the legacy account format collided across wallets
804+
/// (the very bug we are fixing by namespacing the new path),
805+
/// so deleting a `identity_privkey.<path>` row unconditionally
806+
/// could wipe a DIFFERENT wallet's secret if the two wallets
807+
/// happened to derive an identity at the same DIP-9 path. The
808+
/// sweep therefore looks up the legacy row, decodes its
809+
/// metadata blob, and only deletes when
810+
/// `metadata.walletId == walletIdHex`. Pathological in
811+
/// practice but cheap to defend against.
803812
@discardableResult
804813
public nonisolated func deleteIdentityPrivateKey(
805814
walletId: Data,
806815
derivationPath: String
807816
) -> Bool {
808817
let walletIdHex = walletId.toHexString()
809818
let newAccount = "identity_privkey.\(walletIdHex).\(derivationPath)"
810-
let legacyAccount = "identity_privkey.\(derivationPath)"
811-
var ok = true
812-
for account in [newAccount, legacyAccount] {
813-
var query: [String: Any] = [
814-
kSecClass as String: kSecClassGenericPassword,
815-
kSecAttrService as String: serviceName,
816-
kSecAttrAccount as String: account,
817-
]
818-
if let accessGroup = accessGroup {
819-
query[kSecAttrAccessGroup as String] = accessGroup
820-
}
821-
let status = SecItemDelete(query as CFDictionary)
822-
if status != errSecSuccess && status != errSecItemNotFound {
823-
ok = false
824-
}
819+
var newQuery: [String: Any] = [
820+
kSecClass as String: kSecClassGenericPassword,
821+
kSecAttrService as String: serviceName,
822+
kSecAttrAccount as String: newAccount,
823+
]
824+
if let accessGroup = accessGroup {
825+
newQuery[kSecAttrAccessGroup as String] = accessGroup
826+
}
827+
let newStatus = SecItemDelete(newQuery as CFDictionary)
828+
var ok = newStatus == errSecSuccess || newStatus == errSecItemNotFound
829+
830+
if !deleteLegacyKeychainEntryIfOwnedByWallet(
831+
walletIdHex: walletIdHex,
832+
derivationPath: derivationPath
833+
) {
834+
// Legacy sweep had a non-recoverable error (something
835+
// other than "not found" / "different wallet's row").
836+
// The new-format delete already happened, so the
837+
// caller's primary intent landed — surface the legacy
838+
// failure via the return value so anyone treating it
839+
// as a strict success can react.
840+
ok = false
825841
}
826842
return ok
827843
}
828844

845+
/// Best-effort delete of the legacy (no-walletId) keychain
846+
/// entry for `derivationPath`, gated on its decoded
847+
/// `IdentityPrivateKeyMetadata.walletId` matching `walletIdHex`.
848+
/// No-op when the row doesn't exist or belongs to a different
849+
/// wallet. Returns `false` only on a genuine Keychain error
850+
/// (NOT for "not found" or "different wallet's data") so the
851+
/// caller can distinguish "nothing to do" from "tried + failed".
852+
///
853+
/// Shared by both `deleteIdentityPrivateKey` (per-key delete)
854+
/// and `WalletKeyHealthSheet.cleanupLegacyKeychainEntry`
855+
/// (post-rederive cleanup) so both call sites apply the same
856+
/// safety guard.
857+
public nonisolated func deleteLegacyKeychainEntryIfOwnedByWallet(
858+
walletIdHex: String,
859+
derivationPath: String
860+
) -> Bool {
861+
let legacyAccount = "identity_privkey.\(derivationPath)"
862+
var lookupQuery: [String: Any] = [
863+
kSecClass as String: kSecClassGenericPassword,
864+
kSecAttrService as String: serviceName,
865+
kSecAttrAccount as String: legacyAccount,
866+
kSecReturnAttributes as String: true,
867+
kSecMatchLimit as String: kSecMatchLimitOne,
868+
]
869+
if let accessGroup = accessGroup {
870+
lookupQuery[kSecAttrAccessGroup as String] = accessGroup
871+
}
872+
873+
var result: AnyObject?
874+
let lookupStatus = SecItemCopyMatching(lookupQuery as CFDictionary, &result)
875+
if lookupStatus == errSecItemNotFound {
876+
return true
877+
}
878+
guard lookupStatus == errSecSuccess, let attrs = result as? [String: Any] else {
879+
return false
880+
}
881+
guard let metadataData = attrs[kSecAttrGeneric as String] as? Data,
882+
let metadata = try? JSONDecoder().decode(
883+
IdentityPrivateKeyMetadata.self,
884+
from: metadataData
885+
)
886+
else {
887+
// Row exists but metadata is malformed / missing. Be
888+
// conservative: refuse to delete data we can't prove
889+
// we own. Treat as success — we successfully decided
890+
// not to delete.
891+
return true
892+
}
893+
guard metadata.walletId.caseInsensitiveCompare(walletIdHex) == .orderedSame else {
894+
return true
895+
}
896+
897+
var deleteQuery: [String: Any] = [
898+
kSecClass as String: kSecClassGenericPassword,
899+
kSecAttrService as String: serviceName,
900+
kSecAttrAccount as String: legacyAccount,
901+
]
902+
if let accessGroup = accessGroup {
903+
deleteQuery[kSecAttrAccessGroup as String] = accessGroup
904+
}
905+
let deleteStatus = SecItemDelete(deleteQuery as CFDictionary)
906+
return deleteStatus == errSecSuccess || deleteStatus == errSecItemNotFound
907+
}
908+
829909
/// Delete every `identity_privkey.*` keychain row whose
830910
/// `IdentityPrivateKeyMetadata.identityId` matches
831911
/// `identityIdBase58` — the base58 identity id Swift uses

‎packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletKeyHealthSheet.swift‎

Lines changed: 8 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -100,59 +100,22 @@ fileprivate func namespacedKeychainAccount(walletIdHex: String, derivationPath:
100100
"identity_privkey.\(walletIdHex).\(derivationPath)"
101101
}
102102

103-
/// Construct the OLD (no-walletId) keychain account name. Used only
104-
/// for the post-rederive cleanup sweep.
105-
fileprivate func legacyKeychainAccount(derivationPath: String) -> String {
106-
"identity_privkey.\(derivationPath)"
107-
}
108-
109103
/// Best-effort delete of the legacy (no-walletId) keychain entry for
110104
/// `derivationPath`, gated on its metadata's `walletId` matching the
111-
/// wallet we just migrated from. Skips silently if the metadata says
112-
/// the row belongs to a different wallet — never clobber data we
113-
/// don't own. No-op when the row doesn't exist.
105+
/// wallet we just migrated from. Delegates to
106+
/// `KeychainManager.deleteLegacyKeychainEntryIfOwnedByWallet` so the
107+
/// "don't clobber data we don't own" safety check is centralized —
108+
/// shared with `KeychainManager.deleteIdentityPrivateKey`'s sweep.
114109
///
115110
/// Called after a successful re-derive in
116111
/// `WalletKeyHealthChecker.rederive` so the keychain ends up with a
117112
/// single new-format entry per `(wallet, path)`.
118113
@MainActor
119114
fileprivate func cleanupLegacyKeychainEntry(walletIdHex: String, derivationPath: String) {
120-
let account = legacyKeychainAccount(derivationPath: derivationPath)
121-
let serviceName = KeychainManager.shared.serviceName
122-
let lookupQuery: [String: Any] = [
123-
kSecClass as String: kSecClassGenericPassword,
124-
kSecAttrService as String: serviceName,
125-
kSecAttrAccount as String: account,
126-
kSecReturnAttributes as String: true,
127-
kSecMatchLimit as String: kSecMatchLimitOne,
128-
]
129-
130-
var result: AnyObject?
131-
let lookupStatus = SecItemCopyMatching(lookupQuery as CFDictionary, &result)
132-
guard lookupStatus == errSecSuccess, let attrs = result as? [String: Any] else {
133-
return
134-
}
135-
guard let metadataData = attrs[kSecAttrGeneric as String] as? Data,
136-
let metadata = try? JSONDecoder().decode(
137-
IdentityPrivateKeyMetadata.self,
138-
from: metadataData
139-
)
140-
else {
141-
return
142-
}
143-
guard metadata.walletId.caseInsensitiveCompare(walletIdHex) == .orderedSame else {
144-
// Different wallet's data sitting at the legacy account —
145-
// leave it. (Pathological since we'd have to have lost the
146-
// namespaced fix at some point, but defending it is cheap.)
147-
return
148-
}
149-
150-
let deleteQuery: [String: Any] = [
151-
kSecClass as String: kSecClassGenericPassword,
152-
kSecAttrService as String: serviceName,
153-
kSecAttrAccount as String: account,
154-
]
155-
_ = SecItemDelete(deleteQuery as CFDictionary)
115+
_ = KeychainManager.shared.deleteLegacyKeychainEntryIfOwnedByWallet(
116+
walletIdHex: walletIdHex,
117+
derivationPath: derivationPath
118+
)
156119
}
157120

158121
/// Pure helpers — no UI state. Constructed lazily inside the sheet's

0 commit comments

Comments
 (0)