[Hardening] Reject non-object JSON notifications without panicking (F-008)

Open Beginner friendly
  • Is something wrong?
#237 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
2/5
Estimated time
1-3 hours
Newbie friendliness
78/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Active
Tech stack
rust
Domain
networking, security

Research direction

Start in raw_client.rs around lines 764–773, where the notification path reads the JSON response fields. Check how non-object JSON values are handled, then add regression coverage for scalar, array, and null notifications. Done means invalid notification shapes return a normal protocol or parse error without panicking, while the null negative control still allows the ping to complete.

Written by the indexing model from the issue text.

Description

bug

Summary

In rust-electrum-client 0.25.2 (reported upstream commit 7c1d759e2789b5e46fcae1525ab06ff4012bc668), the reader notification path accesses resp["method"].take(). A server-pushed JSON scalar without an id is treated as a notification, but mutable indexing a scalar JSON value panics. A malicious or compromised Electrum server (or an attacker able to alter the Electrum connection) can therefore cause client/wallet availability loss in a release build.

Reproduction

  1. Build the client in release mode.
  2. Negotiate protocol version 1.4.
  3. While a request is in flight, send this line:
5

Observed:

serde_json panic: cannot access key "method" in JSON number

A null notification was tested as a negative control and the subsequent ping completed normally.

Source

Reported source location: raw_client.rs lines 764–773 (in the affected revision).

Suggested hardening

  • Require a JSON object before reading notification fields, or use non-panicking accessors such as get("method").
  • Return a normal protocol/parse error for invalid notification shapes.
  • Add regression tests for scalar, array, and null notifications.

This is a hardening issue: the demonstrated impact is availability loss in release mode; no fund or privacy compromise was observed.

Dominant language
Rust
Stars
89
Forks
87
Avg merge
1h 25m
Merged PRs (30d)
4

Getting set up

This project ships no dev container, Dockerfile or contributing guide, so setting up is up to you: start from its README, and see our first-contribution guide for the general steps.

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from bitcoindevkit/rust-electrum-client

All issues in bitcoindevkit/rust-electrum-client

Similar issues

More Rust issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.