fix(platform-wallet): wallet_id gate on resolver-fed sign entrypoints#3735
Draft
Claudius-Maginificent wants to merge 1 commit into
Draft
fix(platform-wallet): wallet_id gate on resolver-fed sign entrypoints#3735Claudius-Maginificent wants to merge 1 commit into
Claudius-Maginificent wants to merge 1 commit into
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
6 tasks
…ints Four FFI sign entrypoints take a MnemonicResolverHandle + wallet_id_bytes and derive signing material from the resolver-supplied mnemonic. None of them previously checked that the seed actually belongs to wallet_id_bytes — a host calling them with a mismatched (wallet_id, mnemonic) pair would happily derive and sign with the wrong key. This patch closes that gap: - New crate::sign_gate::verify_seed_matches_wallet_id(root_pub, expected_wallet_id) -> bool (constant-time via subtle::ConstantTimeEq, #[must_use]). - New PlatformWalletFFIResultCode::ErrorWrongSeedForWallet = 13 for the structured-result entrypoints. - New SIGN_WITH_RESOLVER_ERR_WRONG_SEED u8 tag for the byte-tagged path. Wired into: 1. sign_with_mnemonic_resolver::dash_sdk_sign_with_mnemonic_resolver_and_path 2. identity_derive_and_persist::dash_sdk_derive_and_persist_identity_keys 3. derive_identity_key_at_slot::dash_sdk_derive_identity_key_at_slot_with_resolver 4. shielded_sync::platform_wallet_manager_bind_shielded (shielded feature) Each call site: invokes the gate BEFORE any derive_priv / signing / persister callback / coordinator handoff; on mismatch zeroes any derived master key (master.private_key.non_secure_erase()), zeros any caller-owned output buffer (e.g. out_signature), and returns the typed tag — the persister callback is never reached, no signature bytes leak. Tests (co-located in each entrypoint's source under #[cfg(test)] mod tests): - sign_with_mnemonic_resolver: happy_path_signs_and_returns_signature + wrong_wallet_id_fails_closed_with_wrong_seed_tag (asserts every byte of sig_buf is zero on mismatch). - identity_derive_and_persist: wrong_wallet_id_fails_closed_before_persisting_anything (asserts persister callback row count is zero). - derive_identity_key_at_slot: matching_seed_returns_derived_key + wrong_wallet_id_fails_closed_with_wrong_seed_tag. - shielded_sync (gated on `shielded` feature): wrong_wallet_id_fails_closed + null_resolver_handle_rejected. - sign_gate::tests: helper unit tests for the CT comparison. Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
65f887d to
ee023a7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why this PR exists
The gap: Four FFI entrypoints in
rs-platform-wallet-ffiaccept aMnemonicResolverHandleplus a caller-suppliedwallet_id_bytes. None of them verifies the resolver-returned mnemonic actually corresponds to thatwallet_id. The resolver is host-controlled opaque state — there is no in-process cross-check.Threat scenario: A host (Swift/iOS or any FFI consumer) calls one of these entrypoints with
wallet_id = W_A, but the registered resolver — whether by host wiring bug, compromised resolver, or malicious resolver — returns the mnemonic for walletW_B. Before this PR, all four entrypoints silently derive from W_B and proceed.Per-site impact (with the gate removed) — confirmed by code audit:
sign_with_mnemonic_resolver_and_pathInvalidSignatureon submit, or L1 mempool reject on asset-lock TX)rc = 0; caller ships it to the network; failure surfaces only after round-tripsign_with_mnemonic_resolver::tests::wrong_wallet_id_fails_closed_with_wrong_seed_tagderive_and_persist_identity_keyswallet_id = W_Abut holding W_B's pubkey/privkey/path. Subsequent registration succeeds with internally-consistent W_B keys, creating an identity W_A's seed cannot sign for. Irrecoverable without manual Keychain surgery.identity_derive_and_persist::tests::wrong_wallet_id_fails_closed_before_persisting_anythingderive_identity_key_at_slot_with_resolverrc = Success; caller uses it freelyderive_identity_key_at_slot::tests::wrong_wallet_id_fails_closed_with_wrong_seed_tagplatform_wallet_manager_bind_shielded(shielded feature)PlatformWallet::bind_shielded(platform_wallet.rs:338-427) usesself.wallet_idand the caller-supplied seed with zero cross-checkSubwalletId { wallet_id: W_A }. Sync flushes W_B's notes into W_A's persister rows; spend paths can authorize W_B's notes under W_A. Cross-wallet fund visibility and spend leakage.shielded_sync::tests::wrong_wallet_id_fails_closed(requires--features shielded)Note that
rs-platform-walletitself does not catch this: pure-Rust callers instantiateWalletobjects whosewallet_idis derived from their own root — the mismatch is unreachable from Rust. The vulnerability is exclusively expressible across the FFI trust boundary, which is why the gate lives inrs-platform-wallet-ffi.Each row's test fails on
v3.1-devwithout this PR's gate and passes once the gate is applied — verified locally against the rebased commit.Prerequisite/blocking relationship: This PR is the logical prerequisite for PR #3692 (seedless watch-only rehydration), which removes the existing load-time wrong-seed gate in
rs-platform-wallet. Without #3735 landing first, #3692 would briefly create a window where any seed could sign for anywallet_id. Decoupled here so the security fix ships tov3.1-devon its own merits.Issue being fixed or feature implemented
Four FFI sign entrypoints in
rs-platform-wallet-ffitake aMnemonicResolverHandleplus awallet_id_bytesarg and derive signing material from the mnemonic the resolver returns. None of them previously checked that the seed actually belongs to that wallet_id. A host that called any of them with a mismatched(wallet_id, mnemonic)pair — whether by host bug or by a malicious resolver — would happily derive and sign with the wrong key.This patch closes that gap.
It also unblocks the seedless-load model in PR #3692: that PR removes a load-time wrong-seed gate, so the sign-time gate has to exist before that change can land without regressing wrong-seed protection. Decoupled here so the security fix can ship to
v3.1-devon its own merits, ahead of the rehydration model rework.What was done?
New helper:
crate::sign_gate::verify_seed_matches_wallet_idpackages/rs-platform-wallet-ffi/src/sign_gate.rs(new):subtle::ConstantTimeEq— no early-return on byte mismatch.#[must_use]so a caller can't silently drop the verdict.subtle = "2"(well-known constant-time-comparison crate, single line added toCargo.lock, no transitive bumps).New result code:
ErrorWrongSeedForWallet = 15PlatformWalletFFIResultCode::ErrorWrongSeedForWallet = 15inerror.rs. Newly allocated slot 15. Slot 13 is taken byErrorArithmeticOverflow(#3549), slot 14 byErrorNoSelectableInputs; 15 was the smallest free integer.For the byte-tagged
sign_with_mnemonic_resolver_and_pathpath, a new u8 tagSIGN_WITH_RESOLVER_ERR_WRONG_SEED = 11is also added in that module.Gate wired into 4 entrypoints
dash_sdk_sign_with_mnemonic_resolver_and_pathsign_with_mnemonic_resolver.rsderive_privdash_sdk_derive_and_persist_identity_keysidentity_derive_and_persist.rsdash_sdk_derive_identity_key_at_slot_with_resolverderive_identity_key_at_slot.rsderive_at_slot_innerplatform_wallet_manager_bind_shielded(shieldedfeature)shielded_sync.rsPLATFORM_WALLET_MANAGER_STORAGEtouchAt each site, on a mismatch:
master.private_key.non_secure_erase()— derived master xpriv zeroedsig_bufzeroed viaptr::write_bytes;out_rowleft atIdentityKeyPreviewFFI::empty();*out_signature_len = 0)ErrorWrongSeedForWalletfor structured paths,SIGN_WITH_RESOLVER_ERR_WRONG_SEEDfor the u8-tagged path)Two of the four sites (
derive_identity_key_at_slot,shielded_sync)non_secure_erasethe gate-master unconditionally — defense in depth, since the gate-master is throwaway whether the verdict is true or false. The other two keep the master alive past the gate because the same master is then used forderive_privand is erased downstream via the existing pattern.How Has This Been Tested?
All green. 83 unit + 26 comprehensive + 5 integration tests pass; 132 platform-wallet tests still pass; new gate tests included:
sign_gate::tests::matching_seed_passessign_gate::tests::mismatched_seed_failssign_with_mnemonic_resolver::tests::happy_path_signs_and_returns_signaturesig_len == 65(compact-recoverable secp256k1)sign_with_mnemonic_resolver::tests::wrong_wallet_id_fails_closed_with_wrong_seed_tagsig_len==0, every byte of 128-bytesig_bufis zeroidentity_derive_and_persist::tests::happy_path_persists_three_keys_and_returns_pubkeysidentity_derive_and_persist::tests::wrong_wallet_id_fails_closed_before_persisting_anythingErrorWrongSeedForWallet,out.count==0,out.items.is_null(), persister callback row count is zeroderive_identity_key_at_slot::tests::matching_seed_returns_derived_keyderive_identity_key_at_slot::tests::wrong_wallet_id_fails_closed_with_wrong_seed_tagprivate_key_bytesis zeroshielded_sync::tests::wrong_wallet_id_fails_closedErrorWrongSeedForWalletviaNULL_HANDLE— gate short-circuits before manager-storage touchshielded_sync::tests::null_resolver_handle_rejectedcheck_ptr!ordering locked inEvery wrong-seed test asserts on substance — typed tag + output-buffer state + side-effect counters — not "did the function run".
Breaking Changes
None for shipped consumers. The change is purely additive:
(wallet_id, mnemonic)pairs behave identically to before.No
!in the title.Security note
This patch is logically the prerequisite for PR #3692 (seedless watch-only rehydration), which removes a load-time wrong-seed gate. Without this PR landed first, #3692 would briefly create a window where the same wallet_id could be signed by any seed. Once this lands, the merge-up cycle (
v3.1-dev → #3625 → #3672 → #3692) will dedupe the duplicate sign-gate commits sitting on #3692 naturally — no force-push to that PR needed.Checklist:
For repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent