fix(platform-wallet): zeroize private keys when freeing preview rows#3797
Conversation
`release_row` in identity_key_preview.rs freed `private_key_wif` (a human-readable WIF private key) and dropped the inline `private_key_bytes: [u8; 32]` ECDSA scalar without zeroizing either, leaving real private keys in freed heap pages on every `platform_wallet_preview_identity_registration_keys[_free]` call. The mid-loop error `cleanup` closure in identity_keys_from_mnemonic.rs had the same gap. The sibling `dash_sdk_derive_identity_keys_from_mnemonic_free` already zeroizes correctly; this aligns both paths: - `release_row` now takes `&mut`, scrubs the WIF backing bytes (`into_bytes_with_nul()` + `Zeroize::zeroize`), zeroizes `private_key_bytes` in place, and nulls every owned pointer (preserving double-free idempotency). - A shared `zeroize_and_free_row` helper is factored out and used by both the mid-loop cleanup closure and the public `_free`, so the paths can't drift. Public `extern "C"` ABI is unchanged (only internal helpers changed). Adds tests asserting the scalar is zeroized, pointers are nulled, and a second free is a safe no-op. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 14 minutes and 26 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCentralizes zeroization/free of FFI identity key preview rows via a new idempotent helper and switches all modules (mnemonic derivation, preview, derive-at-slot, registration-with-signer) to call it; adds tests verifying zeroization, pointer nulling, and idempotency. ChangesSensitive Key Material Cleanup
🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
✅ Review complete (commit cbaddeb) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-platform-wallet-ffi/src/identity_key_preview.rs (1)
297-340: ⚡ Quick winConsider reusing
zeroize_and_free_rowfrom the sibling module.
release_rowduplicates the logic ofcrate::identity_keys_from_mnemonic::zeroize_and_free_row, which is alreadypub(crate). Consolidating to a single implementation eliminates the drift risk noted in the doc comment.♻️ Suggested refactor
+use crate::identity_keys_from_mnemonic::zeroize_and_free_row; + /// Release an [`IdentityKeyPreviewsFFI`] ... #[no_mangle] pub unsafe extern "C" fn platform_wallet_preview_identity_registration_keys_free( previews: *mut IdentityKeyPreviewsFFI, ) { // ... existing null checks ... unsafe { let slice = std::slice::from_raw_parts_mut(previews.items, previews.count); let mut boxed: Box<[IdentityKeyPreviewFFI]> = Box::from_raw(slice); for row in boxed.iter_mut() { - release_row(row); + zeroize_and_free_row(row); } drop(boxed); } // ... } -/// Reclaim the heap allocations owned by a single preview row ... -unsafe fn release_row(row: &mut IdentityKeyPreviewFFI) { - // ... entire function body ... -} /// Release every row in a partially-built preview list ... fn free_rows(mut rows: Vec<IdentityKeyPreviewFFI>) { for row in &mut rows { - unsafe { release_row(row) }; + unsafe { zeroize_and_free_row(row) }; } drop(rows); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-ffi/src/identity_key_preview.rs` around lines 297 - 340, The function release_row duplicates the zeroization/free logic already implemented as pub(crate) zeroize_and_free_row in crate::identity_keys_from_mnemonic; replace the body of unsafe fn release_row (and any duplicate drop/zeroize steps) with a call to crate::identity_keys_from_mnemonic::zeroize_and_free_row(row) so the single canonical implementation is used, ensure the module imports or fully-qualify that symbol and that the signature of release_row matches the expected argument type (or adapt by forwarding/converting to the existing function) and remove the duplicated cleanup code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/rs-platform-wallet-ffi/src/identity_key_preview.rs`:
- Around line 297-340: The function release_row duplicates the zeroization/free
logic already implemented as pub(crate) zeroize_and_free_row in
crate::identity_keys_from_mnemonic; replace the body of unsafe fn release_row
(and any duplicate drop/zeroize steps) with a call to
crate::identity_keys_from_mnemonic::zeroize_and_free_row(row) so the single
canonical implementation is used, ensure the module imports or fully-qualify
that symbol and that the signature of release_row matches the expected argument
type (or adapt by forwarding/converting to the existing function) and remove the
duplicated cleanup code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f11d8aa6-30e8-4c77-90f2-ac27c775993a
📒 Files selected for processing (2)
packages/rs-platform-wallet-ffi/src/identity_key_preview.rspackages/rs-platform-wallet-ffi/src/identity_keys_from_mnemonic.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Defensive-security fix that correctly closes a residual-secret leak across two FFI cleanup paths: release_row in identity_key_preview.rs and the shared zeroize_and_free_row helper in identity_keys_from_mnemonic.rs both now scrub the WIF backing bytes and the inline 32-byte ECDSA scalar before allocations are returned to the allocator, and pointer-nulling preserves double-free idempotency. The one in-scope concern is that the PR's stated 'no-drift' goal is only partially realized — two near-identical helpers for the same #[repr(C)] row type now coexist (one per file) instead of a single shared implementation.
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-ffi/src/identity_key_preview.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/identity_key_preview.rs:312-340: release_row duplicates zeroize_and_free_row for the same struct — the drift surface the PR claims to remove still exists
The PR description states a shared `zeroize_and_free_row(&mut IdentityKeyPreviewFFI)` helper was factored out so the cleanup and public free paths 'can't drift'. That guarantee only holds inside `identity_keys_from_mnemonic.rs`. In `identity_key_preview.rs`, `release_row` is a second hand-written copy of the same operation against the same `IdentityKeyPreviewFFI` struct — same fields reclaimed, same zeroization performed, with a slightly different field-handling order (path → wif → public_key → scalar vs. path → public_key → wif → scalar). The doc comment on `release_row` even claims it 'mirrors' the other implementation. Because both helpers operate on identical struct layout and ownership semantics, a future change to one (e.g., adding a new sensitive `*mut c_char` field, or moving `private_key_bytes` behind a `Zeroizing` wrapper) can silently leave the other path leaking key material across the FFI boundary — the exact class of bug this PR is fixing today. Resolve by having `release_row` delegate to the already `pub(crate)` `zeroize_and_free_row`, or move a single canonical impl into a shared submodule. The `TODO` at identity_key_preview.rs:230 about implementing `Drop` for the row hints at the cleanest endpoint: a `Drop` impl on `IdentityKeyPreviewFFI` itself (combined with the existing `mem::forget` discipline at the FFI cliff) would make exactly-one zeroization site by construction.
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "413f7c85dddc1ab4dcac783c4b8c3b6c5ab60dbfe22418a6edee7a7e99cc8d33"
)Xcode manual integration:
|
Adversarial review found the original fix was incomplete: two more producers of IdentityKeyPreviewFFI (which carries a clear-text WIF private key and the raw 32-byte ECDSA scalar) released that material without proper zeroization. - identity_registration_with_signer.rs: the mid-loop `cleanup` closure (error path when key_count>1 and derivation fails at index N>0) freed the WIF in clear text and never scrubbed private_key_bytes, leaving rows 0..N-1's secrets in freed heap. Now routes through the shared zeroize_and_free_row helper, as does its public _free (so success and error paths can't drift). - derive_identity_key_at_slot.rs: its _free scrubbed with non-volatile std::ptr::write_bytes + a manual `*byte = 0` loop, both of which the optimizer may elide. Now uses zeroize_and_free_row (volatile zeroize). Verified the remaining producer (identity_derive_and_persist.rs) builds redacted rows (null WIF, zero scalar; the real scalar is zeroized locally), so it has nothing to scrub. Public extern "C" ABI unchanged. Adds tests for both fixed paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3797 +/- ##
==========================================
Coverage 87.18% 87.19%
==========================================
Files 2624 2624
Lines 321014 321186 +172
==========================================
+ Hits 279892 280046 +154
- Misses 41122 41140 +18
🚀 New features to boost your workflow:
|
…_and_free_row Review follow-up: `release_row` in identity_key_preview.rs was a second hand-written copy of the same zeroize-and-free operation on the same `IdentityKeyPreviewFFI` struct as the shared `zeroize_and_free_row` helper (only the field-handling order differed). That left the exact drift surface this PR set out to remove: a future change to one path (a new sensitive `*mut c_char` field, wrapping `private_key_bytes` in `Zeroizing`, etc.) could silently leave the other leaking key material. `release_row` now delegates to the single canonical `zeroize_and_free_row`, so all four free paths (this file's public free + build-loop cleanup, plus identity_keys_from_mnemonic, identity_registration_with_signer, and derive_identity_key_at_slot) share exactly one implementation. The existing `release_row_zeroizes_secret_and_is_idempotent` test still passes, now exercising the delegation. (The `Drop`-impl endpoint noted in the build-loop TODO remains the eventual single-site-by-construction goal.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-forward prior finding (f0bf1095f995, release_row duplication) is fixed at cbaddeb by making release_row a thin delegation to the canonical zeroize_and_free_row helper. All in-scope IdentityKeyPreviewFFI free paths now route through one implementation. No new in-scope defects in the latest delta; the resolver-length validation gap flagged by codex-ffi-engineer at derive_identity_key_at_slot.rs:183 is pre-existing dispatch code untouched by this zeroize-on-free PR and is recorded only as an out-of-scope follow-up.
2 participants
Issue being fixed or feature implemented
Private-key material left un-zeroized in freed heap memory (mobile wallet).
release_rowinidentity_key_preview.rsfreedprivate_key_wif(a*mut c_charholding the WIF private key in clear text) and dropped the row's inlineprivate_key_bytes: [u8; 32](the raw ECDSA scalar) without zeroizing either, returning freed pages that still contain real private keys to the allocator. This runs on everyplatform_wallet_preview_identity_registration_keys→_freeround-trip (the "scan/preview identity registration keys" UI path). The mid-loop errorcleanupclosure inidentity_keys_from_mnemonic.rshad the same gap.The structurally identical sibling
dash_sdk_derive_identity_keys_from_mnemonic_freealready zeroizes both correctly — this aligns the lagging paths with it.What was done?
release_rownow takes&mut IdentityKeyPreviewFFI, scrubs the WIF backing bytes (CString::into_bytes_with_nul()+zeroize::Zeroize::zeroize), zeroizesprivate_key_bytesin place, and nulls every owned pointer after reclaiming it (preserving double-free idempotency). Call sites (platform_wallet_preview_identity_registration_keys_free,free_rows) updated toiter_mut/&mut.zeroize_and_free_row(&mut IdentityKeyPreviewFFI)helper is factored out inidentity_keys_from_mnemonic.rsand used by both the mid-loop cleanup closure and the publicdash_sdk_derive_identity_keys_from_mnemonic_free, so the two paths can't drift.extern "C"ABI is unchanged — only internal helpers changed.How Has This Been Tested?
cargo test -p platform-wallet-ffi identity_key_preview— 3 passed, 0 failed:release_row_zeroizes_secret_and_is_idempotent: asserts the inline scalar is wiped to all-zero, every owned pointer is nulled, and a secondrelease_rowis a safe no-op.public_free_zeroizes_and_resets: drives rows through the real_freeentry point; asserts the outer struct is reset and a second free no-ops.free_rows_zeroizes_partial_list: mid-loop cleanup path releases a partial list without panicking/double-freeing.Crate builds clean;
cargo fmt --allapplied.Breaking Changes
None. Internal helper signatures only; the
extern "C"FFI surface is unchanged.Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Bug Fixes
Tests