Skip to content

fix(net): prevent hello message replay #6972

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lvs0075 merged 4 commits into from
Oct 16, 2026
+153 −0
Merged

fix(net): prevent hello message replay #6972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
afc874a
fix(net): prevent hello message replay
Apr 20, 2026
c3847e9
fix(net): validate both sides of hello timestamp window
Sep 18, 2026
dd63f3a
Merge branch 'release_v4.8.3' into fix/net-hello-message-replay
Oct 12, 2026
f482bf1
fix(test): adapt relay hello signing to updated crypto API
Oct 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions framework/src/main/java/org/tron/core/net/service/relay/RelayService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package org.tron.core.net.service.relay;

import com.google.common.cache.Cache;
import com.google.common.cache.CacheBuilder;
import com.google.protobuf.ByteString;
import java.net.InetSocketAddress;
import java.util.Arrays;
Expand Down Expand Up @@ -42,6 +44,7 @@
public class RelayService {

private static final int MAX_PEER_COUNT_PER_ADDRESS = 5;
private static final long HELLO_MESSAGE_TIMESTAMP_THRESHOLD = TimeUnit.MINUTES.toMillis(5);

@317787106 317787106 Oct 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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 xxo1shine Oct 13, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.


@Autowired
private ChainBaseManager chainBaseManager;
Expand Down Expand Up @@ -74,6 +77,10 @@ public class RelayService {

private int maxFastForwardNum = Args.getInstance().getMaxFastForwardNum();

private final Cache<ByteString, Long> helloReplayCache = CacheBuilder.newBuilder()
.maximumSize(100)
.build();

public void init() {
manager = ctx.getBean(Manager.class);
witnessScheduleStore = ctx.getBean(WitnessScheduleStore.class);
Expand Down Expand Up @@ -153,6 +160,22 @@ public boolean checkHelloMessage(HelloMessage message, Channel channel) {
return false;
}

long now = System.currentTimeMillis();
long timestamp = msg.getTimestamp();
if (timestamp < now - HELLO_MESSAGE_TIMESTAMP_THRESHOLD
|| timestamp > now + HELLO_MESSAGE_TIMESTAMP_THRESHOLD) {
Comment on lines +165 to +166

@317787106 317787106 Oct 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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 xxo1shine Oct 13, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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 317787106 Oct 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 xxo1shine Oct 16, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

logger.warn("HelloMessage from {}, timestamp {} is outside the window around {}",
channel.getInetAddress(), timestamp, now);
return false;
}

Long lastTimestamp = helloReplayCache.getIfPresent(msg.getAddress());
Comment thread
lxcmyf marked this conversation as resolved.
if (lastTimestamp != null && msg.getTimestamp() <= lastTimestamp) {
logger.warn("HelloMessage from {}, timestamp {} is not greater than last {}",
channel.getInetAddress(), msg.getTimestamp(), lastTimestamp);
return false;
}

boolean flag;
try {
Sha256Hash hash = Sha256Hash.of(ByteArray.fromLong(msg.getTimestamp()));
Expand All @@ -167,6 +190,7 @@ public boolean checkHelloMessage(HelloMessage message, Channel channel) {
flag = Arrays.equals(sigAddress, witnessPermissionAddress);
}
if (flag) {
helloReplayCache.put(msg.getAddress(), msg.getTimestamp());

@317787106 317787106 Sep 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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 xxo1shine Oct 9, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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 halibobo1205 Oct 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 xxo1shine Oct 12, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

TronNetService.getP2pConfig().getTrustNodes().add(channel.getInetAddress());
DesensitizedConverter.addSensitive(channel.getInetAddress().toString().substring(1),
ByteArray.toHexString(msg.getAddress().toByteArray()));
Expand Down
129 changes: 129 additions & 0 deletions framework/src/test/java/org/tron/core/net/services/RelayServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import static org.mockito.Mockito.doNothing;
import static org.mockito.Mockito.mock;

import com.google.common.cache.Cache;
import com.google.protobuf.ByteString;
import java.lang.reflect.Field;
import java.lang.reflect.InvocationTargetException;
Expand All @@ -13,6 +14,7 @@
import java.util.Comparator;
import java.util.List;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import javax.annotation.Resource;
import lombok.extern.slf4j.Slf4j;
import org.bouncycastle.util.encoders.Hex;
Expand Down Expand Up @@ -72,6 +74,11 @@ public static void init() {
@After
public void clearPeers() {
closePeer();
Cache<?, ?> cache =
(Cache<?, ?>) ReflectUtils.getFieldObject(service, "helloReplayCache");
if (cache != null) {
cache.invalidateAll();
}
}

@Test
Expand Down Expand Up @@ -246,6 +253,128 @@ private void testCheckHelloMessage() {
}
}

private HelloMessage buildSignedHello(long timestamp) throws Exception {
String key = "0154435f065a57fec6af1e12eaa2fa600030639448d7809f4c65bdcf8baed7e5";
ByteString address = getFromHexString("418A8D690BF36806C36A7DAE3AF796643C1AA9CC01");
Node node = new Node(NetUtil.getNodeId(), "127.0.0.1", null, 10001);
HelloMessage msg = new HelloMessage(node, timestamp,
ChainBaseManager.getChainBaseManager());
SignInterface engine = SignUtils.fromPrivate(ByteArray.fromHexString(key));
ByteString sig = ByteString.copyFrom(engine.Base64toBytes(engine
.signHash(Sha256Hash.of(ByteArray.fromLong(timestamp)).getBytes())));
msg.setHelloMessage(msg.getHelloMessage().toBuilder()
.setAddress(address)
.setSignature(sig)
.build());
return msg;
}

private Channel buildChannel() {
InetSocketAddress addr = new InetSocketAddress("127.0.0.1", 10001);
Channel c = mock(Channel.class);
Mockito.when(c.getInetSocketAddress()).thenReturn(addr);
Mockito.when(c.getInetAddress()).thenReturn(addr.getAddress());
return c;
}

private void setupRelayServiceDeps() throws Exception {
Field f1 = service.getClass().getDeclaredField("witnessScheduleStore");
f1.setAccessible(true);
f1.set(service, chainBaseManager.getWitnessScheduleStore());
Field f2 = service.getClass().getDeclaredField("manager");
f2.setAccessible(true);
f2.set(service, dbManager);
ReflectUtils.setFieldValue(tronNetService, "p2pConfig", new P2pConfig());
}

@Test
public void testCheckHelloMessage_staleTimestamp() throws Exception {
initWitness();
setupRelayServiceDeps();
Args.getInstance().fastForward = true;
long threshold = TimeUnit.MINUTES.toMillis(5);
long staleTimestamp = System.currentTimeMillis() - threshold - 1000;
HelloMessage msg = buildSignedHello(staleTimestamp);
boolean result = service.checkHelloMessage(msg, buildChannel());
Assert.assertFalse(result);
}

@Test
public void testCheckHelloMessage_futureTimestampDoesNotPoisonCache() throws Exception {
assertTimestampRejectedWithoutPoisoningCache(
System.currentTimeMillis() + TimeUnit.HOURS.toMillis(1));
}

@Test
public void testCheckHelloMessage_maxTimestampDoesNotPoisonCache() throws Exception {
assertTimestampRejectedWithoutPoisoningCache(Long.MAX_VALUE);
}

@Test
public void testCheckHelloMessage_minTimestampDoesNotPoisonCache() throws Exception {
assertTimestampRejectedWithoutPoisoningCache(Long.MIN_VALUE);
}

private void assertTimestampRejectedWithoutPoisoningCache(long timestamp) throws Exception {
initWitness();
setupRelayServiceDeps();
Args.getInstance().fastForward = true;
Channel channel = buildChannel();
Assert.assertFalse(service.checkHelloMessage(buildSignedHello(timestamp), channel));
Assert.assertFalse(TronNetService.getP2pConfig().getTrustNodes()
.contains(channel.getInetAddress()));
Assert.assertTrue(service.checkHelloMessage(
buildSignedHello(System.currentTimeMillis()), channel));
}

@Test
public void testCheckHelloMessage_freshTimestamp() throws Exception {
initWitness();
setupRelayServiceDeps();
Args.getInstance().fastForward = true;
long freshTimestamp = System.currentTimeMillis();
HelloMessage msg = buildSignedHello(freshTimestamp);
boolean result = service.checkHelloMessage(msg, buildChannel());
Assert.assertTrue(result);
}

@Test
public void testCheckHelloMessage_replayRejected() throws Exception {
initWitness();
setupRelayServiceDeps();
Args.getInstance().fastForward = true;
long t = System.currentTimeMillis();
HelloMessage msg1 = buildSignedHello(t);
Assert.assertTrue(service.checkHelloMessage(msg1, buildChannel()));
HelloMessage msg2 = buildSignedHello(t);
Assert.assertFalse(service.checkHelloMessage(msg2, buildChannel()));
}

@Test
public void testCheckHelloMessage_strictlyLargerTimestampPasses() throws Exception {
initWitness();
setupRelayServiceDeps();
Args.getInstance().fastForward = true;
long t = System.currentTimeMillis();
Assert.assertTrue(service.checkHelloMessage(buildSignedHello(t), buildChannel()));
Assert.assertTrue(service.checkHelloMessage(buildSignedHello(t + 1), buildChannel()));
}

@Test
public void testCheckHelloMessage_badSigDoesNotPoisonCache() throws Exception {
initWitness();
setupRelayServiceDeps();
Args.getInstance().fastForward = true;
long t = System.currentTimeMillis();
HelloMessage badMsg = buildSignedHello(t);
badMsg.setHelloMessage(badMsg.getHelloMessage().toBuilder()
.setSignature(ByteString.copyFrom(new byte[65]))
.build());
Assert.assertFalse(service.checkHelloMessage(badMsg, buildChannel()));
HelloMessage goodMsg = buildSignedHello(t);
Assert.assertTrue(service.checkHelloMessage(goodMsg, buildChannel()));
}

@Test
public void testNullWitnessAddress() {
try {
Expand Down
Loading