Conversation
9a6dbb8 to
56038ea
Compare
Hello messages were accepted without checking timestamp freshness, allowing a previously signed witness message to be replayed indefinitely. Use a fixed five-minute freshness threshold and track the latest verified timestamp per witness so stale or repeated messages are rejected before trust is granted.
56038ea to
afc874a
Compare
| } | ||
|
|
||
| long now = System.currentTimeMillis(); | ||
| if (now - msg.getTimestamp() > HELLO_MESSAGE_TIMESTAMP_THRESHOLD) { |
There was a problem hiding this comment.
[MUST] This check only rejects old timestamps. A timestamp beyond the future side of the window, or an extreme value that overflows the subtraction, is accepted and cached, causing subsequent normal Hello messages to be rejected for a long time. Use an overflow-safe two-sided comparison (timestamp < now - threshold || timestamp > now + threshold) and add future-timestamp coverage.
There was a problem hiding this comment.
I agree that we should validate both sides of the timestamp window. If an SR's clock is ahead, a future timestamp with a valid signature can be cached. Even after the clock is corrected, normal Hello messages will still be rejected because their timestamps are lower than the cached value. Block timestamp validation does not resolve this recovery issue. I will use timestamp < now - threshold || timestamp > now + threshold and add tests for future timestamps and extreme values, verifying that normal Hello messages are still accepted after invalid timestamps are rejected.
| return false; | ||
| } | ||
|
|
||
| Long lastTimestamp = helloReplayCache.getIfPresent(msg.getAddress()); |
There was a problem hiding this comment.
[MUST] The getIfPresent → signature verification → put sequence is not atomic. Replays of the same address/timestamp on different channels can pass concurrently and both add trust. After signature verification, commit the timestamp with an address-scoped atomic compare-and-update or equivalent lock, and add a concurrency test.
There was a problem hiding this comment.
Exploiting this scenario requires first obtaining a Hello message with a valid witness signature that is still within the freshness window, then replaying it concurrently over multiple connections. Given these prerequisites and the added complexity of synchronization or atomic update logic, we will retain the current implementation without adding concurrency control in this PR.
What does this PR do?
Hello messages were accepted without checking timestamp freshness, allowing a previously signed witness message to be replayed indefinitely.
Add a configurable freshness threshold and track the latest verified timestamp per witness so stale or repeated messages are rejected before trust is granted. Adapt the setting to the current NodeConfig-based configuration model while retaining signature-length validation.
Fixes #6675
Why are these changes required?
This PR has been tested by:
Follow up
Extra details