fix(ofp): nonce check after HMAC, 64KB message cap, recipient node_id in handshake HMAC#3920
Conversation
…cipient in handshake HMAC #3880 — NonceTracker DoS: check_and_record was called before HMAC verification in handle_inbound, allowing any unauthenticated TCP client to trigger the DashMap retain() sweep and fill nonce capacity without knowing shared_secret. Fix: verify HMAC first in handle_inbound; record nonce only on success. Also applies the same order fix to connect_to_peer_with_id/send_to_peer ack path. Additionally amortize the O(n) GC sweep: retain() now only runs when the map reaches 80% of max_entries instead of on every insert. #3876 — Budget drain via oversized messages: AgentMessage payloads from federated peers had no size limit before reaching the kernel's LLM pipeline. A peer sharing the same shared_secret could send 16 MB messages and drain the receiver's LLM budget at no cost. Fix: add MAX_PEER_MESSAGE_BYTES = 65_536 constant and reject oversized payloads with code 413 in handle_request_in_loop before calling handle_agent_message. #3875 — HMAC doesn't bind recipient node_id: handshake auth_data was nonce||sender_node_id, allowing a captured handshake to be replayed against any other federation node sharing the same shared_secret. Fix: change auth_data format to nonce|sender_node_id|recipient_node_id throughout (outgoing in connect_to_peer_with_id and send_to_peer; verification in handle_inbound which uses local node_id as the expected recipient). Add connect_to_peer_with_id() as the binding-aware variant; connect_to_peer() delegates to it with "" for bootstrap connections. Closes #3880, #3876, #3875
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
github-actions
Bot
added
area/security
houko
left a comment
houko
left a comment
There was a problem hiding this comment.
The security fixes (HMAC-before-nonce, recipient binding, 64 KB cap) are correct and well-tested. One minor comment inaccuracy in test_nonce_tracker_gc_amortized — see inline note. The test itself is valid.
Generated by Claude Code
| let result = tracker.check_and_record("n-0"); | ||
| assert!(result.is_err()); | ||
| assert!(result.unwrap_err().contains("replay"), "expected replay error"); | ||
| } |
There was a problem hiding this comment.
The comment "Insert 4 nonces — below the 80% threshold (4 entries), no GC runs" is slightly inaccurate.
With max_entries = 5, the GC threshold is 5 * 4 / 5 = 4. During the insert loop (indices 0–3) the map has 0, 1, 2, 3 entries respectively before each insert, so the len >= threshold check is always false and GC indeed does not run during the loop — that part is correct.
However, when check_and_record("n-0") is called for the replay check, the map has 4 entries, so 4 >= 4 is true and the GC sweep does run. Because all entries were inserted moments ago none are expired, so the replay of "n-0" is still correctly detected and the test passes — but the comment says "no GC runs" without qualification.
Suggested fix:
// Insert 4 nonces. During these inserts the map has 0–3 entries, which
// is below the 80% GC threshold of 4 — so no GC sweep occurs.
for i in 0..4 {
assert!(tracker.check_and_record(&format!("n-{i}")).is_ok());
}
// At the replay check the map has 4 entries (== threshold), so GC does
// run — but all entries are fresh, so nothing is removed and the replay
// is still detected.
let result = tracker.check_and_record("n-0");Generated by Claude Code
…6330) Bootstrap dials sign an empty recipient_node_id because `bootstrap_peers` lists addresses, not identities, but #3920 made the receiver verify the handshake HMAC against its own real node_id with no fallback — so every bootstrap connection 403'd in both directions. The inbound handshake now accepts the empty-recipient binding alongside the recipient-bound form, and threads whichever matched into Ed25519 identity verification. Nonce dedup (#3880), Ed25519 TOFU pinning (#3873), and the per-handshake ECDH session key (#4269) still authenticate the peer; a wrong non-empty recipient is still rejected, preserving the #3875 cross-node replay protection. Regression from #3920.
1 participant
Summary
check_and_record(replay nonce dedup) to AFTER HMAC signature verification — previously a forged message with a valid nonce could exhaust the nonce map without auth (closes OFPNonceTrackerdoes O(n)retain()on every handshake — DoS amplifier under flood; also runs before HMAC verify #3880)MAX_PEER_MESSAGE_BYTES = 65_536before parsing to prevent unbounded allocation from malformed peers (closes OFPAgentMessagelets any authenticated peer spend the receiver's LLM budget — no per-peer rate limit, no per-peer cap #3876)auth_datain HMAC to{nonce}|{sender_node_id}|{recipient_node_id}on all paths (connect handshake + inbound handler), so ciphertext captured from one peer-pair cannot be replayed to another (closes OFP handshake HMAC binds (nonce, sender node_id) but not recipient — captured handshake replayable to any other peer #3875)retain()only runs when the map reaches 80% of capacity instead of every messageTest plan