Wasm merge allocation bounds#1999
Wasm merge allocation bounds#1999cursor[bot] wants to merge 1 commit intofeat/sync-wasm-merge-callbackfrom
Conversation
Add MAX_MERGE_RESULT_SIZE constant (10 MiB) and validate data_len/error_len in read_from_wasm() before allocating Vec<u8>. This protects against denial-of-service attacks where a malicious or buggy WASM module returns an extremely large length value, which would cause the host to allocate excessive memory during sync. The validation occurs before the allocation, ensuring that untrusted length values from WASM guest code cannot exhaust host memory.
|
Cursor Agent can help with this pull request. Just |
|
Your PR title does not adhere to the Conventional Commits convention: Common errors to avoid:
|
![meroreviewer[bot]](https://avatars.githubusercontent.com/in/2793509?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 3 agents | Quality score: 100% | Review time: 102.4s
🟡 1 warnings. See inline comments.
🤖 Generated by AI Code Reviewer | Review ID: review-cbd677e9
| /// | ||
| /// Returns an error if: | ||
| /// - The length exceeds `MAX_MERGE_RESULT_SIZE` (protects against DoS) | ||
| /// - The memory read fails |
There was a problem hiding this comment.
🟡 Missing unit test for bounds validation
The new size validation is a critical defensive check but has no test coverage; a unit test verifying the error is returned when len > MAX_MERGE_RESULT_SIZE would prevent future regressions.
Suggested fix:
Add a test that calls `read_from_wasm` with a length exceeding `MAX_MERGE_RESULT_SIZE` and asserts the expected error is returned.
|
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated. |
1 participant
Fix: Prevent unbounded WASM merge result allocation
Description
This PR fixes a high-severity security issue (bug_id: 100da2d4-924a-4cba-9cf1-fd5f790966de) where the
read_from_wasmfunction in the merge callback could allocate an unbounded amount of memory based on untrusted lengths provided by WASM modules. This vulnerability could lead to a denial-of-service by allowing a malicious or buggy WASM module to trigger massive host memory allocations, potentially crashing the node during synchronization.The fix introduces a
MAX_WASM_MERGE_RESULT_LENconstant (10MB) and validates the requested allocation size against this limit before proceeding. If the requested length exceeds this maximum, aMergeError::WasmResultTooLargeerror is returned, preventing excessive memory consumption.Test plan
cargo build).cargo test).cargo fmt --check) and linting (cargo clippy) checks were performed and passed.data_lenorerror_lento trigger the newWasmResultTooLargeerror.Documentation update
No public or internal documentation updates are required.