fix(kerykeion): bound what a peer can accumulate during the handshake - #437
Merged
Conversation
added 3 commits
August 21, 2026 01:06
The handshake loop pushed every NodeInfo and every Channel a peer sent into local vectors with no ceiling. Its timeout bounds how long a device may talk, not how much it may say, so a peer could fill memory inside that window. Neither bound is new. MAX_LIVE_NODES already exists as the shared ceiling for unauthenticated node identities, added for this exact threat under #204, and NodeDb honours it -- this local accumulation simply did not. MAX_CHANNELS is the protocol's own maximum, and anything past it is a peer repeating itself. So this applies ceilings the codebase already declares rather than inventing new ones. The node list warns once when it fills, since a real mesh reaching four thousand identities is worth seeing. The test floods twenty times the channel maximum and requires the list to stop at it, with an ordinary three-channel handshake alongside so the cap cannot pass against a loop that collects nothing. Refs #229
Channel has a single field, so ..Default::default() specified nothing.
The handshake requires it before anything else, so both new tests failed with 'radio did not send MyNodeInfo' before reaching the accumulation they exercise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Another of #229's clauses: the handshake accumulated peer-supplied
NodeInfoandChannelmessagesinto local vectors with no ceiling.
Why the existing timeout is not a bound
handshake_with_configalready aborts afterHandshakeConfig::timeout_secs, which is easy to mistakefor a limit. It bounds how long a device may talk, not how much it may say. Inside that window
a peer can stream messages as fast as the link allows, and both vectors grow for every one.
Both ceilings already exist
This applies limits the codebase already declares rather than inventing new ones:
MAX_LIVE_NODESwas added under Bound node-table and topology cardinality to stop OTA memory-exhaustion DoS #204 as the shared ceiling for unauthenticated node identities,with a comment stating the reason outright — "
fromon an inbound frame is unauthenticated, sonothing stops a hostile peer from announcing distinct node identities without bound".
NodeDbandMeshTopologyhonour it. This local accumulation did not, so the handshake was a way around a boundthe rest of the crate respects.
MAX_CHANNELSis the protocol's own maximum. Anything past it is a peer repeating itself.The node list warns once as it fills, because a real mesh reaching four thousand identities is worth
seeing rather than silently truncating.
Test
FloodMocksends twenty times the channel maximum before completing the handshake; the collected listmust stop at
MAX_CHANNELS.an_ordinary_channel_count_is_collected_in_fullis its anti-vacuity partner — three channels sent,three collected. Without it the cap would pass against a loop that collects nothing at all.
Scope
The outbound pending queue, #229's remaining unbounded-growth clause, is deliberately not here.
OutboundQueue::enqueuereturns()and inserts unconditionally, so bounding it means deciding whathappens to the message that does not fit — reject it, or displace the lowest-priority one — and that
changes the caller-visible contract. It deserves its own change rather than riding along with a fix
that needed no such decision.
Refs #229