-
Notifications
You must be signed in to change notification settings - Fork 53
feat(swift-sdk): obfuscate runtime mnemonic bytes #3545
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,13 @@ | ||||||||||||||||||||||||||||||
| import Foundation | ||||||||||||||||||||||||||||||
| import DashSDKFFI | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| private func scrubMnemonicBytes(_ bytes: inout [UInt8]) { | ||||||||||||||||||||||||||||||
| bytes.withUnsafeMutableBufferPointer { buffer in | ||||||||||||||||||||||||||||||
| guard let base = buffer.baseAddress else { return } | ||||||||||||||||||||||||||||||
| memset_s(base, buffer.count, 0, buffer.count) | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /// Utility class for mnemonic operations | ||||||||||||||||||||||||||||||
| public class Mnemonic { | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
@@ -59,27 +66,49 @@ public class Mnemonic { | |||||||||||||||||||||||||||||
| /// - passphrase: Optional BIP39 passphrase | ||||||||||||||||||||||||||||||
| /// - Returns: The seed data (typically 64 bytes) | ||||||||||||||||||||||||||||||
| public static func toSeed(mnemonic: String, passphrase: String? = nil) throws -> Data { | ||||||||||||||||||||||||||||||
| try toSeed(mnemonicUTF8Bytes: Data(mnemonic.utf8), passphrase: passphrase) | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /// Convert mnemonic UTF-8 bytes to seed without first | ||||||||||||||||||||||||||||||
| /// materializing a Swift `String` at the call site. | ||||||||||||||||||||||||||||||
| public static func toSeed(mnemonicUTF8Bytes: Data, passphrase: String? = nil) throws -> Data { | ||||||||||||||||||||||||||||||
| guard !mnemonicUTF8Bytes.isEmpty else { | ||||||||||||||||||||||||||||||
| throw KeyWalletError.invalidInput("Mnemonic must not be empty") | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| var error = FFIError() | ||||||||||||||||||||||||||||||
| var seed = Data(count: 64) | ||||||||||||||||||||||||||||||
| var seedLen: size_t = 64 | ||||||||||||||||||||||||||||||
| var mnemonicBytes = [UInt8](mnemonicUTF8Bytes) | ||||||||||||||||||||||||||||||
| guard !mnemonicBytes.contains(0) else { | ||||||||||||||||||||||||||||||
| scrubMnemonicBytes(&mnemonicBytes) | ||||||||||||||||||||||||||||||
| throw KeyWalletError.invalidInput("Mnemonic bytes must not contain NUL") | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| mnemonicBytes.append(0) | ||||||||||||||||||||||||||||||
|
Comment on lines
+82
to
+87
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Reserve the final capacity up front so no reallocation occurs: 🔒 Proposed fix- var mnemonicBytes = [UInt8](mnemonicUTF8Bytes)
- guard !mnemonicBytes.contains(0) else {
- scrubMnemonicBytes(&mnemonicBytes)
- throw KeyWalletError.invalidInput("Mnemonic bytes must not contain NUL")
- }
- mnemonicBytes.append(0)
+ var mnemonicBytes = [UInt8]()
+ mnemonicBytes.reserveCapacity(mnemonicUTF8Bytes.count + 1)
+ mnemonicBytes.append(contentsOf: mnemonicUTF8Bytes)
+ guard !mnemonicBytes.contains(0) else {
+ scrubMnemonicBytes(&mnemonicBytes)
+ throw KeyWalletError.invalidInput("Mnemonic bytes must not contain NUL")
+ }
+ mnemonicBytes.append(0)The same pattern would also be worth applying inside 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| let success = mnemonic.withCString { mnemonicCStr in | ||||||||||||||||||||||||||||||
| seed.withUnsafeMutableBytes { seedBytes in | ||||||||||||||||||||||||||||||
| let seedPtr = seedBytes.bindMemory(to: UInt8.self).baseAddress | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| if let passphrase = passphrase { | ||||||||||||||||||||||||||||||
| return passphrase.withCString { passphraseCStr in | ||||||||||||||||||||||||||||||
| mnemonic_to_seed(mnemonicCStr, passphraseCStr, | ||||||||||||||||||||||||||||||
| seedPtr, &seedLen, &error) | ||||||||||||||||||||||||||||||
| let success = mnemonicBytes.withUnsafeBufferPointer { mnemonicBuf in | ||||||||||||||||||||||||||||||
| guard let mnemonicBase = mnemonicBuf.baseAddress else { | ||||||||||||||||||||||||||||||
| return false | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| return mnemonicBase.withMemoryRebound(to: CChar.self, capacity: mnemonicBuf.count) { mnemonicCStr in | ||||||||||||||||||||||||||||||
| seed.withUnsafeMutableBytes { seedBytes in | ||||||||||||||||||||||||||||||
| let seedPtr = seedBytes.bindMemory(to: UInt8.self).baseAddress | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| if let passphrase = passphrase { | ||||||||||||||||||||||||||||||
| return passphrase.withCString { passphraseCStr in | ||||||||||||||||||||||||||||||
| mnemonic_to_seed(mnemonicCStr, passphraseCStr, | ||||||||||||||||||||||||||||||
| seedPtr, &seedLen, &error) | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||
| return mnemonic_to_seed(mnemonicCStr, nil, | ||||||||||||||||||||||||||||||
| seedPtr, &seedLen, &error) | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||
| return mnemonic_to_seed(mnemonicCStr, nil, | ||||||||||||||||||||||||||||||
| seedPtr, &seedLen, &error) | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| defer { | ||||||||||||||||||||||||||||||
| scrubMnemonicBytes(&mnemonicBytes) | ||||||||||||||||||||||||||||||
| if error.message != nil { | ||||||||||||||||||||||||||||||
| error_message_free(error.message) | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preflight
hasMnemonicand runtimeretrieveMnemonicUTF8Bytesdisagree on the "empty stored data" case.retrieveMnemonicUTF8Bytes(for:)throwsmnemonicNotFoundnot just onerrSecItemNotFoundbut also when the keychain returns aDatawhoseisEmptyistrue(Lines 110-112).hasMnemonic(for:)only checksstatus == errSecSuccess(Line 140) — it doesn't request the bytes and doesn't validate any size attribute. So a corrupt-or-zero-byte keychain row makeshasMnemonicreturntruewhile the actual signing path then fails withmnemonicNotFound, which surfaces inKeychainSigner.canSignas a "yes I can sign" lie followed by a sign-time error.Two reasonable fixes:
kSecReturnAttributeswithkSecAttrSize as Stringor usekSecReturnPersistentRef+ a follow-up data length query) and treat size==0 as "no mnemonic". This keepshasMnemonicplaintext-free.Data, but only on the rare preflight callers, and it removes the disagreement entirely.Empty rows shouldn't appear in practice (
storeMnemonicwritesData(mnemonic.utf8)from a non-empty source), but sinceretrieveMnemonicUTF8Bytesactively guards the case, the two APIs should agree on what "has" means.🤖 Prompt for AI Agents