fix(net): cap the length of a server line
A hostile server could stream an endless line with no newline and exhaust the client's memory. A single reply is now capped at 16 MiB (configurable via maxLineBytes in ConnectAsync); on excess the connection fails with InvalidDataException. See ADR 0035.
This commit is contained in:
1 parent
3be761d97e
commit
65c515bf7a
7 files changed
+121
-3
No files matched your search
@@ -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
|
||||
|
||||
@@ -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`,
|
||||
|
||||
@@ -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
|
||||
@@ -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.
|
||||
@@ -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<Exception?>? 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<ElectrumClient> 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<byte> buffer,
|
||||
out ReadOnlySequence<byte> line)
|
||||
{
|
||||
|
||||
@@ -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<Exception?>(TaskCreationOptions.RunContinuationsAsynchronously);
|
||||
client.Disconnected += ex => disconnected.TrySetResult(ex);
|
||||
|
||||
await Assert.ThrowsAsync<InvalidDataException>(() => client.GetTransactionAsync("00").WaitAsync(Timeout));
|
||||
Assert.IsType<InvalidDataException>(await disconnected.Task.WaitAsync(Timeout));
|
||||
Assert.False(client.IsConnected);
|
||||
await Assert.ThrowsAsync<IOException>(() => 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<Exception?>(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<InvalidDataException>(() => pending.WaitAsync(Timeout));
|
||||
Assert.IsType<InvalidDataException>(await disconnected.Task.WaitAsync(Timeout));
|
||||
Assert.False(client.IsConnected);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task A_request_without_an_answer_fails_with_TimeoutException()
|
||||
{
|
||||
|
||||
@@ -79,6 +79,19 @@ public sealed class FakeElectrumServer : IAsyncDisposable
|
||||
await WriteLineAsync(stream, writeLock, bytes);
|
||||
}
|
||||
|
||||
/// <summary>Sends raw bytes with no trailing newline (an unterminated, still-growing line).</summary>
|
||||
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(); }
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>Abruptly closes every accepted connection (tests the client's failure paths).</summary>
|
||||
public void DropClients()
|
||||
{
|
||||
|
||||
Reference in new issue
Block a user