Skip to content

Enhance secure string handling and memory management in WASM - #4826

Open
sergei-boiko-trustwallet wants to merge 7 commits into
masterfrom
fix/wasm-zeroize-data/sc-147688
Open

Enhance secure string handling and memory management in WASM#4826
sergei-boiko-trustwallet wants to merge 7 commits into
masterfrom
fix/wasm-zeroize-data/sc-147688

Conversation

@sergei-boiko-trustwallet

Copy link
Copy Markdown
Contributor

This pull request introduces significant improvements to how sensitive data is handled in the WASM bindings, focusing on enhanced memory hygiene and security. The main changes include ensuring that data buffers are securely zeroed after use, updating conversion utilities to return JavaScript values instead of C++ strings, and refactoring method templates to consistently wipe sensitive parameters. These updates help prevent leaks of sensitive information in both WASM and JS heaps.

Memory hygiene and security improvements:

  • Updated DataToVal and TWDataToVal to take ownership of data, copy it to a JS Uint8Array, and securely zero the WASM-heap buffer after use (wasm/src/WasmData.cpp, wasm/src/WasmData.h). [1] [2] [3]
  • Refactored TWStringToStd to TWStringToVal, now returning a JS string and explicitly zeroing both the WASM-heap and the intermediate JS-heap byte buffers (wasm/src/WasmString.cpp, wasm/src/WasmString.h). [1] [2]

Template and method call refactoring:

  • Updated the C++ method forward template to detect parameters of type data and, after method calls, securely wipe their buffers before returning results (codegen/lib/templates/cpp/method_forward.erb).
  • Changed AnySigner::sign and AnySigner::plan to use std::move for efficient transfer and zeroing of output buffers (wasm/src/AnySigner.cpp). [1] [2]
  • Modified HexCoding::parseHex to use std::move for proper ownership and zeroing of parsed data (wasm/src/HexCoding.cpp).

API consistency:

  • Updated all relevant CoinTypeExt methods to use TWStringToVal instead of TWStringToStd, ensuring consistent secure string handling across the API (wasm/src/CoinTypeExt.cpp). [1] [2]

@octane-security-app

Copy link
Copy Markdown

Summary by Octane

New Contracts

No new contracts were added.

Updated Contracts

  • AnySigner.cpp: The primary modification involves optimizing data handling by using std::move for improved performance in sign and plan functions.
  • CoinTypeExt.cpp: Replaced TWStringToStd with TWStringToVal for various CoinTypeExt functions.
  • HexCoding.cpp: The modification improves efficiency by using std::move with parsed hex data in the parseHex method.
  • WasmData.cpp: Enhanced memory management by transferring data ownership and securely clearing buffers after use.
  • WasmString.cpp: The smart contract's functionality has been enhanced by converting strings directly to JavaScript values, optimizing memory usage and security.

🔗 Commit Hash: 80dc5e7

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the WASM bindings’ handling of sensitive strings and byte buffers by ensuring buffers are copied into JS-owned values and the original WASM-heap memory is securely zeroed after use, while also refactoring the codegen template to wipe data parameters consistently.

Changes:

  • Updated TWString conversion to return an emscripten::val JS string via TextDecoder, and zeroed the intermediate JS Uint8Array.
  • Updated DataToVal/TWDataToVal to take ownership, copy into a JS Uint8Array, then memzero the WASM-heap buffer.
  • Refactored the C++ codegen method-forward template to wipe data parameters after the underlying core call.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
wasm/src/WasmString.h Renames string conversion helper to return emscripten::val and documents zeroization behavior.
wasm/src/WasmString.cpp Implements JS-string conversion via TextDecoder with explicit zeroing of intermediate JS byte buffer.
wasm/src/WasmData.h Changes DataToVal API to take ownership (Data&&) to enable post-copy zeroization.
wasm/src/WasmData.cpp Moves/owns buffers, copies to JS Uint8Array, and securely zeroes WASM-heap memory.
wasm/src/AnySigner.cpp Moves output buffers into DataToVal to ensure ownership transfer and zeroization.
wasm/src/HexCoding.cpp Moves parsed hex data into DataToVal to ensure ownership transfer and zeroization.
wasm/src/CoinTypeExt.cpp Switches string-returning APIs to TWStringToVal for consistent secure handling.
codegen/lib/templates/cpp/method_forward.erb Adds parameter wiping for data parameters and refactors return handling.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread codegen/lib/templates/cpp/method_forward.erb
Comment thread wasm/src/WasmData.h
@github-actions

Copy link
Copy Markdown

Binary size comparison

➡️ aarch64-apple-ios: 14.31 MB

➡️ aarch64-apple-ios-sim: 14.31 MB

➡️ aarch64-linux-android: 18.73 MB

➡️ armv7-linux-androideabi: 16.17 MB

➡️ wasm32-unknown-emscripten: 13.66 MB

@octane-security-app

Copy link
Copy Markdown

Overview

Vulnerabilities found: 3                                                                                
Severity breakdown: 2 Medium, 1 Low

Detailed findings

wasm/src/CoinTypeExt.cpp

  • WASM-typed view exposure in TWStringToVal for CoinTypeExt identity getters causes private key exfiltration. See more

wasm/src/HexCoding.cpp

  • Added unbounded memzero in WASM DataToVal for HexCoding.decode causes CPU‑amplification DoS. See more

wasm/src/WasmString.cpp

  • Per-call global TextDecoder lookup in WASM string conversion (TWStringToVal) used by CoinTypeExt helpers causes semantic-drift signing on unintended chains. See more

🔗 Commit Hash: 80dc5e7
🛡️ Octane Dashboard: All vulnerabilities

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants