Skip to content

Harden sync message validation and resource controls #6942

Open

Description

@xxo1shine

Summary

Several validation, rate-limiting, and deduplication issues exist in sync message processing:

  • ChainInventoryMessage.remainNum is not validated against integer overflow, allowing the future block-height upper-bound check to be bypassed.
  • SYNC_BLOCK_CHAIN rate limiting can be bypassed under a specific combination of fields.
  • An off-by-one mismatch exists between the block-fetch deduplication window and the deduplication cache size, allowing a block ID at the window boundary to be requested repeatedly.

These areas should be hardened to ensure stricter validation and more reliable rate limiting and deduplication during synchronization.

Root Cause

1. Future block-height validation bypass through remainNum overflow

ChainInventoryMsgHandler.check() uses the following comparison to limit the future block height claimed by a peer:

lastNum + remainNum > maxFutureNum

However, remainNum is a peer-controlled signed 64-bit value and is not range-checked. When it is set close to Long.MAX_VALUE, the addition may overflow and wrap to a negative value. The comparison then evaluates to false, bypassing the validation intended to reject the message.

2. SYNC_BLOCK_CHAIN rate-limit bypass

The rate-limit check in SyncBlockChainMsgHandler depends on peer.getRemainNum() > 0.

A crafted request combining remainNum == 0 with a forged trailing block ID can bypass the rate-limit check, causing the limiter not to be applied to the request.

Rate limiting should apply to the incoming request itself rather than depend on peer-controlled or previously derived synchronization state.

3. Off-by-one mismatch between the block-fetch window and cache size

FetchInvDataMsgHandler permits block requests within the inclusive range:

[lastSyncBlockId - 2 * SYNC_FETCH_BATCH_NUM, lastSyncBlockId]

With SYNC_FETCH_BATCH_NUM = 2000, this is:

[last - 4000, last]

The inclusive window contains 4,001 distinct block numbers. However, syncBlockIdCache, which prevents duplicate requests, has a maximum size of:

2 * SYNC_FETCH_BATCH_NUM = 4000

The valid request window is therefore one entry larger than the deduplication cache.

After 4,000 entries have filled the cache, inserting the 4,001st valid entry may evict an older entry due to the cache's maximum-size eviction policy. Because the evicted block ID remains inside the valid request window, it can be requested again and pass the getIfPresent() deduplication check.

Reproduction

1. remainNum overflow

Construct a ChainInventoryMessage that satisfies the preceding continuity and linkage checks, but set remainNum close to Long.MAX_VALUE.

The addition in the upper-bound check overflows to a negative value, allowing the message to pass validation.

2. Rate-limit bypass

Construct a SYNC_BLOCK_CHAIN request that combines remainNum == 0 with a specially crafted trailing block ID.

Observe that the rate limiter is not invoked as expected.

3. Deduplication off-by-one

  1. Fill syncBlockIdCache with 4,000 block IDs from the valid request window.
  2. Request the 4,001st block ID in the window, causing an older cache entry to be evicted.
  3. Request the evicted block ID again.
  4. Because the ID is still within the valid range but no longer exists in the cache, it passes the getIfPresent() deduplication check and is fetched again.

Impact

  • Validation and rate limiting: A peer may cause the node to accept synchronization messages that should have been rejected or process certain requests without applying the intended rate limit. Other continuity and linkage checks still constrain these messages, so the issues do not affect consensus correctness or asset security.
  • Deduplication off-by-one: A block ID within the valid request window can fall outside the deduplication cache and become requestable again, causing additional redundant block-fetch work. The impact is bounded: because the handler checks every hash and rejects the entire message if any entry is already cached, at most one entry within the valid window can fall outside the cache at a given time. This results in a rolling single-entry bypass rather than bypassing the entire window.
  • Overall, the impact is limited to resource consumption and processing efficiency in the synchronization flow. It does not affect consensus correctness or asset security.

