fix(rs-sdk-ffi): zeroize private key arrays after use in crypto/signer FFI#3433
Conversation
…r FFI Private key byte arrays in non-shielded FFI functions were not being zeroized after use, leaving key material on the stack. The shielded crypto module already handles this correctly via zeroize::Zeroize. Wrap all `[u8; 32]` key arrays with `Zeroizing<>` so they are automatically zeroed on drop, covering all return paths including early returns. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Review GateCommit:
|
📝 WalkthroughWalkthroughRemoved Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3433 +/- ##
============================================
+ Coverage 80.22% 80.49% +0.26%
============================================
Files 2855 2855
Lines 281685 284101 +2416
============================================
+ Hits 225983 228678 +2695
+ Misses 55702 55423 -279
🚀 New features to boost your workflow:
|
cargo-machete flagged serde_json as unused in rs-scripts, causing CI failure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-sdk-ffi/src/crypto/mod.rs (1)
53-71:⚠️ Potential issue | 🟠 MajorZeroizing the stack buffer is good, but key bytes still persist in a non-zeroized
Vec<u8>.At lines 53, 113, and 188, private keys are decoded into
private_key_bytes(Vec<u8>) and then copied intoZeroizing<[u8; 32]>. TheVec<u8>copy is not zeroized on drop, so sensitive material can still linger in heap memory.Use
hex::decode_to_slicedirectly into theZeroizingbuffer to avoid creating the extra secret copy.Suggested pattern (apply in all 3 functions)
- let private_key_bytes = match hex::decode(private_key_str) { - Ok(bytes) if bytes.len() == 32 => bytes, - Ok(_) => { - return DashSDKResult::error(DashSDKError::new( - DashSDKErrorCode::InvalidParameter, - "Private key must be exactly 32 bytes".to_string(), - )) - } - Err(e) => { - return DashSDKResult::error(DashSDKError::new( - DashSDKErrorCode::InvalidParameter, - format!("Invalid private key hex: {}", e), - )) - } - }; - - let mut key_array = Zeroizing::new([0u8; 32]); - key_array.copy_from_slice(&private_key_bytes); + let mut key_array = Zeroizing::new([0u8; 32]); + if let Err(e) = hex::decode_to_slice(private_key_str, &mut *key_array) { + return DashSDKResult::error(DashSDKError::new( + DashSDKErrorCode::InvalidParameter, + format!("Invalid private key hex: {}", e), + )); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/crypto/mod.rs` around lines 53 - 71, The code decodes hex into a temporary Vec<u8> named private_key_bytes and then copies into a Zeroizing<[u8; 32]> (key_array), leaving the heap copy around; change each occurrence (where private_key_str is decoded at lines referenced) to allocate key_array as Zeroizing::new([0u8; 32]) first and use hex::decode_to_slice(private_key_str, &mut *key_array) (or equivalent) to decode directly into the zeroized stack buffer, validate the returned length/error and avoid calling copy_from_slice on a Vec so no intermediate Vec<u8> containing the secret is ever created (apply same change for all three functions/locations that use private_key_str / private_key_bytes / key_array).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/rs-sdk-ffi/src/crypto/mod.rs`:
- Around line 53-71: The code decodes hex into a temporary Vec<u8> named
private_key_bytes and then copies into a Zeroizing<[u8; 32]> (key_array),
leaving the heap copy around; change each occurrence (where private_key_str is
decoded at lines referenced) to allocate key_array as Zeroizing::new([0u8; 32])
first and use hex::decode_to_slice(private_key_str, &mut *key_array) (or
equivalent) to decode directly into the zeroized stack buffer, validate the
returned length/error and avoid calling copy_from_slice on a Vec so no
intermediate Vec<u8> containing the secret is ever created (apply same change
for all three functions/locations that use private_key_str / private_key_bytes /
key_array).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 58226d2a-5e04-43de-b13e-df78a5f28a68
📒 Files selected for processing (3)
packages/rs-scripts/Cargo.tomlpackages/rs-sdk-ffi/src/crypto/mod.rspackages/rs-sdk-ffi/src/signer_simple.rs
💤 Files with no reviewable changes (1)
- packages/rs-scripts/Cargo.toml
|
Self reviewed |
|
✅ 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: "6af15c010dd65b2dab699725e8ab77c6ded24a7145096f4571e12d1380bb898f"
)Xcode manual integration:
|
2 participants
Issue Being Fixed
Security audit finding M5: Private key byte arrays in non-shielded FFI functions (
crypto/mod.rsandsigner_simple.rs) were not being zeroized after use, leaving key material lingering on the stack.The shielded crypto module (
shielded/crypto/address.rs,bundle_build.rs,decrypt.rs) already handles this correctly viazeroize::Zeroize. This PR brings the non-shielded FFI functions to the same standard.What was changed
Wrapped all
[u8; 32]private key arrays withzeroize::Zeroizing<>so they are automatically zeroed on drop. This covers all return paths including early returns, with zero runtime overhead beyond the memset on drop.Files changed:
packages/rs-sdk-ffi/src/crypto/mod.rs— 3 functions:dash_sdk_validate_private_key_for_public_key,dash_sdk_private_key_to_wif,dash_sdk_public_key_data_from_private_key_datapackages/rs-sdk-ffi/src/signer_simple.rs— 1 function:dash_sdk_signer_create_from_private_keyBreaking Changes
None.
Zeroizing<[u8; 32]>implementsDeref<Target=[u8; 32]>, so all existing call sites (.as_slice(),&key_array, indexing) work transparently.Tests
Existing compilation and runtime behavior unchanged —
Zeroizingis a zero-cost wrapper that only adds a zeroize call on drop.🤖 Generated with Claude Code
Summary by CodeRabbit
Security
Chores