fix(net): prevent hello message replay

[xxo1shine]

What does this PR do?

Previously, incoming Hello messages were not checked for timestamp freshness, allowing previously signed witness messages to be replayed indefinitely.

This change introduces timestamp freshness validation and tracks the latest successfully verified message timestamp for each witness, rejecting expired or replayed messages to prevent replay attacks. The existing signature length validation is preserved.

Fixes #6675

Why are these changes required?

This PR has been tested by:

• Unit Tests
• Manual Testing

Follow up

Extra details

[lxcmyf]

[MUST] This review found two blocking issues; see the inline comments.

[317787106]

[MUST] Obtaining a valid signed Hello is already the premise of the replay attack this PR addresses. The freshness window limits its lifetime, but concurrent copies can still pass the cache check and grant trust to multiple IPs.

With controlled thread scheduling, I reproduced both duplicate acceptance and timestamp rollback: an older request finishes after a newer one, overwrites the cached timestamp, and allows the newer Hello to be replayed successfully.

A synchronized block around the timestamp comparison and cache update, after signature verification, would keep the critical section small.. Only requests that successfully update the timestamp should grant trust. Given that this directly affects the replay protection being introduced, I think we should fix it and add concurrency coverage in this PR.

Suggest to replace this put with following:

synchronized (helloReplayCache) {
    Long lastTimestamp = helloReplayCache.getIfPresent(msg.getAddress());
    if (lastTimestamp != null && timestamp <= lastTimestamp) {
      logger.warn("HelloMessage from {}, timestamp {} is not greater than last {}",
          channel.getInetAddress(), timestamp, lastTimestamp);
      return false;
    }

    helloReplayCache.put(msg.getAddress(), timestamp);
}

Move the timestamp check from lines 175–180 into this synchronized block so that the check and update are atomic.

[xxo1shine]

The goal of this fix is to prevent historical signed Hello messages from being replayed indefinitely to establish connections. It does not bind a message to the current connection.

Even if the cache check and update are made atomic, an attacker who intercepts a valid message and delivers it before the legitimate request can still have the replay accepted, while the legitimate request is rejected. The proposed synchronized block can prevent concurrent duplicate acceptance and timestamp rollback, but it does not address this first-arrival replay scenario.

Addressing that scenario would require a handshake protocol mechanism such as a receiver-issued challenge or session binding. For this fix targeting historical message replay, I would prefer to keep the current scope and not treat this concurrency hardening as a blocker for this PR.

[halibobo1205]

Agreed that first-arrival replay is out of scope. The race is a different case: +1 to 317787106's reproduction, I get the same on this head merged with the current release_v4.8.3 (one hello accepted on two channels, and a cache rollback that lets an already-accepted hello through again). That's a repeated message getting through, which the description says this PR prevents.

Not a blocker for me, but since the branch needs a rebase for #6999 anyway, it's worth fixing here: after the signature check, replace the put at L198 with helloReplayCache.asMap().compute(...) (keep last when timestamp <= last), and grant trust only through a flag set inside the lambda. For a concurrency test, hook manager.getDynamicPropertiesStore() (L190) with a CyclicBarrier; under the default test config L190 never reaches getAccountStore().

[xxo1shine]

This change primarily addresses long-term replay of stale hello messages and repeated submissions of messages that have already been cached. The race you identified requires processing of multiple hello messages from the same witness to overlap between the cache check and update. I expect this to be very unlikely during normal operation, so I do not consider it a priority for this change. Even if it occurs, the messages must still pass signature verification and the timestamp-window check, so I consider the incremental impact limited. I therefore prefer to retain the current implementation without adding atomic cache updates in this PR.

[halibobo1205]

Requesting changes for one mechanical issue: after #6999 the test helper no longer compiles (inline). Otherwise the core fix looks right.

Before merging:

• Rebase, fix the test helper, and re-run CI.
• Atomic check-and-update (L198 thread): non-blocking from my side.
• Update the description: the window is a fixed ±5 min with no config key. Since Fixes #6675 will close the issue, please also note what stays open (first-arrival and cross-node replay).

[halibobo1205]

[MUST] This no longer compiles against the current release_v4.8.3: #6999 removed SignUtils.fromPrivate(byte[], boolean), Sha256Hash.of(boolean, byte[]) and isECKeyCryptoEngine(). The merge is textually clean, so GitHub still shows the PR as mergeable. Suggested fix:

