bisect-b: last 7 fenrir fixes (temporary, do not merge) - #161
Closed
danielinux wants to merge 7 commits into
Closed
Conversation
…ener In tcp_input the per-socket match gated the 4-tuple comparison on state > TCP_LISTEN, so a TCP_LISTEN socket was matched on local port alone. The SYN handler separately validates bound_local_ip, but a non-SYN segment (data/ACK/FIN) for the same port and a different local address on the same host fell through to the shared bookkeeping writes - t->if_idx, t->last_pkt_ttl, matched = 1, and (for non-RST) t->sock. tcp.peer_rwnd - before any destination-address check. That lets a peer that only needs to know a specifically-bound listener's port corrupt the listener's MTU/TTL bookkeeping, seed the initial window of a later accepted child, and set matched so the RFC 793 unmatched-segment RST is suppressed. Add an else branch so a listener in TCP_LISTEN is skipped when its bound_local_ip is a specific address that does not equal the segment's destination. bound_local_ip (not local_ip) is the discriminator: a 0.0.0.0 bind leaves local_ip set to the interface/primary address as a default source, so local_ip cannot tell a wildcard listener from a specifically-bound one. Wildcard listeners are unaffected (no-op), and the established-socket path (state > TCP_LISTEN) is unchanged. Note on the finding's RST-blackhole claim: a TCP_CLOSED socket with a non-zero src_port never reaches the port-match gate - close_socket() and every RX-path teardown zero proto (first guard) or set CB_EVENT_CLOSED (second guard) and always zero src_port, so a matchable TCP_CLOSED slot only matches invalid port-0 segments. That part of the report does not manifest; the local-address validation gap for listeners is real and is what this commit closes. Adds test_tcp_listen_requires_matching_local_ip: a non-SYN segment for a bound listener's port but a different local address must not change last_pkt_ttl, while one for the bound address still matches. Verified RED (last_pkt_ttl became 64 pre-fix) and GREEN. Plain, IP_MULTICAST and VLAN unit builds pass.
The per-socket match loop recomputed expected_len = ee16(udp->len) + IP_HEADER_LEN + ETH_HEADER_LEN and bailed with if ((int)frame_len < (int)expected_len) return;. That branch is unreachable: the unconditional guard before the socket loop already rejects any datagram where ee16(udp->len) > frame_len - ETH_HEADER_LEN - IP_HEADER_LEN, i.e. it guarantees frame_len >= expected_len. frame_len is pass-by-value and udp->len is not modified between the two, so the inner comparison is always false for every caller (the dispatch path presents an option-stripped IHL=5 header; the loopback multicast caller builds frames where the two quantities are exactly equal). Remove the dead comparison and its now-unused expected_len local. No behavioral change; the plain, IP_MULTICAST and VLAN unit suites pass unchanged.
The header described ssh_server_get_uptime as returning the SSH server uptime in seconds, but the implementation returns the constant 0 until a main-loop tick source is integrated, so the "uptime" SSH command always reports zero. Correct the API-contract comment to state that it is a placeholder returning 0. No behavioral change.
The WOLFTFTP_REQ_BUF_MAX comment totaled the worst-case RRQ/WRQ as 63 + MAX_FILENAME, assigning 12 bytes to the timeout option. But timeout_s is an unclamped uint16_t serialized by wolftftp_append_opt, which includes both key and value terminators: 65535 becomes "timeout\0" (8) + "65535\0" (6) = 14 bytes. The correct total is 65 + MAX_FILENAME. The WOLFTFTP_REQ_BUF_MAX allocation (MAX_FILENAME + 128) already covers this with margin to spare, so no behavioral change - the fix stops a maintainer from using the documented calculation to reduce the margin below a valid maximal request.
dns_callback only parsed an incoming datagram as a DNS response when (flags & DNS_FLAGS_RESPONSE_RD) == DNS_FLAGS_RESPONSE_RD, i.e. when both the QR (query/response) and RD (recursion-desired) bits were set (0x8100). Per RFC 1035 s4.1.1 the QR bit alone distinguishes a response from a query; RD is merely the Recursion-Desired flag a server echoes from the query. A conformant server that does not echo RD (a response otherwise lacking the RD bit) therefore had its reply silently ignored, letting the outstanding query time out and retransmit needlessly. Add a DNS_FLAGS_RESPONSE macro for the QR bit and gate response parsing on it. DNS_FLAGS_RESPONSE_RD is retained: it is still a valid QR|RD response flags value used by the test helpers to build conformant replies. Adds test_dns_callback_qr_without_rd_is_accepted: a response with QR set and RD clear must be parsed and the lookup delivered. Verified RED (lookup not delivered pre-fix) and GREEN. Plain, IP_MULTICAST and VLAN unit builds pass.
After dispatching a TCP socket callback, handle_socket_callbacks() re-read ts->sock.tcp.state and, if TCP_CLOSED, disarmed the callback and called close_socket(), which memsets the whole slot. ts is a fixed slot address, so that check is purely positional. A close callback that closes the socket via wolfIP_sock_close() (freeing the slot) and then allocates a fresh one - tcp_new_socket() scans from index 0 for the first proto == 0 slot, so it can land in the very same slot - leaves the slot holding a brand-new socket whose state is TCP_CLOSED by design. The positional reap then destroyed that fresh socket (zeroing S, proto and the FIFO backing), leaving the application with a permanently dead descriptor. Capture the callback/callback_arg pair before invoking the callback and only reap if both are unchanged afterward: a replaced slot carries a different (or no) callback, so the reap no longer touches it. The normal deferred-close path is unaffected - there the callback does not replace the socket, so the pair is unchanged and the reap proceeds as before. A socket the callback closed without re-creating is already memset (callback NULL), so it is likewise left alone (close_socket would be a no-op). Adds test_handle_socket_callbacks_keeps_recreated_socket: a socket left in the RX-deferred close state (TCP_CLOSED + CB_EVENT_CLOSED) whose callback closes it and re-creates a socket in the same slot must survive the dispatcher's post-callback reap. Verified RED (slot proto zeroed pre-fix) and GREEN. Plain, IP_MULTICAST and VLAN unit builds pass.
In the WOLFIP_RAWSOCKETS branch of wolfIP_sock_sendto(), total_len = ETH_HEADER_LEN + (uint32_t)len narrows the public size_t len to uint32_t before the total_len > LINK_MTU guard, but both payload copies - the ipheader_include memcpy and the non-ipheader memcpy(rip->data, buf, len) - use the original, un-narrowed len. On a 64-bit build a len of UINT32_MAX + 100 narrows to 100 for the MTU check (which passes) while the memcpy still attempts to copy the full, enormous len into the fixed-size frame buffer, overflowing it. Reject len > LINK_MTU as size_t before any narrowing conversion: a payload larger than the frame capacity cannot be sent anyway, and the bound is far below UINT32_MAX so the subsequent (uint32_t)len is exact and the total_len arithmetic cannot wrap. No behavioral change for any valid payload. The unit build enables WOLFIP_RAWSOCKETS unconditionally (unit_shared.c), so the test runs in all unit builds. test_raw_sendto_rejects_oversized_len _before_narrowing pins the bound: the oversized length is refused with -WOLFIP_EINVAL. Verified RED - with the bound check removed the same call segfaults (signal 11) in the payload memcpy - and GREEN. Plain, IP_MULTICAST and VLAN unit builds pass.
Contributor
There was a problem hiding this comment.
Pull request overview
Temporary bisect branch pulling in several recent wolfIP fixes related to packet validation, socket matching, callback dispatch safety, and DNS response parsing, plus corresponding unit-test coverage to reproduce/regress the reported behaviors (e.g., hangs/incorrect state during macOS bisect of test-wolfssl).
Changes:
- Tighten TCP LISTEN socket matching to respect
bound_local_ipfor non-SYN segments and avoid corrupting listener bookkeeping / suppressing RFC 793 RST behavior. - Harden RAW
sendto()length validation to prevent size narrowing/wrap from bypassing MTU checks and risking frame-buffer overflows. - Fix DNS response detection to key on QR (response) bit rather than requiring RD echo; add regression tests.
- Make TCP deferred-close callback dispatch reaping safer when callbacks close and recreate sockets in the same slot; add regression test coverage.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/wolfip.c | Core stack fixes: UDP/TCP receive validation/matching, RAW send length hardening, DNS response parsing, and safer deferred-close callback dispatch/reap logic. |
| src/tftp/wolftftp.h | Update RRQ/WRQ sizing documentation for timeout option worst case. |
| src/test/unit/unit.c | Register new unit tests covering the added regressions. |
| src/test/unit/unit_tests_tcp_ack.c | Add regression test for callback dispatch/reap behavior when a close callback recreates a socket. |
| src/test/unit/unit_tests_proto.c | Add regression test ensuring LISTEN sockets respect bound_local_ip for non-SYN segments. |
| src/test/unit/unit_tests_dns_edges.c | Add regression test accepting DNS responses with QR set but RD clear. |
| src/test/unit/unit_tests_branches.c | Add regression test for RAW sendto oversized size_t length handling. |
| src/port/stm32h563/ssh_server.h | Clarify uptime API is currently a placeholder returning 0. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+2638
to
+2639
| ck_assert_int_eq(wolfIP_sock_sendto(&s, fd, buf, (size_t)UINT32_MAX + 100, 0, | ||
| (struct wolfIP_sockaddr *)&sin, sizeof(sin)), -WOLFIP_EINVAL); |
Comment on lines
+11292
to
+11297
| if ((ts->callback == cb) && (ts->callback_arg == cb_arg) && | ||
| (ts->sock.tcp.state == TCP_CLOSED)) { | ||
| ts->callback = NULL; | ||
| ts->callback_arg = NULL; | ||
| close_socket(ts); | ||
| } |
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.
Temp bisect branch: F-10281, F-6471, F-9374, F-9380, F-6211, F-8523, F-8525 on master. macOS bisect for the test-wolfssl hang. Will be deleted.