security(twdata): Zeroize every TWData instance in TWDataDelete, and other TWData improvements#4667
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens TWData lifecycle handling in the Trust Wallet Core C interface by attempting to securely wipe TWData contents in TWDataDelete before deallocation.
Changes:
- Add
<TrezorCrypto/memzero.h>include tosrc/interface/TWData.cpp. - Call
memzeroon the underlyingDatabuffer inTWDataDeleteprior todelete.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Binary size comparison➡️ aarch64-apple-ios: 14.34 MB ➡️ aarch64-apple-ios-sim: 14.34 MB ➡️ aarch64-linux-android: 18.77 MB ➡️ armv7-linux-androideabi: 16.20 MB ➡️ wasm32-unknown-emscripten: 13.68 MB |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
TWData instance in TWDataDeleteTWData instance in TWDataDelete, and other TWData improvements
* Deprecate `TWDataGet` as it may lead to UB
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
tests/interface/TWDataTests.cpp:86
- Test coverage: since
TWDataSetnow guards against out-of-range indices, add a unit test that callsTWDataSet(data, TWDataSize(data), ...)(and/or a larger index) and asserts the data remains unchanged. This would lock in the new safety behavior and prevent regressions back to OOB writes.
TEST(TWData, Set) {
const auto data = WRAPD(TWDataCreateWithHexString(STRING("deadbeef").get()));
assertHexEqual(data, "deadbeef");
TWDataSet(data.get(), 1, 0xff);
assertHexEqual(data, "deffbeef");
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This pull request refactors the
TWDatainterface and related code for improved safety, clarity, and memory handling. The most significant changes include removing theTWDataGetfunction, updating the semantics and error handling forTWDataCopyBytesandTWDataReplaceBytes, and ensuring sensitive data is securely erased before deletion. Test cases have also been updated to reflect these changes and improve error coverage.API changes and safety improvements
TWDataGetfunction from both the header and implementation, encouraging direct use ofTWDataBytesfor accessing data and simplifying the API.TWDataCopyBytesandTWDataReplaceBytesto return an integer status code (0 for success, -1 for failure) and added bounds checking to prevent invalid memory access. The header documentation and implementation were both updated accordingly.TWPrivateKeyCreateWithDataandTWPrivateKeyIsValid.Memory handling and security
TWDataandTWStringto securely erase memory usingmemzerobefore freeing, reducing the risk of sensitive data leakage.memzero.hfor secure memory erasure.Test updates and improvements
TWDataGetfunction, switching to direct buffer access viaTWDataBytes.TWDataCopyBytesandTWDataReplaceBytes, adding new cases to verify correct behavior for out-of-bounds and zero-size operations.nullptrresults.These changes collectively improve the safety, maintainability, and correctness of the codebase, especially around memory management and error handling.