fix: security and correctness fixes from full code review

[srperens]

Summary

A full read-through of src/ turned up a set of security, concurrency and RFC-conformance issues. This PR fixes all of the verified ones in four commits. No functional features are added; behaviour changes are limited to cases where the server previously accepted something RFC 5766 says it must reject (called out below).

Security

• Attributes after MESSAGE-INTEGRITY are now ignored (demux/stun.rs). The parser used to keep collecting XOR-PEER-ADDRESS, LIFETIME, USERNAME etc. from the unprotected tail of the message, so anyone who could capture one authenticated request and spoof the client's source address could append attributes and have them honoured while the HMAC still verified. Parsing now stops at MESSAGE-INTEGRITY per RFC 5389 §15.4.
• Allocate authenticates before anything else (turn/handler.rs). The retransmission shortcut answered an unauthenticated Allocate with a signed success response whenever an allocation already existed for the source address.
• Own external IP on non-relay ports is refused on every path. Send indications already blocked external_ip:<other port>; ChannelBind did not, so a client could reach other UDP services on the relay host through ChannelData. A shared is_forbidden_peer_addr now guards Send, ChannelBind and the ChannelData egress.
• IPv4-mapped IPv6 and link-local (::ffff:127.0.0.1, ::ffff:169.254.169.254, fe80::/10) are judged correctly. Relevant when external_ip is IPv6, since the server then binds a dual-stack [::].
• Nonces stamped in the future are rejected. saturating_sub returned 0 for them and treated them as fresh indefinitely.

Concurrency

• Lock-order inversion between lookups and cleanup (lookup/table.rs). get_by_client and create_or_get held a by_client guard while locking allocations; the cleanup paths hold allocations (via retain) and then remove from by_client. That is an ABBA inversion against the documented lock order and can deadlock regardless of the lock's fairness policy. Both now release one guard before taking the other. A watchdog regression test in the style of the existing one covers it.
• No allocation guard is held across socket I/O in the relay paths (relay/engine.rs, server.rs). handler.rs already followed this rule; the relay engine held DashMap Refs across every send_to().await, which stalls cleanup and deadlocks a single-worker runtime. Sends now go through a small Delivery snapshot captured under short-lived guards. The identical RTP / non-RTP ChannelData branches were merged in the process.

RFC 5766 conformance (behaviour changes)

• 437 Allocation Mismatch for a new Allocate (different transaction id) over a live allocation. A true retransmission (same id) still gets the success response. Matches coturn; libwebrtc never re-Allocates on a socket after success, so this only affects stale state after a client restart.
• 400 Bad Request when ChannelBind would rebind a channel to a different peer or a peer to a different channel (§11.2). Previously accepted, leaving a stale reverse mapping that framed traffic from the old peer with a channel number the client now associated with the new peer. bind_channel also cleans stale entries as defense-in-depth.
• ChannelBind's implied permission now goes through the capped path instead of bypassing max_permissions_per_alloc.

Other fixes

• Refresh with lifetime 0 only releases the rate-limiter quota slot if it actually removed the allocation; a concurrent cleanup could otherwise cause a double decrement and let an IP exceed its quota. AllocationTable::remove now returns bool.
• Send indications targeting another TURN client of this server are wrapped in ChannelData / Data Indication, as the ChannelData path already did. A raw datagram from the relay address lands on the peer's TURN control socket and is discarded.
• Receive buffer 2048 → 65535 bytes; recv_from silently truncated larger datagrams.
• Long-term credential verification is factored into one verify_long_term_credentials shared by Allocate and the post-Allocate requests, removing a ~100-line duplicate.

Follow-up review rounds

Two further review passes over this branch turned up five issues in the code it adds. Fixed in f49d6ca and 74a067b.

Error responses were unsigned. build_error_response never appended MESSAGE-INTEGRITY, and RFC 5389 §10.2.3 has a client using long-term credentials discard an error response without it (400, 401 and 438 excepted). The new 437s and 508s were therefore invisible: the client retransmitted through its whole RTO schedule (~39.5s) and reported a timeout instead of the error. Allocations here live at most 60s, so a client that loses a single Refresh hit exactly that. build_signed_error_response now signs every error raised after authentication, and authenticate_for_allocation moves authentication ahead of the allocation lookup so the 437 for a missing allocation has a key to sign with. AuthResult / validate_request_auth are replaced by it. The forbidden-peer 403 moves after authentication for the same reason; an unauthenticated source with no allocation now gets 401 rather than 403.

ChannelBind mutated state before validating. The implied permission was installed before the conflict and channel-cap checks ran, so a client could fill its permission table with peer IPs through ChannelBinds the server went on to reject with 400. §11.2 requires an error response to leave the allocation unchanged. The guarded block now only decides the outcome; the permission insert and the bind follow once it is admissible.

Allocate over a lapsed allocation. cleanup_expired runs every 2s, so a client that lets its allocation expire and reconnects usually finds the dead entry still in the table. It got the new 437 and was locked out of the port until the reaper caught up (before this PR: a success response with lifetime = 0, just as dead). handle_allocate now reaps the expired allocation, releases its quota slot as cleanup_expired would have, and creates a fresh one.

The orphan-sender timer is no longer armed from Send-indication routing. Feeding it there looked uniform but reaped real allocations: that path carries ICE connectivity checks, sent before any peer exists, so the first client into a call (ringing, waiting room, slow signalling) lost its allocation 45s later while actively sending and refreshing, and its next Refresh got 437. A successful internal route still records relay success. The ChannelData and raw media paths keep arming it on a failed relay - STUN never reaches them, so by then the client is past ICE. Trade-off: an allocation that never relays successfully is no longer reapable by cleanup_orphaned_senders, bounded only by the per-IP quota. Narrowing that check is left for a separate change.

Verification

• cargo fmt --check, cargo clippy --all-targets: clean
• cargo test: 54 passed (18 new: MI-tail regression, two lock-order watchdogs, remove semantics, bind_channel consistency, IPv4-mapped/link-local/own-IP forbidden checks, future nonce, signed vs unsigned error responses, ChannelBind conflict leaves no permission behind, same-pair rebind is idempotent, targetless Send indication leaves the orphan timer unarmed, signed 437 on Refresh without an allocation, signed 403 on both forbidden-peer paths, 401 on a credential/allocation owner mismatch, Allocate over an expired allocation starts fresh, 437 for a new transaction id over a live one)
• Browser flow: test-webrtc/webrtc-test.html in two tabs against the release binary, iceTransportPolicy: 'relay'. Two complete calls. Allocate, CreatePermission, ChannelBind 0x4000 both ways, ICE Binding requests relayed as Data Indications, then continuous bidirectional ChannelData; the first call torn down gracefully with Refresh lifetime=0 on both sides, exercising the remove() quota path. Zero WARN and zero ERROR across 1692 log lines - no 437, no 403, no ChannelBind conflict, no cap exceeded, no stale nonce. The only 401s are the expected first-contact challenges. Both RFC-conformance behaviour changes behave as predicted: libwebrtc never re-Allocates on a socket and never rebinds a channel, so neither the 437 nor the 400 path fires in a normal call.

Not in this PR (design questions)

Noted during review, left for a separate discussion: ICE ufrag registration and relay-IP auto-grant are driven by unauthenticated Binding requests (first-come-first-served); a single inline receive loop means one blocked send stalls all processing; Binding requests from TURN clients are never answered, so a client using the same host:port as both stun: and turn: gets no srflx candidate from it.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Jn5PhyfAEWPGDjLpu5rqeB