Skip to content

fix(platform-wallet): bound bincode decode size on asset-lock proof bytes - #4585

Merged
lklimek merged 3 commits into
v4.2-devfrom
fix/platform-wallet-ffi-asset-lock-proof-size-gate
Sep 11, 2026
Merged

fix(platform-wallet): bound bincode decode size on asset-lock proof bytes#4585
lklimek merged 3 commits into
v4.2-devfrom
fix/platform-wallet-ffi-asset-lock-proof-size-gate

Conversation

@Claudius-Maginificent

@Claudius-Maginificent Claudius-Maginificent commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

TL;DR: Bounds the size of asset-lock proof data decoded at the platform-wallet FFI boundary, so a malicious or malformed input can't exhaust memory before it's even validated.

User story

As an app developer integrating the platform-wallet FFI, I want asset-lock proof decoding to reject oversized input up front, so a malformed or hostile proof can't stall or crash my app via unbounded memory allocation.

Scenario

Base flow

A host app passes raw asset-lock proof bytes across the FFI boundary; the FFI decodes them with bincode before further processing.

Actual behavior

The decode was unbounded. A crafted byte sequence — including one with an oversized internal length prefix — could make bincode attempt to allocate gigabytes before erroring: a memory-exhaustion DoS vector at the FFI boundary.

Expected behavior

Proof bytes over 16 MiB are rejected immediately with the existing ErrorInvalidParameter code, before any decode is attempted. The bincode decode itself is also bounded (with_limit::<MAX>()), so a hostile length prefix inside an otherwise-valid-sized buffer can't over-allocate either.

Detailed discussion

What was done

Split out of #3968 (rs-platform-wallet-storage PR) as part of a coordinated PR-splitting effort — see that PR's description for the full breakdown. This change is fully independent of the storage crate: packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs gains MAX_ASSET_LOCK_PROOF_SIZE_BYTES = 16 MiB, a validate_asset_lock_proof_size gate checked before from_raw_parts, and bincode::config::standard().with_limit::<MAX>() on the decode call. Reuses the pre-existing ErrorInvalidParameter FFI code — no new discriminants, no ABI surface change.

Testing

New unit test asserting oversized proof bytes are rejected before decode is attempted (cargo nextest -p platform-wallet-ffi, passes). cargo clippy -p platform-wallet-ffi --all-targets clean. cargo fmt --check clean.

Breaking changes

None — no new error codes, no signature changes.

Checklist

  • I have performed a self-review of my own code
  • I have added or updated relevant unit tests
  • No breaking changes
  • No documentation changes needed

Prior work

Split out of #3968 as part of a coordinated 4-PR split (PR 0 = trimmed #3968, this PR, plus two more for typed persister errors and FFI/misc fixes). See #3968 for the full rationale.

🤖 Co-authored by Claudius the Magnificent AI Agent

Summary by CodeRabbit

  • Bug Fixes
    • Added validation to reject asset lock proofs larger than 16 MiB.
    • Improved handling of oversized proofs during recovery to prevent invalid data from being processed.

…of bytes

`asset_lock_manager_recover` fed attacker-controlled `proof_bytes` straight
into `bincode::decode_from_slice` with the unbounded `config::standard()`,
so a hostile length prefix could drive an unbounded allocation across the
FFI boundary (memory-exhaustion DoS).

Gate `proof_len` at 16 MiB before touching the slice and hand bincode the
same ceiling via `with_limit::<>`, so nested length prefixes cannot
over-allocate inside the budget either. Rejection reuses the existing
`ErrorInvalidParameter` code — no ABI surface change, no registry entry.

<sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: cd2ad19e-1f75-4b5f-bade-64f1ee0cf66e

📥 Commits

Reviewing files that changed from the base of the PR and between e096d1e and e91714f.

📒 Files selected for processing (1)
  • packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The asset lock recovery path now rejects proofs larger than 16 MiB before decoding. Bincode decoding uses the same size limit. Tests verify acceptance at the limit and rejection above it.

Changes

Asset lock proof validation

Layer / File(s) Summary
Bounded proof validation and recovery
packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs
Adds the 16 MiB limit and validation helper. Recovery validates proof size before decoding and applies a matching bincode limit. Tests cover the exact boundary and oversized proofs.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to e9171

The change bounds proof validation and decoding at 16 MiB with boundary coverage, so it is mergeable after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: limiting bincode decode size for asset-lock proof bytes.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/platform-wallet-ffi-asset-lock-proof-size-gate

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

# Conflicts:
#	packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs
@lklimek
lklimek marked this pull request as ready for review September 11, 2026 11:15
@thepastaclaw

thepastaclaw commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

🕓 Queued for automated review — 3rd in line, estimated start in ~35 min (commit e91714f)
Estimated review time once started: ~25 min (two-phase automated review; median of recent runs).

  • Request priority review — tick this box and the review moves to the front of the queue.

@lklimek lklimek changed the title fix(platform-wallet-ffi): bound bincode decode size on asset-lock proof bytes fix(platform-wallet): bound bincode decode size on asset-lock proof bytes Sep 11, 2026
@lklimek
lklimek merged commit d020728 into v4.2-dev Sep 11, 2026
30 of 33 checks passed
@lklimek
lklimek deleted the fix/platform-wallet-ffi-asset-lock-proof-size-gate branch September 11, 2026 13:50
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.

4 participants