Suggested Fix

  1. remainNum: Reject negative or otherwise out-of-range remainNum values. Use Math.addExact() with explicit overflow handling, or use a subtraction-based comparison after validating the relevant ranges, to prevent the future block-height check from being bypassed by integer overflow.

  2. SYNC_BLOCK_CHAIN rate limiting: Remove the peer.getRemainNum() > 0 precondition and apply rate limiting to every received SYNC_BLOCK_CHAIN request.

  3. Deduplication off-by-one: Increase the maximum size of syncBlockIdCache so that it covers the entire valid request window:

    2 * SYNC_FETCH_BATCH_NUM + 1 = 4001
    

    Alternatively, narrow the permitted block-fetch window by one entry so that it matches the existing cache capacity.

Activity

  1. jakamobiii commented on Sep 2, 2026

    @jakamobiii

    @xxo1shine Besides preventing overflow in lastNum + remainNum, do we explicitly reject negative remainNum values as well? It would be good to add boundary tests for -1, Long.MIN_VALUE, Long.MAX_VALUE, and the exact valid upper bound to ensure the range validation and overflow handling are consistent.

  2. xxo1shine commented on Sep 2, 2026

    @xxo1shine
    CollaboratorAuthor

    @jakamobiii Yes. The fix validates the range of remainNum before performing the height calculation, and negative values are rejected directly. We'll also add unit tests for these boundary values, including the maximum valid value and values exceeding the upper bound.

  3. lxcmyf commented on Sep 4, 2026

    @lxcmyf
    Collaborator

    Two parts of the proposed fix need stronger invariants and tests:

    1. peer.remainNum is written by both ChainInventoryMsgHandler and SyncBlockChainMsgHandler, so it represents state from two synchronization directions at different points in the flow. Removing the remainNum > 0 condition makes the limiter unconditional, but it also changes initial/resumed handshake behavior and can disconnect a legitimate peer once tryAcquire() fails. Please document the exact packet/state sequence for the bypass and add sequence-level tests for both the malicious case and normal initial/resumed synchronization, rather than testing only a peer mocked with remainNum == 0.

    2. Increasing syncBlockIdCache from 4,000 to 4,001 only matches one static inclusive window. The cache lives for the entire peer connection while lastSyncBlockId advances, and entries that have fallen below the current minimum remain in the cache. With out-of-order requests, those stale entries can still cause an entry inside the current window to be evicted, so the +1 change does not by itself establish the deduplication invariant. Consider pruning entries by block height when the window advances, or otherwise making the cache explicitly window-aware. The regression test should advance through at least two windows and vary request order.

    For remainNum, ensure the negative-value check is unconditional and occurs outside the current headBlockNum > 0 branch; the future-height comparison should then use an overflow-safe subtraction or checked addition. Boundary coverage should include a node at height zero as well as -1, Long.MIN_VALUE, Long.MAX_VALUE, the exact allowed value, and allowed + 1.

  4. xxo1shine commented on Sep 9, 2026

    @xxo1shine
    CollaboratorAuthor

    @lxcmyf, Regarding rate limiting, we have analyzed the initial and resumed synchronization flows and concluded that removing the remainNum > 0 condition will not affect normal synchronization. We plan to apply rate limiting uniformly to incoming SYNC_BLOCK_CHAIN requests.

    For deduplication, our current analysis indicates that a capacity of 4,001 is sufficient to cover the valid request window. We will add validation for the advancing-window and out-of-order request scenarios you described to confirm that stale entries do not cause entries within the current window to be evicted prematurely.

    The negative-value check for remainNum will be placed outside the headBlockNum > 0 branch. We will also use an overflow-safe height comparison and add tests covering a node at height zero, negative values, extreme values, and values around the allowed upper bound.

  5. removed this from the GreatVoyage-v4.8.3 milestone on Sep 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    topic:netp2p net work, synchronization

    Type

    No type

    Projects

    • Status
      No status

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions