Repository navigation
fix: validate Electrum import input boundaries - #7834
PastaPastaPasta merged 1 commit into
Conversation
Parse the actual JSON byte count from terminated owning storage and reject stream errors. Reject negative rescan indices before importing keys while retaining the above-tip clamp. Cover JSON and CSV private-key import compatibility and public rejection before wallet mutation.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 39 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (2)
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 |
|
✅ Final review complete — no blockers (commit 9178483) · triage: low |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The Electrum import changes correctly select UniValue’s length-bounded parser using terminated string storage and reject negative rescan indices before file processing or key installation, while preserving the default and above-tip clamp. The regression tests cover malformed JSON rejection without key import, negative indices for both export formats, and valid private-key imports; no in-scope defects were found. Validation was static only: the supplied CI snapshot shows successful formatting and metadata checks, but build-container jobs remain queued, so build and test success is not independently confirmed.
Review provenance
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — The diff is a small, contained RPC input-validation fix with focused regression tests, not a large or intricate change to key handling or another critical surface. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort high); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 93% left, weekly 95% left; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer
thepastaclaw
added
the
pastaclaw:approved
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
sure; idk if anyone even uses this.
2 participants
Issue being fixed or feature implemented
importelectrumwalletreads an exact-sized JSON file allocation through UniValue's pointer-only parser, which determines length usingstrlen. A full read supplies no terminator. It also accepts negative rescan indices, imports keys and then passes the negative height to an exact chain lookup that aborts the node. Both paths require an authorized RPC caller, a readable local export and an unlocked legacy wallet. Method-limited deployments make the negative-index abort relevant beyond an administrator's existing ability to stop a node.What was done?
Use a binary, length-managed string for JSON input, validate file size/read errors, resize to the actual byte count and invoke the bounded parser overload. The string also supplies the parser's sentinel. Reject a negative rescan index before processing the export or installing keys; preserve the existing clamp for indices above the tip and document the non-negative argument.
Extend
wallet_dump.pyto reject a valid JSON prefix followed by embedded NUL/trailing bytes, check negative indices for JSON and CSV without importing keys, and retain valid bare/p2pkh-prefixed WIF imports at zero, tip and above-tip indices. Verify the imported private keys through dump/sign/verify RPCs and unload the temporary wallets before the existing dumpwallet scenarios.How Has This Been Tested?
On macOS arm64 with BDB and SQLite enabled: full
make -j1, the completewallet_dump.py --legacy-wallet --portseed=1, a separate negative-index node-liveness regression (both serially at TEST_RUNNER_PORT_MIN=12000), 31 wallet/walletdb unit cases, and both UniValue object/data test executables passed. Python lint with the repository selection, assertion/include/whitespace lints andgit diff --checkpassed. The UniValue data executable requiressrcas its working directory; an initial root-directory invocation failed to find its fixtures and the corrected invocation passed. Independent four-pass review found no blocking source defects; the final transferred patch exactly matches the reviewed file/diff hashes. Baseline defensive RPC regressions accepted the embedded-NUL JSON prefix, and a separate disposable regtest negative-index RPC terminated the node at the productiongetBlockHashassertion. The earlier isolated actual-UniValue ASan proof demonstrates the allocation/parser primitive, not a full-daemon disclosure or code-execution outcome. No deployed restricted account or production crash probability was measured. Stream-error injection, concurrent file modification and exact rescan-start observation were not tested.Breaking Changes
Negative rescan indices now return RPC_INVALID_PARAMETER before import. JSON bytes after an embedded NUL are parsed and rejected instead of silently ignored. Valid export formats and the above-tip clamp remain supported.
Checklist:
This pull request was created by Codex.