SignInterface engine = SignUtils.fromPrivate(ByteArray.fromHexString(key));
ByteString sig = ByteString.copyFrom(engine.Base64toBytes(engine
    .signHash(Sha256Hash.of(ByteArray.fromLong(timestamp)).getBytes())));

[xxo1shine]

Fixed.

[halibobo1205]

Agreed that first-arrival replay is out of scope. The race is a different case: +1 to 317787106's reproduction, I get the same on this head merged with the current release_v4.8.3 (one hello accepted on two channels, and a cache rollback that lets an already-accepted hello through again). That's a repeated message getting through, which the description says this PR prevents.

Not a blocker for me, but since the branch needs a rebase for #6999 anyway, it's worth fixing here: after the signature check, replace the put at L198 with helloReplayCache.asMap().compute(...) (keep last when timestamp <= last), and grant trust only through a flag set inside the lambda. For a concurrency test, hook manager.getDynamicPropertiesStore() (L190) with a CyclicBarrier; under the default test config L190 never reaches getAccountStore().

[317787106]

[SHOULD] Please use a seconds-scale freshness window here. msg.getTimestamp() originates from Channel.startTime, initialized with System.currentTimeMillis() when the channel is created. HandshakeService.startHandshake() passes that value into the Hello message, and RelayService.fillHelloMessage() signs it. It therefore represents the current connection attempt, not a timestamp intended for reuse across connections.

A ±5-minute window allows a signed Hello from several minutes ago to pass the freshness check whenever the receiving node has not cached it. That is too permissive for handshake authentication. Please use a seconds-scale threshold, with an explicit budget for clock skew and connection/handshake delay, and add tests for both window boundaries. The timestamp should remain in milliseconds; the change is to the allowed tolerance, e.g. TimeUnit.SECONDS.toMillis(...).

[xxo1shine]

This change primarily limits long-term replay of historical signed Hello messages. The five-minute window accommodates node clock skew and connection/handshake delays, while the cache rejects repeated messages that have already been received. A shorter window would further reduce the replay interval, but would also tighten the tolerance for legitimate connections. We currently have no data supporting a specific seconds-scale threshold, so I prefer to retain the current value and consider a stricter timing budget as a follow-up.

[317787106]

[SHOULD] Please separate the maximum Hello age from the allowed future clock skew.

msg.getTimestamp() originates from Channel.startTime, recorded when the channel is created. The past-side tolerance therefore needs to cover clock skew plus connection/handshake and processing delays. The future-side tolerance only needs to cover clock skew: transmission and processing delays make a message older, not further into the future.

These are different budgets and should be justified independently. Please use separate limits rather than a shared ±5-minute threshold, document the assumptions behind each, and add boundary tests for both directions

[xxo1shine]

I agree that the timing budgets differ in the two directions, and transmission or processing delays do not justify tolerance for future timestamps. However, this change primarily limits long-term replay of historical Hello messages and uses a shared window as a conservative clock-tolerance bound. We currently have no data supporting separate thresholds, so I prefer to retain the current implementation and consider independent timing budgets as a follow-up.

[317787106]

I don’t think the five-minute threshold is appropriate for the future side. libp2p’s Ping/Pong validation uses Parameter.NETWORK_TIME_DIFF (1,000 ms) as its future timestamp tolerance. Network and processing delays make a timestamp older; they do not justify accepting one five minutes ahead.

Please use a separate, seconds-scale limit for timestamp > now, while keeping a larger past window if needed. Otherwise, a signed Hello with a future timestamp can advance helloReplayCache and cause legitimate Hellos to be rejected if the peer’s clock is corrected.

[xxo1shine]

The five-minute window is intended to tolerate clock skew between nodes. Ping/Pong's one-second limit is a useful reference, but it does not necessarily require signed HELLO messages to use the same threshold. A tighter window would also make legitimate HELLO messages from nodes with clock skew more likely to be rejected.

The clock-correction scenario you describe is valid: rejecting a lower timestamp is a consequence of the current replay protection requiring timestamps to increase. Reducing the future tolerance can mitigate that effect, but cannot eliminate it. This PR primarily addresses the indefinite replay of historical signed HELLO messages. I accept the current trade-off and prefer to retain the five-minute window without introducing separate thresholds in this PR.