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.
| 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.
3 participants
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