Skip to content

fix(protocol): don't terminate retrieval threads on messages with trailing bytes (DoS)#5105

Open
damip wants to merge 1 commit into
mainfrom
fix/protocol-retrieval-survives-trailing-bytes
Open

fix(protocol): don't terminate retrieval threads on messages with trailing bytes (DoS)#5105
damip wants to merge 1 commit into
mainfrom
fix/protocol-retrieval-survives-trailing-bytes

Conversation

@damip

@damip damip commented Jul 21, 2026

Copy link
Copy Markdown
Member

Summary

The block and operation retrieval workers each run a single long-lived select! loop
that serves all peers. When a network message deserialized successfully but left
trailing bytes (!rest.is_empty()), both loops did:

if !rest.is_empty() {
    println!("Error: message not fully consumed");
    return; // <-- terminates the whole shared retrieval thread
}

A return here kills the entire retrieval thread, so a single peer sending a valid
message prefix followed by junk bytes can permanently stop block/operation protocol
handling for every peer — a denial-of-service. The stray println! also bypasses the
logging framework.

Change

In both block_handler::retrieval and operation_handler::retrieval, treat a message
with trailing bytes as a per-message error: log a warn! (including the offending peer
and the number of trailing bytes) and continue the loop instead of returning. The
worker keeps serving other peers.

Testing

  • cargo build / cargo clippy clean for massa_protocol_worker.
  • The fix is a control-flow change (returncontinue) inside the workers' large
    select! loops; a faithful regression test would require standing up the full
    retrieval worker with network/pool/mock wiring, so no bespoke unit test is added.
    The change is small and self-contained.

Risk

Low — well-behaved peers never send trailing bytes, so the only behavioural change is
that a malformed/hostile message no longer tears down the shared worker. We
intentionally keep the change minimal (log + skip) and do not add peer-banning here;
banning on trailing bytes can be considered as a follow-up.

Tracking: findings F120 and F128.

… bytes

The block and operation retrieval workers each run one long-lived loop for
all peers. On a deserialized message with trailing bytes they did
'println!(...); return;', terminating the shared thread, so one peer could
DoS block/operation handling for everyone. Log a warning and continue the
loop instead, keeping the worker alive.

Findings F120, F128.

Co-authored-by: Cursor <cursoragent@cursor.com>

@Leo-Besancon Leo-Besancon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, indeed peer banning as follow-up seems like a good idea.
Is there an issue regarding this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants