Fenrir fixes 2026 06 05#128
Merged
Merged
Conversation
A malicious WRQ peer could advertise tsize up to UINT32_MAX in a single datagram and force wolftftp_server_start_request to hand that value to io.open as size_hint before any DATA arrived, enabling a multi-GiB pre-allocation DoS. The configured max_image_size was only enforced later on the DATA path (wolftftp_server_accept_wrq_data), which the attacker need never reach. Reject an advertised tsize that already exceeds cfg.max_image_size in start_request before io.open is called, sending ENOSPACE and reaping the session. This mirrors the existing client OACK check. The existing WRQ ENOSPACE-on-DATA test advertised tsize=10 with max_image_size=2; switch it to tsize=0 so it still exercises the DATA-path enforcement rather than the new up-front check.
wolftftp_client_accept_data enforced the per-transfer size limit with (client->total_size + data->data_len) > client->cfg.max_image_size. Once a rogue server streamed ~4 GiB without a short final block, total_size sat near UINT32_MAX and the unsigned addition wrapped, so the sum slipped below max_image_size and the guard was bypassed. The wrapped value then propagated into next_offset/total_size and the io.write offset, allowing already-written data to be overwritten (and the accumulators to wrap even when max_image_size == 0, which skips the cap entirely). Rewrite the guard to be wrap-safe: reject the block if total_size would exceed UINT32_MAX, and compare data_len against the remaining headroom (max_image_size - total_size) instead of adding first. total_size is kept <= max_image_size by this same check, so the subtraction never underflows. Add a regression test that fast-forwards total_size to just below the 32-bit limit and confirms the next block is rejected with ERR_SIZE and never reaches the write sink.
wolftftp_server_accept_wrq_data enforced the per-transfer size limit with (session->total_size + data->data_len) > server->cfg.max_image_size. Once a rogue client streamed ~4 GiB into a single WRQ session, total_size sat near UINT32_MAX and the unsigned addition wrapped, so the sum slipped below max_image_size and the guard was bypassed. The wrapped value then propagated into session->next_offset and the io.write offset, seeking back over already written data (a firmware header/signature region on embedded targets). When max_image_size == 0 the cap is skipped entirely and the accumulators still wrap unconditionally after 4 GiB. Rewrite the guard to be wrap-safe (same fix as F-4254 on the client RRQ path): reject the block if total_size would exceed UINT32_MAX, and compare data_len against the remaining headroom (max_image_size - total_size) instead of adding first. The first clause also bounds next_offset/total_size so the write offset can no longer wrap, including the max_image_size == 0 case. Add a regression test that fast-forwards a WRQ session's total_size to just below the 32-bit limit and confirms the next block is rejected with ERR_SIZE and never reaches the write sink.
…Q send The RRQ send path (wolftftp_server_send_window) accumulated next_offset and total_size (both uint32_t) with no max_image_size or overflow guard, unlike the symmetric WRQ receive path in wolftftp_server_accept_wrq_data. A reader that keeps returning full blocks would advance past the configured limit and, beyond 4 GiB, wrap the uint32_t offset that is passed to io.read (whose offset parameter is uint32_t), silently re-reading near-start data and corrupting the transfer. Add a per-block guard mirroring the WRQ path: terminate the session with an ENOSPACE error before total_size can exceed max_image_size or wrap UINT32_MAX. Using uint64_t accumulators (as the report suggested) would not help because the io.read/io.write callback offset is uint32_t.
Per RFC 2347 §2 a TFTP server's OACK may only acknowledge options the client actually requested. wolftftp_parse_oack accepted any recognized option and wolftftp_client_receive installed it into client->neg without consulting client->requested_opts, letting a malicious server or on-path attacker inject a timeout (per-retry interval inflation), windowsize (client withholds ACK until N blocks arrive while a window=1 server stalls), or tsize (size-cap abort) that was never negotiated. Pass client->requested_opts to wolftftp_parse_oack as a filter mask and reject with WOLFTFTP_ERR_UNSUPPORTED any option whose bit is not set, before its value is applied to neg.
A request retransmitted to the listen port (the client has not yet seen our reply) was dispatched straight to wolftftp_server_alloc_session, allocating a fresh slot each time. A single source could replay an RRQ/WRQ once per slot and pin the whole session pool, starving legitimate clients; a lossy client's own RRQ retries also leaked slots. Look up an existing session by remote endpoint in the listen-port branch and drop the duplicate so the in-progress session's poll-driven retransmit redelivers the reply.
…ction disclosure wolfip_wait_for_event_locked() drops wolfIP_mutex around host_poll() with an unbounded timeout. During that window a concurrent close() can release the public-fd slot (in_use=0, pipe closed, wolfIP TCP slot memset) and a following socket()/accept() can reuse the same public fd number and internal slot for an unrelated connection. The blocking caller (recv/recvfrom/send/sendto/write/...) captured its internal_fd before blocking and re-uses it after the wait, so it would read/write the reused slot's data and return it to the original caller. Mirror the revalidation already present in wolfip_accept_common: snapshot the slot's generation and internal_fd before dropping the mutex and, after re-acquiring it, return -EBADF if the slot was freed or reused. A monotonic per-alloc generation counter detects reuse even when the same public fd and internal slot are recycled. Fixing the single shared wait helper covers every blocking path that funnels through it.
… reuse wolfip_accept_common() drops wolfIP_mutex around host_poll() with an unbounded timeout while waiting for an incoming connection. During that window a concurrent close() can release the listener's public-fd slot (in_use=0, pipe closed) and a following socket()/accept() can reuse the same public fd number and internal slot for an unrelated connection. The post-poll re-resolution only re-fetched the entry via wolfip_entry_from_public(sockfd) and checked for NULL, which cannot distinguish the reused slot from the original listener: in_use is 1 again but internal_fd now points at the unrelated connection, so the accept would target the wrong internal socket and could steal another caller's pending connections. This path has its own inline poll loop and does not funnel through wolfip_wait_for_event_locked(), so the F-4781 revalidation does not cover it. Mirror that fix here: snapshot the listener slot's generation and internal_fd before dropping the mutex and, after re-acquiring it, return EBADF if the slot was freed or reused.
…muggling parse_http_request copied the entire recv-buffer tail after the blank line into req.body regardless of any declared Content-Length or Transfer-Encoding. A request with Content-Length: 0 and a second request appended in the same segment was delivered to the handler as a 61-byte body, providing a CL.0 request-smuggling primitive behind a CL-honouring reverse proxy (CWE-444). Parse Content-Length and Transfer-Encoding in the header loop and bound the body to the declared length: reject surplus bytes (Content-Length shorter than data), a body without Content-Length, duplicate/oversized Content-Length, and Transfer-Encoding (chunked unsupported). Also advance past the blank-line CRLF so the body offset is exact. Adds build/test-http-smuggle covering the smuggling case and well-formed GET/POST framing.
…ery/body When the query (GET) or body (POST) buffer was exactly full, the terminating NUL landed on the last byte of the array. The unconditional p = q + 1 then advanced one byte past the array end into the adjacent struct member (headers for GET, body_len for POST), so the loop interpreted that memory as further key=value pairs - an out-of-bounds read that could surface header content as a query argument and bypass query-parameter validation. Break out of the loop when q points at the terminating NUL instead of stepping past it. Adds a regression test exercising the full-buffer GET and POST cases plus normal lookups.
…info cross-contamination
The single-slot dns_wait_ctx used 'pending' both as the slot-occupied
flag (checked by wolfip_dns_begin_wait) and the query-outstanding flag
(cleared by the forward/reverse callbacks on completion). Because the
callback cleared pending=0 to signal completion, it simultaneously
released the slot before the original waiter had reaped the result. A
second thread's getaddrinfo() could then claim the slot, overwrite the
context, and the first thread would wait on / reap the second thread's
DNS response: thread A returns thread B's IP while thread B fails with
EAI_AGAIN despite a successful resolution.
Separate slot ownership ('busy', set in begin_wait and cleared only by
the reaper: wait success, type-mismatch, timeout, or abort) from the
query-outstanding signal ('pending', still cleared by the callbacks).
begin_wait now guards on 'busy', so the slot cannot be reused until the
owning thread has fully reaped its result, closing the TOCTOU window.
conditional_steal_call, conditional_steal_blocking_call and wolfip_accept_common assigned the raw negative wolfIP return code to errno (e.g. errno = -EINVAL) instead of its positive magnitude. Callers comparing errno == EINVAL / ENOTCONN / ECONNRESET would silently mismatch. Negate the value to match the adjacent hand-written paths (bind/close/sendto/send) and the macros' own EAGAIN/wait-error branches. Adds test-posix-errno which drives getpeername() through the conditional_steal_call macro and asserts errno is a positive value.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR applies a set of security and robustness fixes across the POSIX socket shim, HTTP request parsing, and TFTP client/server logic, with accompanying regression tests to prevent reintroduction of the reported issues.
Changes:
- Harden POSIX shim behavior against errno sign bugs, DNS wait-slot cross-contamination, and fd-slot reuse across blocking waits.
- Tighten HTTP request framing and argument parsing to prevent request-smuggling and OOB reads.
- Strengthen TFTP option negotiation and transfer-size enforcement to prevent slot pinning and overflow/size-cap bypasses, adding unit 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/port/posix/bsd_socket.c | Fix errno translation, add generation-based fd reuse detection, and serialize DNS wait-slot usage. |
| src/http/httpd.c | Enforce Content-Length-based body framing and stop arg parsing at NUL to prevent OOB reads. |
| src/tftp/wolftftp.c | Reject unsolicited OACK options, enforce overflow-safe max size checks, reject oversized WRQ tsize early, and coalesce duplicate RRQs. |
| src/test/unit/unit_tests_tftp.c | Add/adjust unit tests covering new TFTP security/overflow and slot-coalescing behavior. |
| src/test/test_posix_errno.c | Add standalone regression test for POSIX shim errno sign handling. |
| src/test/test_http_smuggle.c | Add standalone regression test for CL.0 request-smuggling rejection. |
| src/test/test_http_arg_oob.c | Add standalone regression test for httpd_get_request_arg NUL-termination/OOB behavior. |
| Makefile | Wire new standalone HTTP tests into default build; add a target for the POSIX errno regression test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gasbytes
approved these changes
Jun 5, 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.
e335905 F-4950: negate wolfIP error code when setting errno in POSIX shim macros
285437e F-5012: hold DNS wait slot until reaped to prevent concurrent getaddrinfo cross-contamination
f3cc191 F-5258: stop httpd_get_request_arg at NUL to prevent OOB read past query/body
9f08314 F-5259: derive HTTP body length from Content-Length to prevent CL.0 smuggling
af09c73 F-5072: revalidate listener slot after accept poll to prevent fd-slot reuse
92871de F-4781: revalidate fd slot after blocking wait to prevent cross-connection disclosure
4d2783e F-4765: coalesce retransmitted TFTP requests onto existing session
aad5aac F-4768: reject unsolicited OACK options not present in client RRQ
07cd58d F-5783: enforce max_image_size and prevent offset overflow on TFTP RRQ send
c7fba07 F-4253: prevent uint32_t overflow bypassing TFTP server WRQ size cap
5c791d9 F-4254: prevent uint32_t overflow bypassing TFTP client RRQ size cap
6e1999c F-4389: cap WRQ tsize against max_image_size before io.open