[Security review][L1] Bound the receive wait and resend phase on a wall-clock deadline - #37
Merged
Merged
Conversation
7 tasks
Both the receiver's main wait and the sender's resend-serving phase decided their timeout from how long a single recvfrom sat idle (SO_RCVTIMEO = 1s), not from real elapsed time. Any host sending faster than once a second — even one-byte junk — kept recvfrom returning data, so the idle countdown never fired and the loop ran forever. The session id is cleartext in every broadcast, so a valid-looking RESEND flood was just as easy to forge. Receiver: the main loop now waits on a steady_clock deadline that only a recognised packet from our sender (ANNOUNCE/TRANSFER/FINISH for our session) pushes forward. Junk no longer freezes the countdown, so the receiver still gives up ttl_max seconds after the last real packet. The recovery loop keeps its own per-round ttl countdown and now resets it explicitly instead of inheriting the main loop's counter. Sender: serveResends gains an absolute phase deadline (ttl_max * 10, steady clock), fixed at phase start and never extended by incoming traffic, so a continuous RESEND flood can no longer keep it re-reading the file and re-broadcasting parts without end. ttl is now refreshed only when a part is actually re-sent, and the per-part rate-limit map is probed with find() instead of operator[] so merely being asked about a part no longer inserts a node. Adds tests/e2e_flood.sh (+ tests/flood.py): a junk flood must still let the receiver time out, and a valid RESEND flood must still let the sender stop at its resend deadline after actually serving resends. Both cases hang on the pre-fix binary.
gistrec
force-pushed
the
fix/absolute-timeout-bounds
branch
from
July 6, 2026 09:41
b71638f to
e3da454
Compare
The wall-clock deadline fixed the main receive wait and the sender's resend phase, but the receiver's RESEND recovery loop (checkParts) was left on the old mechanism and had the identical busy-socket hole. Its inner drain loop broke only on an idle recvfrom (SO_RCVTIMEO), and the per-round ttl countdown lived outside that loop. So once the receiver took FINISH with parts still missing, a junk flood on the receive port kept recvfrom returning data forever: the drain loop never broke, ttl never counted down, and g_interrupted -- checked only at the top of the outer loop -- was never reached, so even Ctrl+C could not free it. This is the same flood-keeps-filecast-alive-forever bug the deadline was introduced to kill, just after FINISH instead of before ANNOUNCE. Recovery now waits on the same steady_clock deadline as the main loop, refreshed only by a genuinely recovered part, and checks both the interrupt and the deadline before every read inside the drain loop. A junk flood no longer lands a wanted TRANSFER, so it never pushes the deadline and recovery still gives up ttl_max seconds after the last real part (exit 2); a real burst of replies keeps refreshing it and is never cut short. The per-round ttl counter is gone. Adds a third e2e_flood.sh scenario (+ tests/dropproxy.py): a drop-proxy strands ~1/3 of the parts to force recovery, then a junk flood on the receive port must still let the recovery loop time out (exit 2, reaching the "part(s) missing" path) instead of wedging. Hangs on the pre-fix binary; passes on this one.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Both timeout loops decided when to give up from how long a single
recvfromsat idle (
SO_RCVTIMEO= 1s), not from real elapsed time. Any host sendingfaster than once a second kept
recvfromreturning data, so the idle countdownnever advanced and the loop ran forever. The session id is cleartext in every
broadcast, so a "valid" RESEND flood is as easy to forge as raw junk.
Receiver::run(src/Receiver.cpp):ttldecremented only onrecvfrom < 0. A flood of one-byte junk (parseHeader → NotOurs, nottlchange) kept
while (ttl > 0)alive indefinitely — directly contradicting thein-code comment that the timeout "stays reachable". Impact is low (a foreground
CLI just keeps waiting), but the loop was genuinely unbounded.
Sender::serveResends(src/Sender.cpp):ttl = ttl_maxon anyvalid RESEND for our session, before the rate-limit. The only exit was
ttl_maxconsecutive idle seconds, which a ≥1 RESEND/s flood prevents forever —the sender keeps re-reading the file from disk and (in broadcast mode)
re-broadcasting parts to the whole segment.
sent_part[part]also usedoperator[], inserting a node for every requested index.Fix
Decide both timeouts on a
steady_clockdeadline that only real progress pushesforward.
receive_deadline, refreshed only by arecognised packet from our sender (ANNOUNCE/TRANSFER/FINISH for our session).
Junk never pushes it, so the receiver still gives up
ttl_maxseconds afterthe last real packet. Per-packet transfer semantics (the
ttl_maxbudgetbetween packets) are unchanged. The recovery loop (
checkParts) keeps its ownper-round countdown and now resets it explicitly rather than inheriting the
main loop's counter.
serveResendsgains an absolute phase deadline (ttl_max * 10,steady clock) fixed at phase start and never extended by incoming traffic, so a
RESEND flood can no longer keep the phase running without end.
ttlis nowrefreshed only when a part is actually re-sent, and the rate-limit map is
probed with
find()instead ofoperator[]so merely being asked about a partno longer inserts a node. The map stays bounded by the sender's own
total_parts(its own file), which the attacker cannot inflate, so no hard mapcap is needed on top of the deadline.
Tests
tests/e2e_flood.sh(+tests/flood.py, registered in CMake, Python-gated likethe other proxy tests):
--ttl 2) muststill time out (exit 2) instead of hanging.
with valid RESENDs — the sender must still stop at its resend deadline
(exit 0) after actually serving resends.
Both scenarios hang on the pre-fix binary (verified against
master) andpass on this branch. Full suite (unit + all e2e) passes locally, including under
ASan/UBSan.
Notes
anymore.
RESEND_PHASE_TTL_MULTIPLE = 10(150s at the default--ttl 15) is agenerous backstop for real LAN recovery while keeping a single run finite; it
scales with the operator's
--ttl.master.