diff --git a/CHANGELOG.md b/CHANGELOG.md index c97fa16..340ff17 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,10 @@ release and covers the full history from the initial commit. ### Fixed +- `ElectrumClient`: a single server reply is now capped at 16 MiB (`maxLineBytes` in + `ConnectAsync`). A hostile server could stream an endless line with no newline and exhaust the + client's memory; the connection now fails with `InvalidDataException` as soon as the cap is + exceeded. - `ElectrumClient`: every request is now bounded (30 s, 10 s for the handshake; configurable in `ConnectAsync`). A server that stops answering without closing the socket made a running sync wait forever; now the request fails with `TimeoutException`, the connection is marked dead diff --git a/SECURITY.md b/SECURITY.md index bdf0699..d9d39d5 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -41,6 +41,10 @@ It cannot (given correct Merkle verification): - Fabricate a confirmed transaction with a valid Merkle proof - Forge a payment to a wrong address +**Network resource limits.** Server input is bounded so a hostile server cannot hang or kill the +client: every request has a timeout (30 s; 10 s for the handshake; a timeout drops the connection) +and a single reply line is capped at 16 MiB (`Core/Net/ElectrumClient.cs`, ADR 0034 and 0035). + **Progressive verification (mobile-friendly sync).** On a wallet with many historical transactions, `WalletSynchronizer` no longer blocks the whole sync on every Merkle proof: balance and history are shown as soon as transaction downloads finish (`PartialResult`, diff --git a/docs/adr/0035-cap-server-line-length.md b/docs/adr/0035-cap-server-line-length.md new file mode 100644 index 0000000..c74df0b --- /dev/null +++ b/docs/adr/0035-cap-server-line-length.md @@ -0,0 +1,27 @@ +# 0035. Cap the length of a server line + +- **Status:** Accepted +- **Area:** Network +- **Recorded:** 2026-11-03 + +## Context + +`ElectrumClient.ReadLoopAsync` accumulated bytes until it found a newline, with no maximum. A malicious, misconfigured or compromised server could send an endless line and drive memory use up until the process died, which is cheap to do and fatal on Android. `SECURITY.md` treats denial of service against the indexing server as out of scope, but here it is the client that dies, and the fix is small. + +## Decision + +- A single JSON-RPC line is capped at `ElectrumClient.DefaultMaxLineBytes` (16 MiB), a parameter of `ConnectAsync` (`maxLineBytes`). The default is well above the largest legitimate replies: header batches (a few hundred KB), a large transaction (up to about 2 MB of hex) and the history of a very active address (a few MB). +- The check runs as the buffer grows, **before** a newline arrives, and also on complete lines, so neither an unterminated stream nor one huge terminated line is buffered past the cap (apart from what a single read delivers). +- On excess the read loop fails with `InvalidDataException`. This reuses the protocol-error path: pending requests fail with that exception, the connection is marked dead, `Disconnected(ex)` is raised and later requests fail fast. + +## Consequences + +- One hostile server can no longer exhaust the memory of the client with an unbounded line. +- A legitimate reply larger than 16 MiB would be refused; none is expected on this chain, and the cap is a parameter. +- No new parser is introduced, so no new fuzz target. + +## References + +- `src/Core/Net/ElectrumClient.cs` +- `tests/PalladiumWallet.Tests/Net/ElectrumClientTests.cs` +- Related: 0009, 0034 diff --git a/docs/adr/README.md b/docs/adr/README.md index 831697c..02d0ab3 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -38,6 +38,7 @@ One ADR per feature or significant design decision. Format: `template.md`. Workf | 0032 | [Run tests, fuzz smoke and docs checks in GitHub Actions](0032-github-actions-ci.md) | Testing | Accepted | | 0033 | [Test the UI layer and CLI headlessly, gate coverage](0033-test-strategy-beyond-core.md) | Testing | Accepted | | 0034 | [Bound every network request with a timeout](0034-bound-every-network-request.md) | Network | Accepted | +| 0035 | [Cap the length of a server line](0035-cap-server-line-length.md) | Network | Accepted | ADRs `0002` onwards were written retroactively on 2026-11-03 from the code, comments and changelog. If one misstates the original reasoning, fix it in a pull request. diff --git a/src/Core/Net/ElectrumClient.cs b/src/Core/Net/ElectrumClient.cs index 4f2ad40..5e503a7 100644 --- a/src/Core/Net/ElectrumClient.cs +++ b/src/Core/Net/ElectrumClient.cs @@ -52,6 +52,16 @@ public sealed class ElectrumClient : IAsyncDisposable public static readonly TimeSpan DefaultRequestTimeout = TimeSpan.FromSeconds(30); public static readonly TimeSpan DefaultHandshakeTimeout = TimeSpan.FromSeconds(10); private readonly TimeSpan _requestTimeout; + + // Cap on a single newline-delimited reply. Without it a hostile or broken server could + // stream an endless line and grow the read buffer until the process dies (fatal on + // Android). The default is far above the largest legitimate replies (header batches of a + // few hundred KB, a large transaction up to ~2 MB of hex, a very active address history + // of a few MB). It is checked as the buffer grows, before a newline arrives; on excess the + // read loop fails with InvalidDataException, which kills the connection like any + // protocol error (pending requests fail, Disconnected is raised). + public const int DefaultMaxLineBytes = 16 * 1024 * 1024; + private readonly int _maxLineBytes; private readonly SemaphoreSlim _inFlight; private long _nextId; @@ -65,9 +75,10 @@ public sealed class ElectrumClient : IAsyncDisposable public event Action? Disconnected; private ElectrumClient(TcpClient tcp, Stream stream, string host, int port, bool useSsl, - int maxInFlight, TimeSpan requestTimeout) + int maxInFlight, TimeSpan requestTimeout, int maxLineBytes) { _requestTimeout = requestTimeout; + _maxLineBytes = maxLineBytes; _tcp = tcp; _stream = stream; Host = host; @@ -81,7 +92,7 @@ public sealed class ElectrumClient : IAsyncDisposable public static async Task ConnectAsync(string host, int port, bool useSsl, CertificatePinStore? pins = null, CancellationToken ct = default, int maxInFlight = DefaultMaxInFlight, TimeSpan? requestTimeout = null, - TimeSpan? handshakeTimeout = null) + TimeSpan? handshakeTimeout = null, int maxLineBytes = DefaultMaxLineBytes) { var tcp = new TcpClient { NoDelay = true }; try @@ -111,7 +122,7 @@ public sealed class ElectrumClient : IAsyncDisposable } var client = new ElectrumClient(tcp, stream, host, port, useSsl, maxInFlight, - requestTimeout ?? DefaultRequestTimeout); + requestTimeout ?? DefaultRequestTimeout, maxLineBytes); await client.RequestCoreAsync(handshakeTimeout ?? DefaultHandshakeTimeout, "server.version", ct, [ClientName, ProtocolVersion]); return client; @@ -229,8 +240,15 @@ public sealed class ElectrumClient : IAsyncDisposable try { while (TrySliceLine(ref buffer, out var line)) + { + if (line.Length > _maxLineBytes) + throw TooLong(); if (!line.IsEmpty) DispatchLine(line); + } + // What is left has no newline yet: it is the start of a line still arriving. + if (buffer.Length > _maxLineBytes) + throw TooLong(); } finally { @@ -259,6 +277,9 @@ public sealed class ElectrumClient : IAsyncDisposable } } + private InvalidDataException TooLong() => + new($"Server reply from {Host}:{Port} exceeds {_maxLineBytes} bytes without a newline."); + private static bool TrySliceLine(ref ReadOnlySequence buffer, out ReadOnlySequence line) { diff --git a/tests/PalladiumWallet.Tests/Net/ElectrumClientTests.cs b/tests/PalladiumWallet.Tests/Net/ElectrumClientTests.cs index dd2c910..27bd194 100644 --- a/tests/PalladiumWallet.Tests/Net/ElectrumClientTests.cs +++ b/tests/PalladiumWallet.Tests/Net/ElectrumClientTests.cs @@ -147,6 +147,54 @@ public class ElectrumClientTests Assert.Equal(big, result); } + [Fact] + public async Task A_reply_just_below_the_line_limit_arrives_intact() + { + var big = new string('x', 60_000); // line = 60 KB + JSON envelope, below the 64 KB cap + await using var server = new FakeElectrumServer(); + server.Handle("blockchain.transaction.get", _ => big); + await using var client = await ElectrumClient.ConnectAsync(server.Host, server.Port, + useSsl: false, maxLineBytes: 64 * 1024); + + Assert.Equal(big, await client.GetTransactionAsync("00").WaitAsync(Timeout)); + Assert.True(client.IsConnected); + } + + [Fact] + public async Task A_terminated_line_above_the_limit_kills_the_connection() + { + var big = new string('x', 100_000); + await using var server = new FakeElectrumServer(); + server.Handle("blockchain.transaction.get", _ => big); + await using var client = await ElectrumClient.ConnectAsync(server.Host, server.Port, + useSsl: false, maxLineBytes: 64 * 1024); + var disconnected = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + client.Disconnected += ex => disconnected.TrySetResult(ex); + + await Assert.ThrowsAsync(() => client.GetTransactionAsync("00").WaitAsync(Timeout)); + Assert.IsType(await disconnected.Task.WaitAsync(Timeout)); + Assert.False(client.IsConnected); + await Assert.ThrowsAsync(() => client.GetTransactionAsync("00").WaitAsync(Timeout)); + } + + [Fact] + public async Task An_unterminated_line_above_the_limit_fails_before_any_newline_arrives() + { + await using var server = new FakeElectrumServer(); + server.Handle("server.banner", _ => FakeElectrumServer.NoResponse); + await using var client = await ElectrumClient.ConnectAsync(server.Host, server.Port, + useSsl: false, maxLineBytes: 64 * 1024); + var disconnected = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + client.Disconnected += ex => disconnected.TrySetResult(ex); + + var pending = client.BannerAsync(); + await server.SendUnterminatedAsync(new string('x', 100_000)); // never followed by '\n' + + await Assert.ThrowsAsync(() => pending.WaitAsync(Timeout)); + Assert.IsType(await disconnected.Task.WaitAsync(Timeout)); + Assert.False(client.IsConnected); + } + [Fact] public async Task A_request_without_an_answer_fails_with_TimeoutException() { diff --git a/tests/PalladiumWallet.Tests/Net/FakeElectrumServer.cs b/tests/PalladiumWallet.Tests/Net/FakeElectrumServer.cs index 898da00..b3cd33f 100644 --- a/tests/PalladiumWallet.Tests/Net/FakeElectrumServer.cs +++ b/tests/PalladiumWallet.Tests/Net/FakeElectrumServer.cs @@ -79,6 +79,19 @@ public sealed class FakeElectrumServer : IAsyncDisposable await WriteLineAsync(stream, writeLock, bytes); } + /// Sends raw bytes with no trailing newline (an unterminated, still-growing line). + public async Task SendUnterminatedAsync(string text) + { + var bytes = System.Text.Encoding.UTF8.GetBytes(text); + foreach (var (stream, writeLock, _) in _clients) + { + await writeLock.WaitAsync(); + try { await stream.WriteAsync(bytes); await stream.FlushAsync(); } + catch (Exception) { } + finally { writeLock.Release(); } + } + } + /// Abruptly closes every accepted connection (tests the client's failure paths). public void DropClients() {