Various hardening fixes - #148
Conversation
gasbytes
commented
Aug 5, 2026
gasbytes
commented
- ed23a84 - drop non-rst segments that arrive without a timestamp option on a conenction where timestamps were negotiated
- 3db744f - release the tcp socket slot when closing a socket that never left TCP_CLOSED
- a43d0fb - drop packets whose ipv4 option list contains an option with a length below the two-bytes minimum
- 90df38b - release the DHCP-learned DNS server along with the address, mask and gateway when a lease is lost
- 5fed382 - re-notify the ethernet-layer filter after the 802.1Q tag is tripped so L2 policy keyed on the vlan sub-interface index or the inner ethertype can match, instead of only ever seeing the parent interface and the 0x8100 TPID
- 03037d2 - connect the dns query socket to the configured resolver so the UDP demux drops replies that did not come from the server's port 53 (RFC 1035 section 4.2.1)
- eded7c3 - Add bytes offset tracking in parse_http_request when parsing header lines
- 93b91f3 - verify the DNS response question section against the outstanding query (per RFC 1035 section 7.3)
- 6749cf6 - do not forward datagrams received as link-layer broadcast or multicast per RFC 1812 section 5.3.4
per RFC 1812 section 5.3.4
(per RFC 1035 section 7.3)
drops replies that did not come from the server's port 53 (RFC 1035 section 4.2.1)
…o L2 policy keyed on the vlan sub-interface index or the inner ethertype can match, instead of only ever seeing the parent interface and the 0x8100 TPID
…gateway when a lease is lost
…below the two-bytes minimum
…enction where timestamps were negotiated
ed23a84 to
0801e6a
Compare
There was a problem hiding this comment.
Pull request overview
This PR delivers a set of TCP/IP stack hardening fixes across TCP, IPv4 option parsing/forwarding, DHCP/DNS behavior, VLAN filter visibility, and HTTP request parsing, with accompanying regression/unit tests.
Changes:
- Tighten protocol validation and forwarding behavior (TCP PAWS TSopt enforcement, IPv4 option length hard-drop, no forwarding of L2 broadcast/multicast frames).
- Improve socket/resource lifecycle correctness (free TCP slot on close from
TCP_CLOSED, preserve “pinned” DNS across DHCP lease loss, VLAN eth-filter re-notify after tag strip). - Strengthen DNS/HTTP parsing security and correctness (connect DNS socket to resolver, verify DNS question section, fix HTTP header accumulation and add standalone regression test).
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/wolfip.c | Core hardening changes across TCP input, socket close, DHCP/DNS handling, IPv4 option parsing/forwarding, VLAN filter notifications, and DNS query validation. |
| src/http/httpd.c | Fix header accumulation logic in parse_http_request by tracking an offset while copying header lines. |
| src/test/test_http_headers.c | New standalone regression test ensuring all HTTP headers are preserved in req.headers without overwrite/overflow. |
| src/test/unit/unit.c | Registers new/updated unit tests covering the new hardening behaviors. |
| src/test/unit/unit_shared.c | Adds DNS test helpers (arm_dns_query) and shared wire-format QNAME constants. |
| src/test/unit/unit_tests_api.c | Adds regression test ensuring undersized IPv4 options can’t hide source-route options from local delivery. |
| src/test/unit/unit_tests_proto.c | Adds regression test ensuring undersized IPv4 options can’t hide source-route options from forwarding path. |
| src/test/unit/unit_tests_ip_arp_recv.c | Adds forwarding regression test: never forward frames received as L2 broadcast; updates malformed-option test expectation to “drop”. |
| src/test/unit/unit_tests_tcp_ack.c | Updates DNS tests to arm query buffer correctly for new DNS question verification behavior. |
| src/test/unit/unit_tests_dns_edges.c | Updates DNS edge tests to use arm_dns_query to match new question verification. |
| src/test/unit/unit_tests_dns_dhcp.c | Updates TCP close tests (slot freed on TCP_CLOSED) and DNS tests to use arm_dns_query; adds new TCP close coverage. |
| src/test/unit/unit_tests_dhcp_edges.c | Adds DHCP tests verifying DNS server is released/relearned on lease loss, but pinned DNS persists. |
| src/test/unit/unit_tests_vlan.c | Adds VLAN eth-filter visibility tests (parent + subif notifications) and enforcement test blocking ARP learning via subif filter. |
| Makefile | Adds build/test-http-headers target to the standalone test suite. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #148
Scan targets checked: wolfip-bugs, wolfip-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
…local packet and use the l2 group address exemption only to skip the forwarding attempt
| req.headers[n] = '\0'; | ||
| { | ||
| size_t sep = (hdr_len > 0) ? 2 : 0; | ||
| if (hdr_len + sep + n >= sizeof(req.headers)) |
There was a problem hiding this comment.
This seems too strict? Before we had 512B total header budget, it seems to be shrunk to 400, which no longer can accommodate a default Chrome default request apparently is much larger.
Consider truncating + parsing what fits instead, and/or increasing HTTP_HEADERS_LEN.
Truncation + partial parsing seems to be safe as long as Content-Length and Transfer-Encoding are present, which are parsed before this copy.
Also style: please remove the anonymous blocks and move the definitions up at the beginning of the function.
| /* Once TSopt is negotiated the peer must carry it in every | ||
| * non-RST segment, so one that arrives without it is | ||
| * dropped silently. */ | ||
| if (!po.ts_found) | ||
| continue; |
There was a problem hiding this comment.
Check behavior consistency across TCP states:
This fix is correct and addresses ESTABLISHED, CLOSE_WAIT, FIN_WAIT_1 and _2, CLOSING.
The issue remains for TCP_TIME_WAIT and TCP_LAST_ACK cases, that stull accept non-RST segments without TSopt.
This creates some discrepancy for ecample when a retransmitted FIN without timestamp option is silently dropped if we are in FIN_WAIT_2 but it works if we are in LAST_ACK.
