Fenrir fixes 2026 09 28 - #179
Merged
Merged
Conversation
tcp_rto_cb() skipped the data-retransmission path in TCP_CLOSE_WAIT and let the control RTO take over TCP_LAST_ACK even with payload outstanding, so a segment lost after the peer half-closed was never retransmitted and only the FIN was later re-sent. - Include CLOSE_WAIT and LAST_ACK in the data-retransmission gate. - Give LAST_ACK the same payload-drain check as FIN_WAIT_1 so the control RTO only takes over once the data is fully drained. - When the control RTO fires over outstanding data, stop it and hand the timer back to the data path so the data RTO is re-armed from the in-flight descriptors. - When a forward ACK drains the last payload in FIN_WAIT_1/LAST_ACK with the FIN still in flight, re-arm the control RTO so the FIN keeps being re-sent. Gate that re-arm on !tcp_has_pending_unsent_payload: arming the control RTO over window-blocked data leaves ctrl_rto_active set, and the data timer that flush swaps in then fires into a bail in tcp_rto_cb, delaying the first data retransmit by an RTO. - Clear the stale tmr_rto on the data-gate early return so a later resync can re-arm the data RTO. Adds unit tests driving tcp_rto_cb in CLOSE_WAIT and LAST_ACK-with-data plus the LAST_ACK drain check and the control-RTO-over-blocked-data gate; they fail before the fix and pass after.
wolfIP_sock_sendto()'s ICMP arm wrote the per-datagram destination straight into ts->remote_ip, the field that connect() sets as the connected peer and that icmp_try_recv() uses as its source filter. A single sendto(A) narrowed the receive filter to A (dropping router-generated errors and late replies from other hosts), and a sendto() on a connected socket silently replaced the peer. Resolve the destination into a local (defaulting to ts->remote_ip when no address is given), validate and route with that local, and pass it to ip_output_add_header(). ts->remote_ip is now changed only by connect(). Updates the sendto-branch test that asserted the clobber and adds a test covering the unconnected-filter and connected-peer cases; both fail before the fix and pass after.
icmp_try_deliver_tcp_error() called close_socket() unconditionally on a SYN_SENT/SYN_RCVD socket matched by a PROT/PORT-unreachable ICMP error. A passive listener transitions in place to SYN_RCVD (is_listener stays set), so close_socket() wiped the whole tsocket: proto cleared, bound port released, and the listening endpoint destroyed by one crafted ICMP error. Apply the same is_listener guard the RST path and the control-RTO give-up use: in SYN_RCVD with is_listener set, revert to LISTEN via tcp_listener_revert_to_listen() instead of closing, so only the half-open connection attempt is aborted (RFC 1122 4.2.3.9). Adds a test driving a port-unreachable at a SYN_RCVD listener; it fails before the fix (proto wiped) and passes after. The non-listener SYN_SENT close test is unchanged and still passes.
flush_tcp_tx() walks the TX FIFO from the oldest descriptor and breaks at the first data segment that does not fit the send window, so pure ACKs queued behind that data were withheld. With the peer window closed, ACKs for data the peer sends were held until a persist probe, and the accumulated ACKs were then flushed back-to-back as duplicate ACKs, triggering spurious fast retransmit on the peer. When a data segment is window-blocked, jump to the first queued pure ACK behind it and send it (RFC 5681 4.2). Only a true pure ACK (flags == ACK) is jumped to: a FIN-ACK is a control segment and stays behind the data. A pure ACK sent ahead of queued data now carries the first unsent sequence instead of SND.NXT, which counts the queued bytes and can fall outside the peer's receive window (RFC 9293 3.10.7.4); a FIN/SYN/RST keeps its own sequence. After the jump the walk stops at the next non-pure-ACK segment (data, FIN, RST), so a zero-length FIN queued behind the ACK cannot pass the send condition and go out ahead of the blocked data. tcp_first_unsent_seq() skips a retransmit that is still pending: marking a descriptor for retransmission clears PKT_FLAG_SENT, so without the RETRANS guard the helper would report the retransmit's old seq. Adds tcp_find_pending_ack() and tcp_first_unsent_seq(), plus tests for the pure-ACK-behind-data case, the FIN-behind-blocked-data-after-ACK case, and the retransmit-skip; all fail before the fix and pass after.
rawsocket.if_idx doubled as the egress route and the receive filter: connect, sendto and flush_raw_tx wrote the transmit route into the same field that raw_try_recv read as the ingress interface bind. After a single sendto routed through interface N, an unbound raw socket (a ping socket, a monitoring tap) silently stopped receiving on every other interface. A wildcard bind made it worse: wolfIP_if_for_local_ip(ANY) returns the primary interface, so INADDR_ANY received only there, and interface 0 could not be selected at all because 0 meant "any". Store the ingress bind in a new recv_if_idx field with the sentinel WOLFIP_RAWSOCK_ANY_IF (0xFF) for "any", so interface 0 stays selectable. Only bind sets it (wildcard -> any); connect/sendto/flush keep writing if_idx, which is now egress-only. The VLAN-delete guard that refuses to remove an interface a socket is bound to now checks recv_if_idx, matching the TCP/UDP/ICMP arms. Adds test_raw_socket_send_and_wildcard_bind_keep_any_if_recv: send to the second subnet (egress lands in if_idx), then receive on both interfaces, and a wildcard bind that keeps receiving on the non-primary interface. Fails before the fix (second-interface frames filtered), passes after.
wolfIP_route_get, wolfIP_route_add and wolfIP_route_delete each repeated "#if WOLFIP_ENABLE_FORWARDING ... #else (void)...; return -WOLFIP_EINVAL; #endif" inside a block that is already wrapped in the same outer guard. The inner #else stubs can never be compiled: they advertise a non-forwarding fallback that does not exist, because the prototypes in wolfip.h are guarded the same way. Anyone maintaining or testing those stubs is working on dead code. Remove the three redundant inner #if/#else/#endif wrappers, keeping the forwarding bodies. The outer guard already excludes the whole block in non-forwarding builds, so behavior is unchanged.
wolfIP_ip_mtu() only ever subtracts ETH_HEADER_LEN, so its return value is the IP datagram MTU (20-byte header included), not a payload size. Every caller confirms this: wolfIP_socket_tcp_mss subtracts IP_HEADER_LEN+TCP_HEADER_LEN to get the TCP MSS, and the UDP/ICMP sendto paths check len against ip_mtu - IP_HEADER_LEN - <proto> header. The comments at the function and at the 1500-byte cap both said "IP payload MTU", which would lead a maintainer sizing a new protocol's data to overshoot the interface budget by 20 bytes. Reword both comments to "IP MTU (header + payload)" / "the IP MTU remains capped at the standard 1500 bytes", matching how every caller treats the value. Doc-only change.
eap_tls_engine_export_msk() derives a single 64-octet MSK (TLS <= 1.2 via wolfSSL_make_eap_keys, TLS 1.3 via the RFC 9190 exporter) under one MSK-specific label. Per RFC 5216 / RFC 9190 the EMSK is a separate 64-octet value from a distinct, longer/differently-labeled export - it is not the upper half of the MSK. The header comment said "msk[32..63] becomes the EMSK (currently unused)" and the caller in supplicant.c repeated "the remaining 32 bytes form the EMSK". A maintainer adding ERP/re-auth bootstrapping (RFC 5295) and following those comments would split the MSK and hand out bytes that are neither the spec EMSK nor independent of the PMK. Reword both comments: msk[0..31] is the PMK (RFC 5216 §2.3), msk[32..63] is the unused MSK tail, and the EMSK is not derived by this function. Doc-only change.
The general per-packet filter in ip_recv() drops src=0.0.0.0 datagrams, but suppresses that arm while the DHCP client is unbound (for local DHCP/BOOTP traffic). With forwarding enabled, a zero-source datagram arriving on a transit interface then reached the forwarding branch, whose RPF checks (loopback, link-local, own-address) never test for a zero source. With TTL<=1 (or DF+oversize, or a malformed option) the router generated an ICMP error whose destination was set to orig->src = 0.0.0.0, which RFC 1812 §4.3.2.7 explicitly forbids. In the forwarding RPF block, unconditionally drop any transit packet whose source is IPADDR_ANY. Local DHCP delivery is unaffected: it is handled by the is_local path earlier in ip_recv(), not the forwarding branch. Adds test_fwd_zero_source_transit_ttl1_silent_drop: a zero-source transit packet with TTL=1 while DHCP is unbound must be dropped silently (no Time Exceeded addressed to 0.0.0.0). Fails before the fix (ICMP sent), passes after.
tcp_listener_revert_to_listen zeroes sock.tcp (including tmr_rto) without an explicit timer cancel, which looks like it leaves a stale SYN-ACK retransmit timer in the heap. It does not: tcp_preaccept_timeout_stop, called before the state reset, cancels the shared tmr_rto slot the control RTO uses. Add a test that pins the invariant (timer heap empty after reverting a SYN_RCVD listener with an armed control RTO) so a future change that reintroduces a stale timer is caught.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #179
Scan targets checked: wolfip-src, wolfip-bugs
Coverage: 1 of 3 in-scope changed file(s) opened by the reviewer; not opened: src/supplicant/eap_tls_engine.h, src/supplicant/supplicant.c
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
When close() replaced an active data RTO with the control RTO, a control-RTO expiry over outstanding payload (LAST_ACK/CLOSE_WAIT) stopped the control RTO, re-armed the data timer, and returned. The control timeout had already expired, so the lost payload was retransmitted only after a second RTO interval. Stop the control RTO and fall through to the data-loss recovery, which retransmits the outstanding payload now and re-arms the data RTO. The control retransmit path now runs only when the state actually needs the control RTO (else branch), so the yield path no longer re-arms it. Also fix a stale assertion comment in the forwarding zero-source test: the injected source is 0.1.2.3, so any Time Exceeded would be addressed to 0.1.2.3, not 0.0.0.0. The LAST_ACK test now asserts the retransmit on the first expiry.
gasbytes
approved these changes
Sep 29, 2026
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.
71112da F-14337: drop zero-source transit packets in the forwarding RPF block
70ed046 F-14336: stop calling the MSK tail the EMSK in eap_tls_engine docs
b2cf666 F-13775: reword wolfIP_ip_mtu comments to say IP MTU, not payload MTU
1d0d7aa F-14332: drop unreachable inner forwarding guards in the route API
240470a F-14331: split raw-socket ingress filter from the egress route
9cf2e60 F-14330: send pure ACKs queued behind window-blocked data
3657c95 F-14355: revert SYN_RCVD listener to LISTEN on ICMP unreachable
580ac1c F-14329: keep ICMP sendto destination per-datagram
ff73471 F-14328: retransmit TCP data in CLOSE_WAIT and LAST_ACK on RTO