Fenrir fixes 2026 08 17 - #153
Open
danielinux wants to merge 21 commits into
Open
Conversation
wolfIP_sock_sendto() queued UDP/ICMP frames carried only the transport header and payload in the txbuf descriptor; the IP header (dst/src, id, ttl, checksums) was filled from the socket-wide t->remote_ip / t->local_ip at flush time in flush_datagram_tx(). A descriptor left in the queue after a failed flush (LL -EAGAIN, pending ARP, filter break) was therefore re-targeted to whatever destination the next sendto() on the same socket used, so the first datagram went out with the second one's destination IP, route and next-hop MAC. Fill the IP header and transport checksums at enqueue time, while the socket's routing state still matches the datagram being queued. The flush now only adds the link-layer header and derives the egress interface and next hop from the destination stored in the frame. The Ethernet header move is factored out of ip_output_add_header() and re-added at the three TCP send sites, which are unaffected by the bug (one connection, one destination).
wolfIP_sock_accept() sent the SYN-ACK from the cloned socket with seq = ISN, then incremented the socket's seq. The control RTO retransmit rebuilds the SYN-ACK from that field, so the retransmit carried ISN+1. A peer processing it would ack ISN+2, which the SYN_RCVD final-ack check (expected ack = snd_una+1 = ISN+1) rejects with a RST, turning a lost SYN-ACK or final ACK into a dropped connection instead of a recovered handshake. Drop the increment: while in SYN_RCVD no data is sent, seq stays at the ISN, and the final-ack handler advances it to ISN+1 on establishment. The passive-open and active-open paths never had the increment. Three existing tests built the client's final ACK from the accepted socket's seq field, which only equalled snd.nxt (ISN+1) because of the increment; they now use tcp_seq_inc(snd_una, 1) explicitly, which is the same wire value.
An active TCP OPEN to a broadcast address gets SYN-ACK candidates from every host on the segment, and a multicast group has no single peer, so neither can complete a connection. wolfIP_sock_connect() mutated the socket to TCP_SYN_SENT, assigned an ISN and queued the SYN for such destinations anyway, while the inbound SYN path already drops SYNs from broadcast and multicast sources. Validate the destination with the existing predicates before mutating the socket and return -WOLFIP_EINVAL, leaving the socket reusable.
An explicitly advertised, nonzero MSS is the peer's commitment about what it will receive; the effective send MSS must not exceed it (RFC 9293 section 3.7.1). tcp_parse_options() raised any value below the 536 IPv4 default up to 536, which (a) violated the constraint and (b) blackholed legitimate low-MTU peers: this stack sends TCP with DF set, and the ICMP PTM path ignores next-hop MTUs below 576, so segments sized at the floored 536 are dropped forever on a path that genuinely carries less. The 536 default still applies when no MSS option is present. The ICMP PTM path keeps its separate rejection of sub-576 MTUs: that input is unauthenticated (spoofable), unlike the MSS option of a peer completing the handshake. One existing test asserted the clamping as intended behavior; it now asserts the advertised value is recorded as-is.
RFC 7323 section 5.3 requires PAWS verification to take precedence over the regular TCP acceptability test on timestamp-synchronized connections. In LAST_ACK and in the ESTABLISHED/CLOSE_WAIT/FIN_WAIT_1/ FIN_WAIT_2/CLOSING branch, tcp_input() ran tcp_segment_acceptable() first; a segment failing the sequence test got its challenge ACK before PAWS was ever consulted, so a replayed TSopt-less segment drew an ACK reply instead of the RFC-mandated silent drop. Move the existing PAWS check ahead of the acceptability test in both branches. TIME_WAIT already ran PAWS first; the RST exemption is inside tcp_paws_check and is unchanged.
RFC 5681 defines a duplicate ACK with five conditions, including that the segment carries no data and that its advertised receive window equals the previously received ACK's window. tcp_ack() counted any ACK-flagged segment that repeated snd_una while data was in flight, so a peer sending three or more data segments without acknowledging new data from us (normal bidirectional transfer) miscounted as three duplicate ACKs and entered fast recovery: ssthresh halved, cwnd inflated, and a retransmit issued with no loss having occurred. Add the two missing conditions to the dup-ACK branch: skip segments whose IP length exceeds the header length, and skip segments whose advertised window differs from the previously processed ACK's. Track the previous raw window in a new last_peer_win field updated for every ACK-flagged segment entering tcp_ack. One existing test primed its window reference explicitly; its three phase-1 ACKs now repeat rather than establish the window.
RFC 6298 section 5.5 requires the new RTO after a retransmission timeout to be min(2*RTO, G) where G is the maximum timer value, 64 seconds. The re-arm paths computed rto << backoff with no upper bound: tcp_rto_cb() (timeout re-arm and the bookkeeping-resync re-arm), the TCP flush re-arm, and tcp_ctrl_rto_start() for control segments. With a 2 s base RTO and a handful of timeouts the effective interval ran into the minutes, delaying loss recovery and control-segment retries far beyond the RFC limit. Add tcp_backoff_rto_ms() which doubles the base RTO per retry and caps the result at TCP_RTO_BACKOFF_MAX_MS (64 s), and route all four re-arm sites through it. The retry counters feeding the shift were already bounded (TCP_RTO_MAX_BACKOFF, TCP_CTRL_RTO_MAXRTX), so the shift itself stays defined; only the missing cap is added.
…loor RFC 9293 section 3.5 gives the R2 retransmission timeout for SYN and FIN segments a 3-minute default. The control-RTO budget of 6 retries over the 1 s base RTO accumulated 1+2+4+8+16+32+64 = 127 s, so an unanswered active-open SYN was abandoned well short of the required 180 s (and FIN retransmits with it). Raise TCP_CTRL_RTO_MAXRTX from 6 to 8. With the 64 s backoff cap in place the arms are 1,2,4,8,16,32,64,64,64 s, giving up at 255 s: at or above the R2 floor with margin, and bounded. The budget is shared by SYN, SYN-ACK and FIN retransmission, all of which the R2 guidance covers. A test drives the retransmission timer to exhaustion and asserts the give-up is not before 180 s.
wolfIP_forward_interface() only matched directly connected subnets, so a transit packet whose destination was reachable only through a configured static route (wolfIP_route_add) found no egress and was dropped in local-delivery processing, while locally originated traffic honored the same routes. When no connected egress exists, resolve the destination in the static route table with the same tie-breaks as the local lookup (longest prefix, then order, then interface) and forward through the route's interface. ARP resolution and the pending-ARP queue now use the route's gateway as next hop instead of the final destination, and a route pointing back at the ingress interface is not honored. Connected subnet forwarding, the no-route drop behavior, and TTL handling are unchanged.
ip_recv validated the minimum frame length, IHL, the total-length floor, and the header checksum, but never compared the declared IPv4 total length against the bytes actually received. A frame whose header claimed more payload than it carried (with a valid header checksum) was relayed downstream with an internally inconsistent length, and a frame carrying trailing link-layer padding was forwarded at the padded frame length, relaying padding as IP payload. Reject any datagram whose declared total length exceeds the received bytes before filtering or forwarding, and forward at ETH_HEADER_LEN + declared length so padding is stripped. The TTL-exceeded guard keeps using the received length: the Time Exceeded reply copies bytes from the frame as received, and datagrams with a declared length shorter than the frame are legitimate. Also resolves F-8518 (same root cause, wolfip-bugs target).
…tion override With ipheader_include set, flush_raw_tx skips checksum generation on the assumption the caller supplied a complete, correct header. But wolfIP_sock_sendto overrides that header's destination with the sendto() socket address when one is given, and destination is covered by the IPv4 header checksum. The packet then reached the wire addressed to the socket destination carrying the caller's checksum for the header destination, failing verification in the middle of the net. Recompute the header checksum after the override. The override itself (socket address wins over the header) is unchanged.
dns_send_query() assigned s->dns_id before validating the encoded name. The two invalid-label exits (label over MAX_DNS_LABEL_LEN, or name over MAX_DNS_NAME_LEN) returned -22 without cleanup, unlike every other failure path in the function. No timer was scheduled on those exits, so dns_timeout_cb() never cleared the state: s->dns_id stayed nonzero and both nslookup() and dns_send_query() gate on it, making every subsequent lookup on the stack fail with -16 until reinitialization. Call dns_abort_query() and reset *id on both exits, matching the connect/sendto failure paths. The existing invalid-name test now asserts the resolver is left un-armed and that a subsequent lookup succeeds, dropping the manual s.dns_id reset the defect had forced on it.
The presentation-name encoder split the name on '.' without rejecting
empty labels. A leading or interior dot encoded a zero-length label —
the wire-format root terminator per RFC 1035 section 3.1 — and then
kept appending the remaining labels after it, producing a malformed
QNAME with labels beyond the terminator.
Reject a zero-length label wherever the tokenizer encounters one. The
trailing-dot FQDN presentation form ("example.com.") ends the loop
before an empty token is encoded and remains valid.
The A-record answer handler accepted any RDLENGTH >= 4 and read the first 4 bytes as the address. RFC 1035 section 3.4.1 defines A RDATA as exactly a 32-bit address, so an RR with a different RDLENGTH is malformed and must not be delivered as the answer. Require rdlen == DNS_IPV4_RDATA_LEN in the A branch. A nonconforming RR is now skipped like any other non-matching record and the query stays outstanding for the retry/timeout path. The PTR branch is untouched: PTR RDATA is a name and legitimately varies in length.
The DHCP client bounded its option scans to the standard options field and never recognized option 52 (Overload). A server that legitimately overloads parameters into the reply's sname (value bit 2) and/or file (value bit 1) fields had them silently ignored; in particular dhcp_parse_offer() rejected an OFFER whose server identifier lived in an overloaded field, dead-ending the DHCP transaction. Add a small option-stream iterator that walks the reply's options in RFC order — standard options field, then sname, then file — with independent bounds per region, continuing into an overloaded field only when the options field is exhausted without a terminating option and option 52 selected it. Option 52 values outside 1-3 are malformed. dhcp_parse_offer(), dhcp_msg_type() (non-strict) and dhcp_parse_ack() (strict) now all run their existing per-option logic over the full stream through the iterator.
RFC 2131 section 4.1's 10 Mb/s Ethernet example puts the first DHCPDISCOVER retransmission at 4 seconds, randomized uniformly by plus or minus 1 second. The client scheduled it at 2 s plus 0-199 ms of jitter, re-probing twice as fast as the guidance intends. Move the discover base to 4000 ms and jitter it uniformly over -1000/+1000 ms around the (per-attempt doubled) backoff. The jitter now lives at the call sites: the discover path uses the RFC 4 s ± 1 s window, and the request/renew/rebind retry scheduler keeps its existing 0-199 ms jitter exactly as before.
wolfIP_filter_set_callback() only stored the callback; with the reason masks at their zero default, wolfIP_filter_dispatch() returned allow for every packet and socket event, so a filter installed without a separate mask call was silently never invoked. Track whether a reason mask has been explicitly configured since the callback was registered. While every mask is still at its zero default, dispatch uses the full reason set, so a freshly installed filter is consulted for everything; the first explicit mask configuration (including an explicit zero) switches to the configured reasons only. Document the two-step model and the fail-open default in wolfip-filter.h.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR delivers a batch of correctness and RFC-alignment fixes across the wolfIP stack, primarily tightening protocol validation and ensuring retransmission/forwarding/filtering behaviors match intended semantics.
Changes:
- Updates TCP behaviors (RTO backoff cap, control-segment retry budget, PAWS ordering, dup-ACK qualification, MSS handling, connect() validation).
- Improves DHCP/DNS robustness (DHCP option overload parsing, DHCPDISCOVER first retry timing/jitter, stricter DNS A RDLENGTH checks, DNS name-encoding rollback and empty-label rejection).
- Adjusts forwarding and transmit semantics (forwarding length clamping, static-route forwarding via gateway, pinning queued datagram IP headers and relocating L2 header construction).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfip-filter.h | Documents new filter-callback/mask interaction semantics. |
| src/wolfip.c | Implements core stack fixes across filtering, TCP, DHCP, DNS, forwarding, and TX header construction. |
| src/test/unit/unit.c | Registers new unit tests covering added/changed behaviors. |
| src/test/unit/unit_tests_tcp_state.c | Adds/updates TCP state-machine tests (e.g., SYN-ACK retransmit ISN behavior). |
| src/test/unit/unit_tests_tcp_flow.c | Adds TCP flow tests (dup-ACK rules, RTO cap, MSS retention, connect() rejection). |
| src/test/unit/unit_tests_socket_api_arms.c | Adds raw IP_HDRINCL checksum-override regression test. |
| src/test/unit/unit_tests_proto.c | Extends protocol regression tests (PAWS precedence; last_peer_win setup). |
| src/test/unit/unit_tests_ip_arp_recv.c | Adds forwarding tests for static routes and declared-length enforcement. |
| src/test/unit/unit_tests_dns_dhcp.c | Adds DHCPDISCOVER retry timing test and strengthens DNS invalid-name assertions. |
| src/test/unit/unit_tests_branches.c | Adds regression test ensuring queued UDP datagrams retain enqueue-time destination. |
| src/test/unit/unit_tests_api.c | Adds filter “fresh callback consulted before masks configured” test and updates accept seq expectations. |
| src/test/unit/unit_shared.c | Extends mock link layer to simulate EAGAIN and capture multiple transmitted frames. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
filling the IP header at enqueue time moved the egress interface derivation fully to flush_datagram_tx(), where the route lookup for a multicast group (no matching route) falls back to the first non-loopback interface and silently overrode the per-socket IP_MULTICAST_IF pin. The frame then egressed, and carried its source MAC, from the wrong interface (regression in test_multicast_if_pins_egress_interface). When a UDP multicast descriptor belongs to a socket with a pinned multicast interface, use that interface for the egress and take the group address as the next hop (the MAC is derived from the group address); otherwise route as before.
…callback wolfIP_filter_set_callback() recomputed the all-reasons default flag from the current mask values, so an explicit configuration to zero made before installing the callback (all values zero) indistinguishable from no configuration at all: the flag reset and the callback was consulted for every reason, ignoring the explicit masks. Track explicit configuration as an event instead of inferring it from values: any mask setter (including an explicit zero) sets the flag, and only uninstalling the callback clears it. Install no longer recomputes anything. Update the header docs to state the event-based semantics.
the first-retry schedule subtracted the 1 s jitter half-window from the unsigned backoff base before adding last_tick. With a configured base smaller than the jitter (e.g. DHCP_DISCOVER_TIMEOUT overridden to 200) and a small tick count, the sum wraps modulo 2^64 and the retry is scheduled far in the future (585 million years for base 200, tick 0). Factor the computation into dhcp_discover_retry_delay(): the centered ±1 s window is anchored at zero when the base is smaller than the jitter half-window, keeping the delay bounded for any configured base.
- materialize DHCP options split across overload region boundaries (RFC 2132 section 9.3 stream continuation) - cover static-route forwarding hairpin drop and 0.0.0.0/0 default route - disarm mock_send_eagain_armed in mock_link_capture_reset
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.
11ff613 F-6941: consult a freshly registered filter callback for all reasons
c8bcb31 F-8568: schedule the first DHCPDISCOVER retry at 4 s ± 1 s
664cb34 F-8567: honor the DHCP option overload option (RFC 2132 §9.3)
792c766 F-8570: require an exact 4-byte RDATA for DNS A records
e2322a1 F-8569: reject zero-length DNS labels in the query encoder
f97f172 F-8515: roll back the armed DNS query when name encoding fails
632eeaa F-8517: recompute the IP header checksum after the IP_HDRINCL destination override
69f0bae F-8558: drop and clamp forwarding on the declared IP total length
8714895 F-8516: forward transit packets using the static route table
2ae0fa2 F-8564: extend control-segment retry budget to meet the 3-minute R2 floor
07fcef0 F-6938: cap the backed-off TCP RTO at 64 seconds
dc521b4 F-6472: count only pure ACKs with unchanged window as duplicate ACKs
72cd518 F-8565: run PAWS before the segment acceptability test
a528fdf F-8563: keep peer-advertised TCP MSS verbatim
64879f1 F-8557: reject TCP connect to broadcast and multicast destinations
ffa9c1c F-8514: keep accept socket seq at the ISN while in SYN_RCVD
bf0c0c9 F-8513: pin datagram IP header to enqueue-time destination