From 6e1999ce6d826bd184faba511db9002721ad6860 Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Fri, 5 Jun 2026 11:19:05 +0200 Subject: [PATCH 01/13] F-4389: cap WRQ tsize against max_image_size before io.open 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. --- src/test/unit/unit_tests_tftp.c | 42 ++++++++++++++++++++++++++++++++- src/tftp/wolftftp.c | 11 +++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/src/test/unit/unit_tests_tftp.c b/src/test/unit/unit_tests_tftp.c index fb86052..c8ee52d 100644 --- a/src/test/unit/unit_tests_tftp.c +++ b/src/test/unit/unit_tests_tftp.c @@ -670,8 +670,11 @@ START_TEST(test_tftp_server_wrq_success_and_failures) transport = tftp_transport_ops(&ctx); io = tftp_io_ops(&ctx); wolftftp_server_init(&server, &transport, &io, &cfg); + /* Advertise tsize=0 (size unknown) so the request is accepted and the + * oversized-payload rejection is exercised on the DATA path rather than + * up front against the advertised tsize. */ ck_assert_int_eq(wolftftp_build_request(pkt, sizeof(pkt), WOLFTFTP_OP_WRQ, - "fw.bin", &cfg, 10, &opts, &req_len), 0); + "fw.bin", &cfg, 0, &opts, &req_len), 0); ck_assert_int_eq(wolftftp_server_receive(&server, WOLFTFTP_PORT, &remote, pkt, req_len), 0); memcpy(pkt + 4, "toolong", 7); @@ -681,6 +684,42 @@ START_TEST(test_tftp_server_wrq_success_and_failures) } END_TEST +/* F-4389: a WRQ advertising tsize greater than cfg.max_image_size must be + * rejected before io.open is handed the size_hint, so a single datagram + * cannot force a huge pre-allocation. */ +START_TEST(test_tftp_server_wrq_tsize_exceeds_limit_rejected) +{ + struct tftp_test_ctx ctx; + struct wolftftp_server server; + struct wolftftp_transfer_cfg cfg = tftp_cfg_defaults(); + struct wolftftp_transport_ops transport; + struct wolftftp_io_ops io; + struct wolftftp_endpoint remote = tftp_remote(0x0A000012U, 5000); + uint8_t pkt[WOLFTFTP_PKT_MAX]; + uint16_t req_len = 0; + uint8_t opts = 0; + + tftp_test_ctx_reset(&ctx); + cfg.max_image_size = 128; + transport = tftp_transport_ops(&ctx); + io = tftp_io_ops(&ctx); + wolftftp_server_init(&server, &transport, &io, &cfg); + + /* WRQ with tsize = UINT32_MAX, far above max_image_size. */ + ck_assert_int_eq(wolftftp_build_request(pkt, sizeof(pkt), WOLFTFTP_OP_WRQ, + "fw.bin", &cfg, 0xFFFFFFFFU, &opts, &req_len), 0); + ck_assert_int_eq(wolftftp_server_receive(&server, WOLFTFTP_PORT, &remote, + pkt, req_len), WOLFTFTP_ERR_SIZE); + /* io.open must never have been reached with the oversized hint, and the + * session slot must be reaped. */ + ck_assert_int_eq(ctx.open_calls, 0); + ck_assert_int_eq(server.sessions[0].state, WOLFTFTP_SESSION_FREE); + /* The peer must have received an ENOSPACE error datagram. */ + ck_assert_int_eq(ctx.send_calls, 1); + ck_assert_uint_eq(wolftftp_read_u16(ctx.sent[0]), WOLFTFTP_OP_ERROR); +} +END_TEST + START_TEST(test_tftp_server_request_errors_and_timeouts) { struct tftp_test_ctx ctx; @@ -2108,6 +2147,7 @@ static void add_tftp_tests(TCase *tc_proto) tcase_add_test(tc_proto, test_tftp_client_poll_and_status_paths); tcase_add_test(tc_proto, test_tftp_server_rrq_success_and_poll); tcase_add_test(tc_proto, test_tftp_server_wrq_success_and_failures); + tcase_add_test(tc_proto, test_tftp_server_wrq_tsize_exceeds_limit_rejected); tcase_add_test(tc_proto, test_tftp_server_request_errors_and_timeouts); tcase_add_test(tc_proto, test_tftp_server_session_reaped_after_completion); tcase_add_test(tc_proto, test_tftp_client_honors_caller_server_port); diff --git a/src/tftp/wolftftp.c b/src/tftp/wolftftp.c index a43723c..6a531db 100644 --- a/src/tftp/wolftftp.c +++ b/src/tftp/wolftftp.c @@ -979,6 +979,17 @@ static int wolftftp_server_start_request(struct wolftftp_server *server, session->neg.windowsize = req->windowsize; session->neg.have_tsize = (uint8_t)((req->opts & WOLFTFTP_OPT_TSIZE) != 0U); session->neg.tsize = req->tsize; + /* Reject an advertised tsize that already exceeds the configured limit + * before io.open is handed the hint, so a single WRQ cannot force a + * 4 GiB pre-allocation. This mirrors the client OACK check and the + * later per-DATA enforcement in wolftftp_server_accept_wrq_data. */ + if (session->neg.have_tsize != 0U && server->cfg.max_image_size != 0U && + session->neg.tsize > server->cfg.max_image_size) { + (void)wolftftp_send_server_error(server, session->local_port, remote, + WOLFTFTP_ENOSPACE, "image too large"); + wolftftp_server_finish(server, session, WOLFTFTP_ERR_SIZE); + return WOLFTFTP_ERR_SIZE; + } (void)wolftftp_copy_string(session->filename, sizeof(session->filename), req->filename); From 5c791d9e00d95fe815afb0f9ae2a372cf480452c Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Fri, 5 Jun 2026 11:26:34 +0200 Subject: [PATCH 02/13] F-4254: prevent uint32_t overflow bypassing TFTP client RRQ size cap 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. --- src/test/unit/unit_tests_tftp.c | 47 +++++++++++++++++++++++++++++++++ src/tftp/wolftftp.c | 5 ++-- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/src/test/unit/unit_tests_tftp.c b/src/test/unit/unit_tests_tftp.c index c8ee52d..0213440 100644 --- a/src/test/unit/unit_tests_tftp.c +++ b/src/test/unit/unit_tests_tftp.c @@ -1782,6 +1782,52 @@ START_TEST(test_tftp_client_max_image_size_enforced_on_data) } END_TEST +START_TEST(test_tftp_client_data_size_cap_is_overflow_safe) +{ + /* A rogue server that streams ~4 GiB without a short final block can + * push client->total_size up to near UINT32_MAX. The size-cap guard + * must not be defeated by an unsigned wrap of (total_size + data_len): + * the next block has to be rejected with ERR_SIZE, not written at a + * wrapped offset. Regression test for F-4254. */ + struct tftp_test_ctx ctx; + struct wolftftp_client client; + struct wolftftp_transfer_cfg cfg = tftp_cfg_defaults(); + struct wolftftp_transport_ops transport; + struct wolftftp_io_ops io; + struct wolftftp_endpoint srv = tftp_remote(0x0A0000A1U, 0); + struct wolftftp_endpoint tid = tftp_remote(srv.ip, 7000); + uint8_t pkt[WOLFTFTP_PKT_MAX]; + int len; + + cfg.max_image_size = 0xFFFFFFFFU; + tftp_test_ctx_reset(&ctx); + transport = tftp_transport_ops(&ctx); + io = tftp_io_ops(&ctx); + wolftftp_client_init(&client, &transport, &io, &cfg); + ck_assert_int_eq(wolftftp_client_start_rrq(&client, &srv, "fw.bin"), 0); + + /* Block 1 establishes the TID and the data phase (16 bytes written). */ + memcpy(pkt + 4, "0123456789abcdef", 16); + len = wolftftp_build_data(pkt, sizeof(pkt), 1, pkt + 4, 16); + ck_assert_int_eq(wolftftp_client_receive(&client, cfg.local_port, + &tid, pkt, (uint16_t)len), 0); + ck_assert_int_eq(ctx.write_calls, 1); + + /* Fast-forward the accumulated total to just below the 32-bit limit, + * as if ~4 GiB had already arrived. Adding the next 16-byte block + * would wrap (total_size + 16) back to a small value that slips under + * max_image_size, defeating the cap. */ + client.total_size = 0xFFFFFFF8U; + memcpy(pkt + 4, "ghijklmnopqrstuv", 16); + len = wolftftp_build_data(pkt, sizeof(pkt), 2, pkt + 4, 16); + ck_assert_int_eq(wolftftp_client_receive(&client, cfg.local_port, + &tid, pkt, (uint16_t)len), WOLFTFTP_ERR_SIZE); + ck_assert_uint_eq(client.state, WOLFTFTP_CLIENT_ERROR); + /* The wrapped block must never reach the write sink. */ + ck_assert_int_eq(ctx.write_calls, 1); +} +END_TEST + START_TEST(test_tftp_client_duplicate_block_replays_last_ack) { /* RFC 1350: if the receiver sees an out-of-order block matching @@ -2169,6 +2215,7 @@ static void add_tftp_tests(TCase *tc_proto) tcase_add_test(tc_proto, test_tftp_client_unexpected_opcode_rejected); tcase_add_test(tc_proto, test_tftp_client_invalid_first_data_does_not_lock_tid); tcase_add_test(tc_proto, test_tftp_client_max_image_size_enforced_on_data); + tcase_add_test(tc_proto, test_tftp_client_data_size_cap_is_overflow_safe); tcase_add_test(tc_proto, test_tftp_client_duplicate_block_replays_last_ack); tcase_add_test(tc_proto, test_tftp_client_open_sink_missing_callbacks); tcase_add_test(tc_proto, test_tftp_client_oack_tsize_exceeds_limit); diff --git a/src/tftp/wolftftp.c b/src/tftp/wolftftp.c index 6a531db..5b0ebbc 100644 --- a/src/tftp/wolftftp.c +++ b/src/tftp/wolftftp.c @@ -645,8 +645,9 @@ static int wolftftp_client_accept_data(struct wolftftp_client *client, int ret; if (data->block == client->expected_block) { - if (client->cfg.max_image_size != 0U && - (client->total_size + data->data_len) > client->cfg.max_image_size) { + if (client->total_size > (uint32_t)(UINT32_MAX - data->data_len) || + (client->cfg.max_image_size != 0U && + data->data_len > client->cfg.max_image_size - client->total_size)) { (void)wolftftp_send_client_error(client, &client->server, WOLFTFTP_ENOSPACE, "image too large"); wolftftp_client_finish(client, WOLFTFTP_ERR_SIZE); From c7fba07dde9b51051c482a67322379a1e8980647 Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Fri, 5 Jun 2026 11:32:18 +0200 Subject: [PATCH 03/13] F-4253: prevent uint32_t overflow bypassing TFTP server WRQ size cap 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. --- src/test/unit/unit_tests_tftp.c | 53 +++++++++++++++++++++++++++++++++ src/tftp/wolftftp.c | 5 ++-- 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/src/test/unit/unit_tests_tftp.c b/src/test/unit/unit_tests_tftp.c index 0213440..96450f7 100644 --- a/src/test/unit/unit_tests_tftp.c +++ b/src/test/unit/unit_tests_tftp.c @@ -2128,6 +2128,58 @@ START_TEST(test_tftp_server_wrq_hash_failure_and_size_overflow) } END_TEST +START_TEST(test_tftp_server_wrq_data_size_cap_is_overflow_safe) +{ + /* A rogue client that streams ~4 GiB into a WRQ session can push + * session->total_size up to near UINT32_MAX. The size-cap guard must + * not be defeated by an unsigned wrap of (total_size + data_len): the + * next block has to be rejected with ERR_SIZE, not written at a wrapped + * offset that seeks back over already-written data. Regression for + * F-4253 (server WRQ counterpart of F-4254). */ + struct tftp_test_ctx ctx; + struct wolftftp_server server; + struct wolftftp_transfer_cfg cfg = tftp_cfg_defaults(); + struct wolftftp_transport_ops transport; + struct wolftftp_io_ops io; + struct wolftftp_endpoint remote = tftp_remote(0x0A000095U, 7500); + uint8_t pkt[WOLFTFTP_PKT_MAX]; + uint16_t req_len = 0; + uint8_t opts = 0; + int len; + + cfg.max_image_size = 0xFFFFFFFFU; + tftp_test_ctx_reset(&ctx); + transport = tftp_transport_ops(&ctx); + io = tftp_io_ops(&ctx); + wolftftp_server_init(&server, &transport, &io, &cfg); + ck_assert_int_eq(wolftftp_build_request(pkt, sizeof(pkt), + WOLFTFTP_OP_WRQ, "fw.bin", &cfg, 0, &opts, &req_len), 0); + ck_assert_int_eq(wolftftp_server_receive(&server, WOLFTFTP_PORT, &remote, + pkt, req_len), 0); + + /* Block 1: full blksize, enters the data phase (16 bytes written). */ + memcpy(pkt + 4, "0123456789abcdef", 16); + len = wolftftp_build_data(pkt, sizeof(pkt), 1, pkt + 4, 16); + ck_assert_int_eq(wolftftp_server_receive(&server, + server.sessions[0].local_port, &remote, pkt, (uint16_t)len), 0); + ck_assert_int_eq(ctx.write_calls, 1); + + /* Fast-forward the accumulated total to just below the 32-bit limit, + * as if ~4 GiB had already arrived. Adding the next 16-byte block + * would wrap (total_size + 16) back to a small value that slips under + * max_image_size, defeating the cap. */ + server.sessions[0].total_size = 0xFFFFFFF8U; + memcpy(pkt + 4, "ghijklmnopqrstuv", 16); + len = wolftftp_build_data(pkt, sizeof(pkt), 2, pkt + 4, 16); + ck_assert_int_eq(wolftftp_server_receive(&server, + server.sessions[0].local_port, &remote, pkt, (uint16_t)len), + WOLFTFTP_ERR_SIZE); + ck_assert_uint_eq(server.sessions[0].state, WOLFTFTP_SESSION_FREE); + /* The wrapped block must never reach the write sink. */ + ck_assert_int_eq(ctx.write_calls, 1); +} +END_TEST + START_TEST(test_tftp_server_rrq_ack_bad_then_recover) { /* ACK that doesn't match last_acked / (next_block-1) / OACK-ack-0 @@ -2223,5 +2275,6 @@ static void add_tftp_tests(TCase *tc_proto) tcase_add_test(tc_proto, test_tftp_server_retries_exhausted_to_timeout); tcase_add_test(tc_proto, test_tftp_server_wrq_full_flow_with_options); tcase_add_test(tc_proto, test_tftp_server_wrq_hash_failure_and_size_overflow); + tcase_add_test(tc_proto, test_tftp_server_wrq_data_size_cap_is_overflow_safe); tcase_add_test(tc_proto, test_tftp_server_rrq_ack_bad_then_recover); } diff --git a/src/tftp/wolftftp.c b/src/tftp/wolftftp.c index 5b0ebbc..dbf6c5a 100644 --- a/src/tftp/wolftftp.c +++ b/src/tftp/wolftftp.c @@ -1061,8 +1061,9 @@ static int wolftftp_server_accept_wrq_data(struct wolftftp_server *server, * trying to replay the OACK now that we have entered the * data phase. */ session->options_sent = 0; - if (server->cfg.max_image_size != 0U && - (session->total_size + data->data_len) > server->cfg.max_image_size) { + if (session->total_size > (uint32_t)(UINT32_MAX - data->data_len) || + (server->cfg.max_image_size != 0U && + data->data_len > server->cfg.max_image_size - session->total_size)) { (void)wolftftp_send_server_error(server, session->local_port, &session->remote, WOLFTFTP_ENOSPACE, "image too large"); wolftftp_server_finish(server, session, WOLFTFTP_ERR_SIZE); From 07cd58dee6d1db4ccde4c451f29f76afa2e41da6 Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Fri, 5 Jun 2026 11:38:57 +0200 Subject: [PATCH 04/13] F-5783: enforce max_image_size and prevent offset overflow on TFTP RRQ 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. --- src/test/unit/unit_tests_tftp.c | 80 +++++++++++++++++++++++++++++++++ src/tftp/wolftftp.c | 13 ++++++ 2 files changed, 93 insertions(+) diff --git a/src/test/unit/unit_tests_tftp.c b/src/test/unit/unit_tests_tftp.c index 96450f7..f6d3e40 100644 --- a/src/test/unit/unit_tests_tftp.c +++ b/src/test/unit/unit_tests_tftp.c @@ -1132,6 +1132,84 @@ START_TEST(test_tftp_server_rrq_sends_zero_byte_terminator_on_exact_multiple) } END_TEST +/* F-5783: the RRQ send path accumulates next_offset/total_size (uint32_t) + * with no max_image_size guard, unlike the WRQ receive path. A reader that + * keeps returning full blocks would advance past the configured limit and + * eventually wrap the uint32_t offset, re-reading near-start data. The + * server must instead refuse to send past max_image_size with an ENOSPACE + * error, mirroring the WRQ per-DATA guard. */ +START_TEST(test_tftp_server_rrq_max_image_size_enforced_on_send) +{ + struct tftp_test_ctx ctx; + struct wolftftp_server server; + struct wolftftp_transfer_cfg cfg; + struct wolftftp_transfer_cfg req_cfg; + struct wolftftp_transport_ops transport; + struct wolftftp_io_ops io; + struct wolftftp_endpoint remote = tftp_remote(0x0A000060U, 4300); + uint8_t pkt[WOLFTFTP_PKT_MAX]; + uint16_t req_len = 0; + uint8_t opts = 0; + uint16_t blksize = 8U; + + memset(&cfg, 0, sizeof(cfg)); + cfg.blksize = blksize; + cfg.timeout_s = 1; + cfg.windowsize = 1; + cfg.max_retries = 3; + /* Exactly two full blocks may be sent; the third must be rejected. */ + cfg.max_image_size = 2U * blksize; + + memset(&req_cfg, 0, sizeof(req_cfg)); + req_cfg.blksize = WOLFTFTP_DEFAULT_BLKSIZE; + req_cfg.timeout_s = WOLFTFTP_DEFAULT_TIMEOUT_S; + req_cfg.windowsize = 1; + + tftp_test_ctx_reset(&ctx); + /* A reader that never signals EOF: every read returns a full block. */ + memset(ctx.read_data, 'A', blksize); + memset(ctx.read_data + blksize, 'B', blksize); + memset(ctx.read_data + 2 * blksize, 'C', blksize); + ctx.read_len[0] = blksize; ctx.read_last[0] = 0; + ctx.read_len[1] = blksize; ctx.read_last[1] = 0; + ctx.read_len[2] = blksize; ctx.read_last[2] = 0; + transport = tftp_transport_ops(&ctx); + io = tftp_io_ops(&ctx); + wolftftp_server_init(&server, &transport, &io, &cfg); + + ck_assert_int_eq(wolftftp_build_request(pkt, sizeof(pkt), WOLFTFTP_OP_RRQ, + "fw.bin", &req_cfg, 0, &opts, &req_len), 0); + ck_assert_uint_eq(opts, 0U); + ck_assert_int_eq(wolftftp_server_receive(&server, WOLFTFTP_PORT, &remote, + pkt, req_len), 0); + /* Block 1 (total 8 of 16). */ + ck_assert_int_eq(ctx.send_calls, 1); + ck_assert_uint_eq(wolftftp_read_u16(ctx.sent[0]), WOLFTFTP_OP_DATA); + ck_assert_uint_eq(wolftftp_read_u16(ctx.sent[0] + 2), 1U); + + /* ACK 1 → block 2 (total 16 of 16). */ + wolftftp_write_u16(pkt, WOLFTFTP_OP_ACK); + wolftftp_write_u16(pkt + 2, 1); + ck_assert_int_eq(wolftftp_server_receive(&server, server.sessions[0].local_port, + &remote, pkt, 4), 0); + ck_assert_int_eq(ctx.send_calls, 2); + ck_assert_uint_eq(wolftftp_read_u16(ctx.sent[1]), WOLFTFTP_OP_DATA); + ck_assert_uint_eq(wolftftp_read_u16(ctx.sent[1] + 2), 2U); + + /* ACK 2 → a third full block would push total past max_image_size; + * the server must answer with an ENOSPACE error, not more DATA, and + * reap the session. Before the fix this sent DATA block 3. */ + wolftftp_write_u16(pkt, WOLFTFTP_OP_ACK); + wolftftp_write_u16(pkt + 2, 2); + ck_assert_int_eq(wolftftp_server_receive(&server, server.sessions[0].local_port, + &remote, pkt, 4), WOLFTFTP_ERR_SIZE); + ck_assert_int_eq(ctx.send_calls, 3); + ck_assert_uint_eq(wolftftp_read_u16(ctx.sent[2]), WOLFTFTP_OP_ERROR); + ck_assert_uint_eq(wolftftp_read_u16(ctx.sent[2] + 2), WOLFTFTP_ENOSPACE); + ck_assert_int_eq(server.sessions[0].state, WOLFTFTP_SESSION_FREE); +} +END_TEST + START_TEST(test_tftp_server_poll_deadline_is_wrap_safe) { struct tftp_test_ctx ctx; @@ -2255,6 +2333,8 @@ static void add_tftp_tests(TCase *tc_proto) tcase_add_test(tc_proto, test_tftp_server_rrq_retransmit_replays_window); tcase_add_test(tc_proto, test_tftp_client_poll_deadline_is_wrap_safe); tcase_add_test(tc_proto, test_tftp_server_poll_deadline_is_wrap_safe); + tcase_add_test(tc_proto, + test_tftp_server_rrq_max_image_size_enforced_on_send); tcase_add_test(tc_proto, test_tftp_server_rrq_sends_zero_byte_terminator_on_exact_multiple); tcase_add_test(tc_proto, diff --git a/src/tftp/wolftftp.c b/src/tftp/wolftftp.c index dbf6c5a..2cdfb54 100644 --- a/src/tftp/wolftftp.c +++ b/src/tftp/wolftftp.c @@ -922,6 +922,19 @@ static int wolftftp_server_send_window(struct wolftftp_server *server, if (ret != 0) return WOLFTFTP_ERR_IO; data_len = out_len; + /* Refuse to advance past the configured limit or wrap the + * uint32_t offset/total accumulators. io.read takes a uint32_t + * offset, so a transfer beyond 4 GiB would silently wrap and + * re-read near-start data; terminate instead. Mirrors the WRQ + * per-DATA guard in wolftftp_server_accept_wrq_data. */ + if (session->total_size > (uint32_t)(UINT32_MAX - data_len) || + (server->cfg.max_image_size != 0U && + data_len > server->cfg.max_image_size - session->total_size)) { + (void)wolftftp_send_server_error(server, session->local_port, + &session->remote, WOLFTFTP_ENOSPACE, "image too large"); + wolftftp_server_finish(server, session, WOLFTFTP_ERR_SIZE); + return WOLFTFTP_ERR_SIZE; + } ret = wolftftp_build_data(pkt, sizeof(pkt), session->next_block, pkt + 4, data_len); if (ret < 0) From aad5aaca16920bf22c87539eac9f3638f131087f Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Fri, 5 Jun 2026 12:51:01 +0200 Subject: [PATCH 05/13] F-4768: reject unsolicited OACK options not present in client RRQ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/test/unit/unit_tests_tftp.c | 90 ++++++++++++++++++++++++++++----- src/tftp/wolftftp.c | 16 +++++- 2 files changed, 90 insertions(+), 16 deletions(-) diff --git a/src/test/unit/unit_tests_tftp.c b/src/test/unit/unit_tests_tftp.c index f6d3e40..c71306a 100644 --- a/src/test/unit/unit_tests_tftp.c +++ b/src/test/unit/unit_tests_tftp.c @@ -256,12 +256,14 @@ START_TEST(test_tftp_helpers_and_builders) WOLFTFTP_OPT_BLKSIZE | WOLFTFTP_OPT_TIMEOUT | WOLFTFTP_OPT_TSIZE | WOLFTFTP_OPT_WINDOWSIZE), 0); ck_assert_int_eq(wolftftp_parse_oack(pkt, (uint16_t)strlen((char *)(pkt + 2)) + 14, - &neg), WOLFTFTP_ERR_PACKET); + &neg, WOLFTFTP_OPT_BLKSIZE | WOLFTFTP_OPT_TIMEOUT | + WOLFTFTP_OPT_TSIZE | WOLFTFTP_OPT_WINDOWSIZE), WOLFTFTP_ERR_PACKET); wolftftp_neg_defaults(&neg, &cfg); ck_assert_int_eq(wolftftp_parse_oack(pkt, (uint16_t)(2 + strlen("blksize") + 1 + strlen("16") + 1 + strlen("timeout") + 1 + strlen("2") + 1 + strlen("tsize") + 1 + strlen("33") + 1 + strlen("windowsize") + 1 + - strlen("2") + 1), &neg), 0); + strlen("2") + 1), &neg, WOLFTFTP_OPT_BLKSIZE | WOLFTFTP_OPT_TIMEOUT | + WOLFTFTP_OPT_TSIZE | WOLFTFTP_OPT_WINDOWSIZE), 0); ck_assert_uint_eq(neg.tsize, 33U); ck_assert_uint_eq(neg.have_tsize, 1U); @@ -325,7 +327,7 @@ START_TEST(test_tftp_parse_request_error_paths) memcpy(pkt + 2, "blksize\0", 8); memcpy(pkt + 10, "512", 3); wolftftp_neg_defaults(&neg, &cfg); - ck_assert_int_eq(wolftftp_parse_oack(pkt, 13, &neg), + ck_assert_int_eq(wolftftp_parse_oack(pkt, 13, &neg, WOLFTFTP_OPT_BLKSIZE), WOLFTFTP_ERR_PACKET); } END_TEST @@ -899,7 +901,7 @@ START_TEST(test_tftp_parse_tsize_rejects_non_numeric) wolftftp_write_u16(pkt, WOLFTFTP_OP_OACK); memcpy(pkt + 2, "tsize\0abc\0", 10); wolftftp_neg_defaults(&neg, &cfg); - ck_assert_int_eq(wolftftp_parse_oack(pkt, 12, &neg), + ck_assert_int_eq(wolftftp_parse_oack(pkt, 12, &neg, WOLFTFTP_OPT_TSIZE), WOLFTFTP_ERR_UNSUPPORTED); /* tsize=0 is still valid and parses to 0. */ @@ -907,7 +909,7 @@ START_TEST(test_tftp_parse_tsize_rejects_non_numeric) wolftftp_write_u16(pkt, WOLFTFTP_OP_OACK); memcpy(pkt + 2, "tsize\0" "0\0", 8); wolftftp_neg_defaults(&neg, &cfg); - ck_assert_int_eq(wolftftp_parse_oack(pkt, 10, &neg), 0); + ck_assert_int_eq(wolftftp_parse_oack(pkt, 10, &neg, WOLFTFTP_OPT_TSIZE), 0); ck_assert_uint_eq(neg.have_tsize, 1U); ck_assert_uint_eq(neg.tsize, 0U); } @@ -1579,14 +1581,14 @@ START_TEST(test_tftp_builders_overflow_and_bad_args) { struct wolftftp_negotiated n; wolftftp_neg_defaults(&n, &cfg); - ck_assert_int_eq(wolftftp_parse_oack(NULL, 10, &n), + ck_assert_int_eq(wolftftp_parse_oack(NULL, 10, &n, 0), WOLFTFTP_ERR_PACKET); - ck_assert_int_eq(wolftftp_parse_oack(pkt, 10, NULL), + ck_assert_int_eq(wolftftp_parse_oack(pkt, 10, NULL, 0), WOLFTFTP_ERR_PACKET); - ck_assert_int_eq(wolftftp_parse_oack(pkt, 1, &n), + ck_assert_int_eq(wolftftp_parse_oack(pkt, 1, &n, 0), WOLFTFTP_ERR_PACKET); wolftftp_write_u16(pkt, WOLFTFTP_OP_RRQ); /* wrong */ - ck_assert_int_eq(wolftftp_parse_oack(pkt, 4, &n), + ck_assert_int_eq(wolftftp_parse_oack(pkt, 4, &n, 0), WOLFTFTP_ERR_PACKET); } } @@ -1712,7 +1714,7 @@ START_TEST(test_tftp_parse_option_ranges) wolftftp_write_u16(pkt, WOLFTFTP_OP_OACK); memcpy(pkt + 2, "blksize\0" "4\0", 10); wolftftp_neg_defaults(&neg, &cfg); - ck_assert_int_eq(wolftftp_parse_oack(pkt, 12, &neg), + ck_assert_int_eq(wolftftp_parse_oack(pkt, 12, &neg, WOLFTFTP_OPT_BLKSIZE), WOLFTFTP_ERR_UNSUPPORTED); /* OACK: timeout=0 → rejected. */ @@ -1720,7 +1722,7 @@ START_TEST(test_tftp_parse_option_ranges) wolftftp_write_u16(pkt, WOLFTFTP_OP_OACK); memcpy(pkt + 2, "timeout\0" "0\0", 10); wolftftp_neg_defaults(&neg, &cfg); - ck_assert_int_eq(wolftftp_parse_oack(pkt, 12, &neg), + ck_assert_int_eq(wolftftp_parse_oack(pkt, 12, &neg, WOLFTFTP_OPT_TIMEOUT), WOLFTFTP_ERR_UNSUPPORTED); /* OACK: windowsize=0 → rejected. */ @@ -1728,7 +1730,7 @@ START_TEST(test_tftp_parse_option_ranges) wolftftp_write_u16(pkt, WOLFTFTP_OP_OACK); memcpy(pkt + 2, "windowsize\0" "0\0", 13); wolftftp_neg_defaults(&neg, &cfg); - ck_assert_int_eq(wolftftp_parse_oack(pkt, 15, &neg), + ck_assert_int_eq(wolftftp_parse_oack(pkt, 15, &neg, WOLFTFTP_OPT_WINDOWSIZE), WOLFTFTP_ERR_UNSUPPORTED); /* OACK: unknown option → rejected. */ @@ -1736,7 +1738,9 @@ START_TEST(test_tftp_parse_option_ranges) wolftftp_write_u16(pkt, WOLFTFTP_OP_OACK); memcpy(pkt + 2, "mystery\0" "1\0", 10); wolftftp_neg_defaults(&neg, &cfg); - ck_assert_int_eq(wolftftp_parse_oack(pkt, 12, &neg), + ck_assert_int_eq(wolftftp_parse_oack(pkt, 12, &neg, + WOLFTFTP_OPT_BLKSIZE | WOLFTFTP_OPT_TIMEOUT | + WOLFTFTP_OPT_TSIZE | WOLFTFTP_OPT_WINDOWSIZE), WOLFTFTP_ERR_UNSUPPORTED); /* OACK: full valid option set (covers all 4 accepted branches). @@ -1755,7 +1759,8 @@ START_TEST(test_tftp_parse_option_ranges) memcpy(pkt + 2, payload, payload_len); wolftftp_neg_defaults(&neg, &cfg); ck_assert_int_eq(wolftftp_parse_oack(pkt, (uint16_t)(2 + payload_len), - &neg), 0); + &neg, WOLFTFTP_OPT_BLKSIZE | WOLFTFTP_OPT_TIMEOUT | + WOLFTFTP_OPT_TSIZE | WOLFTFTP_OPT_WINDOWSIZE), 0); } ck_assert_uint_eq(neg.blksize, 32U); ck_assert_uint_eq(neg.timeout_s, 5U); @@ -1860,6 +1865,62 @@ START_TEST(test_tftp_client_max_image_size_enforced_on_data) } END_TEST +START_TEST(test_tftp_client_rejects_unsolicited_oack_options) +{ + /* RFC 2347 §2: an OACK may only acknowledge options the client actually + * sent. A client that issued a bare RRQ (requested_opts == 0) must reject + * a malicious server's OACK that injects a timeout or windowsize it never + * negotiated, instead of silently installing the server-controlled value + * (timeout inflation / windowsize deadlock). Regression test for F-4768. */ + struct tftp_test_ctx ctx; + struct wolftftp_client client; + struct wolftftp_transfer_cfg cfg = tftp_cfg_defaults(); + struct wolftftp_transport_ops transport; + struct wolftftp_io_ops io; + struct wolftftp_endpoint srv = tftp_remote(0x0A0000A1U, 0); + struct wolftftp_endpoint tid = tftp_remote(srv.ip, 4242); + struct wolftftp_negotiated rogue; + uint8_t pkt[WOLFTFTP_PKT_MAX]; + int len; + + /* Bare RRQ: no blksize/timeout/windowsize/tsize options are sent. */ + cfg.blksize = WOLFTFTP_DEFAULT_BLKSIZE; + cfg.timeout_s = WOLFTFTP_DEFAULT_TIMEOUT_S; + cfg.windowsize = 1; + cfg.max_image_size = 0; + tftp_test_ctx_reset(&ctx); + transport = tftp_transport_ops(&ctx); + io = tftp_io_ops(&ctx); + wolftftp_client_init(&client, &transport, &io, &cfg); + ck_assert_int_eq(wolftftp_client_start_rrq(&client, &srv, "fw.bin"), 0); + ck_assert_uint_eq(client.requested_opts, 0U); + + /* Server injects timeout=255 the client never asked for. */ + wolftftp_neg_defaults(&rogue, &cfg); + rogue.timeout_s = 255; + len = wolftftp_build_oack(pkt, sizeof(pkt), &rogue, WOLFTFTP_OPT_TIMEOUT); + ck_assert_int_gt(len, 0); + ck_assert_int_eq(wolftftp_client_receive(&client, cfg.local_port, + &tid, pkt, (uint16_t)len), WOLFTFTP_ERR_UNSUPPORTED); + ck_assert_uint_eq(client.state, WOLFTFTP_CLIENT_ERROR); + /* The unsolicited value must NOT have been installed. */ + ck_assert_uint_eq(client.neg.timeout_s, WOLFTFTP_DEFAULT_TIMEOUT_S); + + /* Same for an unsolicited windowsize=8 (windowsize deadlock vector). */ + tftp_test_ctx_reset(&ctx); + wolftftp_client_init(&client, &transport, &io, &cfg); + ck_assert_int_eq(wolftftp_client_start_rrq(&client, &srv, "fw.bin"), 0); + wolftftp_neg_defaults(&rogue, &cfg); + rogue.windowsize = 8; + len = wolftftp_build_oack(pkt, sizeof(pkt), &rogue, WOLFTFTP_OPT_WINDOWSIZE); + ck_assert_int_gt(len, 0); + ck_assert_int_eq(wolftftp_client_receive(&client, cfg.local_port, + &tid, pkt, (uint16_t)len), WOLFTFTP_ERR_UNSUPPORTED); + ck_assert_uint_eq(client.state, WOLFTFTP_CLIENT_ERROR); + ck_assert_uint_eq(client.neg.windowsize, 1U); +} +END_TEST + START_TEST(test_tftp_client_data_size_cap_is_overflow_safe) { /* A rogue server that streams ~4 GiB without a short final block can @@ -2351,6 +2412,7 @@ static void add_tftp_tests(TCase *tc_proto) tcase_add_test(tc_proto, test_tftp_client_duplicate_block_replays_last_ack); tcase_add_test(tc_proto, test_tftp_client_open_sink_missing_callbacks); tcase_add_test(tc_proto, test_tftp_client_oack_tsize_exceeds_limit); + tcase_add_test(tc_proto, test_tftp_client_rejects_unsolicited_oack_options); tcase_add_test(tc_proto, test_tftp_server_io_missing_for_rrq_and_wrq); tcase_add_test(tc_proto, test_tftp_server_retries_exhausted_to_timeout); tcase_add_test(tc_proto, test_tftp_server_wrq_full_flow_with_options); diff --git a/src/tftp/wolftftp.c b/src/tftp/wolftftp.c index 2cdfb54..3c0bf0d 100644 --- a/src/tftp/wolftftp.c +++ b/src/tftp/wolftftp.c @@ -458,7 +458,7 @@ static int wolftftp_build_oack(uint8_t *buf, uint16_t max_len, } static int wolftftp_parse_oack(const uint8_t *buf, uint16_t len, - struct wolftftp_negotiated *neg) + struct wolftftp_negotiated *neg, uint8_t requested_opts) { const char *p; size_t slen; @@ -486,21 +486,33 @@ static int wolftftp_parse_oack(const uint8_t *buf, uint16_t len, * past buf+len. */ if (slen == 0 || (const uint8_t *)(value + slen) >= buf + len) return WOLFTFTP_ERR_PACKET; + /* RFC 2347 §2: an OACK may only acknowledge options the client + * actually sent. Reject any option absent from requested_opts so a + * malicious server cannot install unsolicited timeout/windowsize/ + * tsize values that were never negotiated. */ if (wolftftp_stricmp_local(key, "blksize") == 0) { + if ((requested_opts & WOLFTFTP_OPT_BLKSIZE) == 0U) + return WOLFTFTP_ERR_UNSUPPORTED; if (wolftftp_parse_u32(value, WOLFTFTP_MAX_BLKSIZE, &number) != 0 || number < 8U) return WOLFTFTP_ERR_UNSUPPORTED; neg->blksize = (uint16_t)number; } else if (wolftftp_stricmp_local(key, "timeout") == 0) { + if ((requested_opts & WOLFTFTP_OPT_TIMEOUT) == 0U) + return WOLFTFTP_ERR_UNSUPPORTED; if (wolftftp_parse_u32(value, 255U, &number) != 0 || number == 0U) return WOLFTFTP_ERR_UNSUPPORTED; neg->timeout_s = (uint16_t)number; } else if (wolftftp_stricmp_local(key, "tsize") == 0) { + if ((requested_opts & WOLFTFTP_OPT_TSIZE) == 0U) + return WOLFTFTP_ERR_UNSUPPORTED; if (wolftftp_parse_u32(value, 0xFFFFFFFFUL, &number) != 0) return WOLFTFTP_ERR_UNSUPPORTED; neg->tsize = number; neg->have_tsize = 1; } else if (wolftftp_stricmp_local(key, "windowsize") == 0) { + if ((requested_opts & WOLFTFTP_OPT_WINDOWSIZE) == 0U) + return WOLFTFTP_ERR_UNSUPPORTED; if (wolftftp_parse_u32(value, WOLFTFTP_MAX_WINDOWSIZE, &number) != 0 || number == 0U) return WOLFTFTP_ERR_UNSUPPORTED; @@ -748,7 +760,7 @@ int wolftftp_client_receive(struct wolftftp_client *client, uint16_t local_port, client->server.port = remote->port; client->tid_locked = 1; wolftftp_neg_defaults(&neg, &client->cfg); - ret = wolftftp_parse_oack(buf, len, &neg); + ret = wolftftp_parse_oack(buf, len, &neg, client->requested_opts); if (ret != 0) { wolftftp_client_finish(client, ret); return ret; From 4d2783e4205f77c2d21c514914b2c5a2b2e902eb Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Fri, 5 Jun 2026 13:19:45 +0200 Subject: [PATCH 06/13] F-4765: coalesce retransmitted TFTP requests onto existing session 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. --- src/test/unit/unit_tests_tftp.c | 51 +++++++++++++++++++++++++++++++++ src/tftp/wolftftp.c | 23 +++++++++++++++ 2 files changed, 74 insertions(+) diff --git a/src/test/unit/unit_tests_tftp.c b/src/test/unit/unit_tests_tftp.c index c71306a..ee8039d 100644 --- a/src/test/unit/unit_tests_tftp.c +++ b/src/test/unit/unit_tests_tftp.c @@ -818,6 +818,56 @@ START_TEST(test_tftp_server_session_reaped_after_completion) } END_TEST +/* F-4765: a retransmitted RRQ from the same remote endpoint (the client has + * not yet seen our reply) must be coalesced onto the in-progress session + * instead of allocating a fresh slot. Without this a single source can replay + * the listen-port request and pin every slot, starving legitimate clients. */ +START_TEST(test_tftp_server_duplicate_rrq_coalesced_to_one_slot) +{ + struct tftp_test_ctx ctx; + struct wolftftp_server server; + struct wolftftp_transfer_cfg cfg = tftp_cfg_defaults(); + struct wolftftp_transport_ops transport; + struct wolftftp_io_ops io; + struct wolftftp_endpoint atk = tftp_remote(0xC0A80001U, 54321); + struct wolftftp_endpoint legit = tftp_remote(0xC0A80002U, 12345); + uint8_t pkt[WOLFTFTP_PKT_MAX]; + uint16_t req_len = 0; + uint8_t opts = 0; + unsigned int i; + unsigned int occupied; + + tftp_test_ctx_reset(&ctx); + memcpy(ctx.read_data, "abcdefg", 7); + ctx.read_len[0] = 7; + ctx.read_last[0] = 1; + transport = tftp_transport_ops(&ctx); + io = tftp_io_ops(&ctx); + wolftftp_server_init(&server, &transport, &io, &cfg); + + /* One source replays the same RRQ once per slot. Only the first must + * allocate a session; the rest are duplicates of the live transfer. */ + ck_assert_int_eq(wolftftp_build_request(pkt, sizeof(pkt), WOLFTFTP_OP_RRQ, + "fw.bin", &cfg, 0, &opts, &req_len), 0); + for (i = 0; i < WOLFTFTP_SERVER_MAX_SESSIONS; i++) + ck_assert_int_eq(wolftftp_server_receive(&server, WOLFTFTP_PORT, &atk, + pkt, req_len), 0); + ck_assert_int_eq(ctx.open_calls, 1); + + occupied = 0; + for (i = 0; i < WOLFTFTP_SERVER_MAX_SESSIONS; i++) { + if (server.sessions[i].state != WOLFTFTP_SESSION_FREE) + occupied++; + } + ck_assert_uint_eq(occupied, 1); + + /* A legitimate client from a different endpoint still finds a free slot. */ + ck_assert_int_eq(wolftftp_server_receive(&server, WOLFTFTP_PORT, &legit, + pkt, req_len), 0); + ck_assert_int_eq(ctx.open_calls, 2); +} +END_TEST + START_TEST(test_tftp_client_honors_caller_server_port) { struct tftp_test_ctx ctx; @@ -2387,6 +2437,7 @@ static void add_tftp_tests(TCase *tc_proto) tcase_add_test(tc_proto, test_tftp_server_wrq_tsize_exceeds_limit_rejected); tcase_add_test(tc_proto, test_tftp_server_request_errors_and_timeouts); tcase_add_test(tc_proto, test_tftp_server_session_reaped_after_completion); + tcase_add_test(tc_proto, test_tftp_server_duplicate_rrq_coalesced_to_one_slot); tcase_add_test(tc_proto, test_tftp_client_honors_caller_server_port); tcase_add_test(tc_proto, test_tftp_client_default_port_when_zero); tcase_add_test(tc_proto, test_tftp_parse_tsize_rejects_non_numeric); diff --git a/src/tftp/wolftftp.c b/src/tftp/wolftftp.c index 3c0bf0d..aa7d9ab 100644 --- a/src/tftp/wolftftp.c +++ b/src/tftp/wolftftp.c @@ -886,6 +886,21 @@ static struct wolftftp_server_session *wolftftp_server_find_session( return NULL; } +static struct wolftftp_server_session *wolftftp_server_find_by_remote( + struct wolftftp_server *server, const struct wolftftp_endpoint *remote) +{ + unsigned int i; + + for (i = 0; i < WOLFTFTP_SERVER_MAX_SESSIONS; i++) { + if (server->sessions[i].state != WOLFTFTP_SESSION_FREE && + server->sessions[i].remote.ip == remote->ip && + server->sessions[i].remote.port == remote->port) { + return &server->sessions[i]; + } + } + return NULL; +} + static struct wolftftp_server_session *wolftftp_server_alloc_session( struct wolftftp_server *server) { @@ -1168,6 +1183,14 @@ int wolftftp_server_receive(struct wolftftp_server *server, uint16_t local_port, if (ret != 0) return wolftftp_send_server_error(server, server->listen_port, remote, WOLFTFTP_EBADOP, "bad request"); + /* A retransmitted request (the client has not yet seen our reply) + * lands back on the listen port while a session for the same remote + * endpoint is already active. Coalesce it onto that session instead + * of allocating a fresh slot, so a single source cannot pin the whole + * pool and a lossy client's retries do not leak slots. The in-progress + * session's poll-driven retransmit redelivers the pending reply. */ + if (wolftftp_server_find_by_remote(server, remote) != NULL) + return 0; return wolftftp_server_start_request(server, remote, &req); } From 92871dedcd1c34bb0a91e83866d4add1005374f5 Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Fri, 5 Jun 2026 15:58:51 +0200 Subject: [PATCH 07/13] F-4781: revalidate fd slot after blocking wait to prevent cross-connection 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. --- src/port/posix/bsd_socket.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/port/posix/bsd_socket.c b/src/port/posix/bsd_socket.c index 9d8b03d..dc1df9d 100644 --- a/src/port/posix/bsd_socket.c +++ b/src/port/posix/bsd_socket.c @@ -142,6 +142,7 @@ struct wolfip_fd_entry { uint8_t in_use; uint8_t pending_tokens; /* Bitset of queued event bytes in the pipe */ uint16_t events; /* Events armed for current poll/select */ + uint32_t generation; /* Bumped on every alloc; detects slot reuse */ }; #define WOLFIP_TOKEN_R (1u << 0) @@ -177,6 +178,7 @@ static void wolfip_drain_pipe_locked(struct wolfip_fd_entry *entry) } static struct wolfip_fd_entry wolfip_fd_entries[WOLFIP_MAX_PUBLIC_FDS]; +static uint32_t wolfip_fd_generation; /* Monotonic; bumped under wolfIP_mutex on each alloc */ static int tcp_entry_for_slot[MAX_TCPSOCKETS]; static int udp_entry_for_slot[MAX_UDPSOCKETS]; static int icmp_entry_for_slot[MAX_ICMPSOCKETS]; @@ -393,6 +395,7 @@ static int wolfip_fd_alloc(int internal_fd, int nonblock) } idx = pipefds[0]; memset(&wolfip_fd_entries[idx], 0, sizeof(wolfip_fd_entries[idx])); + wolfip_fd_entries[idx].generation = ++wolfip_fd_generation; wolfip_fd_entries[idx].internal_fd = internal_fd; wolfip_fd_entries[idx].public_fd = pipefds[0]; wolfip_fd_entries[idx].pipe_write = pipefds[1]; @@ -449,9 +452,15 @@ static int wolfip_wait_for_event_locked(struct wolfip_fd_entry *entry, short wai { struct pollfd pfd; char want; + uint32_t start_gen; + int start_fd; if (!entry) return -EINVAL; + /* Snapshot the slot identity so we can detect a concurrent close()/reuse + * across the mutex-drop window below. */ + start_gen = entry->generation; + start_fd = entry->internal_fd; want = (wait_events & POLLOUT) ? 'w' : 'r'; entry->events = (uint16_t)wait_events; wolfIP_register_callback(IPSTACK, entry->internal_fd, poller_callback, IPSTACK); @@ -481,6 +490,15 @@ static int wolfip_wait_for_event_locked(struct wolfip_fd_entry *entry, short wai return -EINTR; } pthread_mutex_lock(&wolfIP_mutex); + /* While the mutex was dropped a concurrent close() may have released + * this slot, and a subsequent socket()/accept() may have reused the + * same public fd for an unrelated connection. The caller still holds + * the stale internal_fd it captured before blocking; signal EBADF so + * it does not read/write the reused slot's data. */ + if (!entry->in_use || entry->generation != start_gen || + entry->internal_fd != start_fd) { + return -EBADF; + } if (poll_ret < 0) { return -errno; } From af09c73d215813188e5ae1b3973b0fb25c49b3fc Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Fri, 5 Jun 2026 16:10:20 +0200 Subject: [PATCH 08/13] F-5072: revalidate listener slot after accept poll to prevent fd-slot 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. --- src/port/posix/bsd_socket.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/port/posix/bsd_socket.c b/src/port/posix/bsd_socket.c index dc1df9d..0949b7d 100644 --- a/src/port/posix/bsd_socket.c +++ b/src/port/posix/bsd_socket.c @@ -1494,6 +1494,12 @@ static int wolfip_accept_common(int sockfd, struct sockaddr *addr, socklen_t *ad if (entry) { int internal_ret; int public_fd; + uint32_t start_gen; + int start_fd; + /* Snapshot the listener slot's identity so we can detect a concurrent + * close()/reuse across the mutex-drop window in the poll loop below. */ + start_gen = entry->generation; + start_fd = entry->internal_fd; if (!want_nonblock) want_nonblock = wolfip_fd_is_nonblock(sockfd); do { @@ -1512,8 +1518,15 @@ static int wolfip_accept_common(int sockfd, struct sockaddr *addr, socklen_t *ad pthread_mutex_unlock(&wolfIP_mutex); host_poll(&pfd, 1, -1); pthread_mutex_lock(&wolfIP_mutex); + /* While the mutex was dropped a concurrent close() may have + * released this slot, and a subsequent socket()/accept() may + * have reused the same public fd for an unrelated connection. + * The bare in_use check below cannot tell the slot apart from + * the original listener, so verify the snapshotted identity to + * avoid accepting on the wrong internal socket. */ entry = wolfip_entry_from_public(sockfd); - if (!entry) { + if (!entry || entry->generation != start_gen || + entry->internal_fd != start_fd) { errno = EBADF; pthread_mutex_unlock(&wolfIP_mutex); return -1; From 9f083140d6923735c9dd5f5b3b514dedaa414430 Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Fri, 5 Jun 2026 16:47:47 +0200 Subject: [PATCH 09/13] F-5259: derive HTTP body length from Content-Length to prevent CL.0 smuggling 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. --- Makefile | 9 ++ src/http/httpd.c | 65 +++++++++++++- src/test/test_http_smuggle.c | 167 +++++++++++++++++++++++++++++++++++ 3 files changed, 238 insertions(+), 3 deletions(-) create mode 100644 src/test/test_http_smuggle.c diff --git a/Makefile b/Makefile index d3e6dcd..f20f5a3 100644 --- a/Makefile +++ b/Makefile @@ -182,6 +182,7 @@ endif EXE=build/tcpecho build/tcp_netcat_poll build/tcp_netcat_select \ build/test-evloop build/test-dns build/test-wolfssl-forwarding \ build/test-ttl-expired build/test-wolfssl build/test-httpd \ + build/test-http-smuggle \ build/ipfilter-logger \ build/test-esp build/esp-server ifeq ($(UNAME_S),Linux) @@ -395,6 +396,14 @@ build/test-httpd: $(OBJ) build/test/test_httpd.o build/port/wolfssl_io.o build/c @echo "[LD] $@" @$(CC) $(CFLAGS) -o $@ $(BEGIN_GROUP) $(^) $(LDFLAGS) -lwolfssl $(END_GROUP) +# Standalone regression test for HTTP request framing (F-5259). It #includes +# httpd.c directly to reach the static parser and stubs the wolfIP/wolfSSL I/O. +build/test-http-smuggle:CFLAGS+=-Wno-cpp -DWOLFSSL_DEBUG -DWOLFSSL_WOLFIP -DWOLFIP_ENABLE_HTTP -Isrc/http +build/test-http-smuggle: src/test/test_http_smuggle.c src/http/httpd.c + @mkdir -p build || true + @echo "[LD] $@" + @$(CC) $(CFLAGS) -o $@ src/test/test_http_smuggle.c $(LDFLAGS) -lwolfssl + build/%.o: src/%.c @mkdir -p `dirname $@` || true @echo "[CC] $<" diff --git a/src/http/httpd.c b/src/http/httpd.c index bd1aabb..9dd20de 100644 --- a/src/http/httpd.c +++ b/src/http/httpd.c @@ -290,6 +290,26 @@ int http_url_encode(char *buf, size_t len, size_t max_len) { return len; } +/* Case-insensitive check that header line [s, s+len) begins with the field + * name `name` immediately followed by ':'. Returns a pointer to the value + * (past the colon and any leading spaces/tabs) on match, or NULL otherwise. */ +static const char *http_header_value(const char *s, size_t len, const char *name) { + size_t nl = strlen(name); + size_t i; + if (len < nl + 1) + return NULL; + for (i = 0; i < nl; i++) { + if (tolower((unsigned char)s[i]) != tolower((unsigned char)name[i])) + return NULL; + } + if (s[nl] != ':') + return NULL; + i = nl + 1; + while (i < len && (s[i] == ' ' || s[i] == '\t')) + i++; + return s + i; +} + static int parse_http_request(struct http_client *hc, uint8_t *buf, size_t len) { char *p = (char *) buf; char *end = p + len; @@ -297,6 +317,8 @@ static int parse_http_request(struct http_client *hc, uint8_t *buf, size_t len) size_t n; int ret; int decoded_len; + long content_length = -1; /* -1: no Content-Length header seen */ + int has_te = 0; /* Transfer-Encoding header present */ struct http_request req; struct http_url *url = NULL; memset(&req, 0, sizeof(struct http_request)); @@ -346,19 +368,56 @@ static int parse_http_request(struct http_client *hc, uint8_t *buf, size_t len) goto bad_request; n = q - p; if (n == 0) { + p = q + 2; /* Skip the blank line; body (if any) starts here */ break; /* End of headers */ } /* Enforce header maximum length */ if (n >= sizeof(req.headers)) goto bad_request; + /* Extract framing headers so the body length is derived from the + * declared Content-Length rather than from the recv buffer tail. */ + { + const char *v = http_header_value(p, n, "content-length"); + if (v) { + long cl = 0; + const char *d = v; + if (content_length >= 0) /* duplicate Content-Length */ + goto bad_request; + if (d >= q || *d < '0' || *d > '9') + goto bad_request; + while (d < q && *d >= '0' && *d <= '9') { + cl = cl * 10 + (*d - '0'); + if (cl > (long)sizeof(req.body)) /* too large / overflow */ + goto bad_request; + d++; + } + if (d != q) /* trailing garbage after the number */ + goto bad_request; + content_length = cl; + } else if (http_header_value(p, n, "transfer-encoding")) { + has_te = 1; + } + } /* Copy header and terminate */ memcpy(req.headers, p, n); req.headers[n] = '\0'; p = q + 2; } - /* Parse the body */ - if (p < end) { - n = end - p; + /* Parse the body. The body length is taken from the declared + * Content-Length; surplus bytes in the recv buffer are not part of this + * request. Transfer-Encoding (chunked) framing is not supported and a + * body present without a Content-Length is malformed - both are rejected + * to avoid request-smuggling (CL.0 / TE) ambiguity. */ + n = end - p; + if (has_te) + goto bad_request; + if (content_length >= 0) { + if ((size_t)content_length != n) + goto bad_request; + } else if (n > 0) { + goto bad_request; + } + if (n > 0) { if (n >= sizeof(req.body)) { return -1; } diff --git a/src/test/test_http_smuggle.c b/src/test/test_http_smuggle.c new file mode 100644 index 0000000..1f03fc4 --- /dev/null +++ b/src/test/test_http_smuggle.c @@ -0,0 +1,167 @@ +/* test_http_smuggle.c + * + * Copyright (C) 2024 wolfSSL Inc. + * + * This file is part of wolfIP TCP/IP stack. + * + * wolfIP is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * wolfIP is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335, USA + * + * + * Regression test for F-5259: parse_http_request must derive the body length + * from the declared Content-Length, not from the recv buffer tail. A request + * declaring Content-Length: 0 with a second request appended in the same + * segment (a CL.0 request-smuggling primitive) must be rejected instead of + * delivering the trailing bytes to the handler as a body. + */ + +#include +#include +#include + +/* Pull in the unit under test - parse_http_request is static. */ +#include "httpd.c" + +/* --- stubs for the wolfIP / wolfSSL symbols referenced by httpd.c --------- */ +int wolfIP_sock_socket(struct wolfIP *s, int d, int t, int p) +{ (void)s; (void)d; (void)t; (void)p; return -1; } +int wolfIP_sock_bind(struct wolfIP *s, int fd, const struct wolfIP_sockaddr *a, socklen_t l) +{ (void)s; (void)fd; (void)a; (void)l; return -1; } +int wolfIP_sock_listen(struct wolfIP *s, int fd, int b) +{ (void)s; (void)fd; (void)b; return -1; } +int wolfIP_sock_accept(struct wolfIP *s, int fd, struct wolfIP_sockaddr *a, socklen_t *l) +{ (void)s; (void)fd; (void)a; (void)l; return -1; } +int wolfIP_sock_send(struct wolfIP *s, int fd, const void *b, size_t l, int f) +{ (void)s; (void)fd; (void)b; (void)f; return (int)l; } +int wolfIP_sock_recv(struct wolfIP *s, int fd, void *b, size_t l, int f) +{ (void)s; (void)fd; (void)b; (void)l; (void)f; return -1; } +int wolfIP_sock_close(struct wolfIP *s, int fd) +{ (void)s; (void)fd; return 0; } +void wolfIP_register_callback(struct wolfIP *s, int fd, tsocket_cb cb, void *arg) +{ (void)s; (void)fd; (void)cb; (void)arg; } +int wolfSSL_SetIO_wolfIP(WOLFSSL *ssl, int fd) +{ (void)ssl; (void)fd; return 0; } +int wolfSSL_SetIO_wolfIP_CTX(WOLFSSL_CTX *ctx, struct wolfIP *s) +{ (void)ctx; (void)s; return 0; } + +/* --- test harness --------------------------------------------------------- */ +static int handler_calls; +static size_t handler_body_len; + +static int upload_handler(struct httpd *httpd, struct http_client *hc, struct http_request *req) +{ + (void)httpd; (void)hc; + handler_calls++; + handler_body_len = req->body_len; + return 0; +} + +static int run(struct httpd *httpd, const char *raw, size_t len) +{ + struct http_client hc; + memset(&hc, 0, sizeof(hc)); + hc.httpd = httpd; + hc.client_sd = 1; + hc.ssl = NULL; + handler_calls = 0; + handler_body_len = 0; + return parse_http_request(&hc, (uint8_t *)raw, len); +} + +#define CHECK(cond) do { if (!(cond)) { \ + printf("FAIL %s:%d: %s\n", __FILE__, __LINE__, #cond); failures++; } } while (0) + +int main(void) +{ + struct httpd httpd; + int failures = 0; + int r; + + memset(&httpd, 0, sizeof(httpd)); + httpd_register_handler(&httpd, "/upload", upload_handler); + + /* 1. CL.0 smuggling: declared Content-Length 0 but a whole second request + * appended in the same segment. Must be rejected; handler must not run + * and must never observe the smuggled bytes as a body. */ + { + const char *req = + "POST /upload HTTP/1.1\r\n" + "Host: victim.local\r\n" + "Content-Length: 0\r\n" + "\r\n" + "GET /admin HTTP/1.1\r\n" + "Host: victim.local\r\n" + "X-Evil: injected\r\n" + "\r\n"; + r = run(&httpd, req, strlen(req)); + CHECK(r < 0); /* rejected as bad request */ + CHECK(handler_calls == 0); /* handler never reached */ + } + + /* 2. Well-formed POST: body matches Content-Length, delivered verbatim. */ + { + const char *req = + "POST /upload HTTP/1.1\r\n" + "Host: victim.local\r\n" + "Content-Length: 5\r\n" + "\r\n" + "hello"; + r = run(&httpd, req, strlen(req)); + CHECK(r == 0); + CHECK(handler_calls == 1); + CHECK(handler_body_len == 5); + } + + /* 3. Body larger than Content-Length (CL shorter than data) is rejected. */ + { + const char *req = + "POST /upload HTTP/1.1\r\n" + "Content-Length: 2\r\n" + "\r\n" + "hello"; + r = run(&httpd, req, strlen(req)); + CHECK(r < 0); + CHECK(handler_calls == 0); + } + + /* 4. Body present without Content-Length is malformed and rejected. */ + { + const char *req = + "POST /upload HTTP/1.1\r\n" + "Host: victim.local\r\n" + "\r\n" + "smuggled"; + r = run(&httpd, req, strlen(req)); + CHECK(r < 0); + CHECK(handler_calls == 0); + } + + /* 5. Plain GET with no body still works. */ + { + const char *req = + "GET /upload HTTP/1.1\r\n" + "Host: victim.local\r\n" + "\r\n"; + r = run(&httpd, req, strlen(req)); + CHECK(r == 0); + CHECK(handler_calls == 1); + CHECK(handler_body_len == 0); + } + + if (failures == 0) + printf("test_http_smuggle: all checks passed\n"); + else + printf("test_http_smuggle: %d check(s) failed\n", failures); + return failures ? 1 : 0; +} From f3cc1912fb5b807dac61132bebfa45f13358208c Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Fri, 5 Jun 2026 16:51:07 +0200 Subject: [PATCH 10/13] F-5258: stop httpd_get_request_arg at NUL to prevent OOB read past query/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. --- Makefile | 9 ++- src/http/httpd.c | 2 + src/test/test_http_arg_oob.c | 135 +++++++++++++++++++++++++++++++++++ 3 files changed, 145 insertions(+), 1 deletion(-) create mode 100644 src/test/test_http_arg_oob.c diff --git a/Makefile b/Makefile index f20f5a3..e306fb7 100644 --- a/Makefile +++ b/Makefile @@ -182,7 +182,7 @@ endif EXE=build/tcpecho build/tcp_netcat_poll build/tcp_netcat_select \ build/test-evloop build/test-dns build/test-wolfssl-forwarding \ build/test-ttl-expired build/test-wolfssl build/test-httpd \ - build/test-http-smuggle \ + build/test-http-smuggle build/test-http-arg-oob \ build/ipfilter-logger \ build/test-esp build/esp-server ifeq ($(UNAME_S),Linux) @@ -404,6 +404,13 @@ build/test-http-smuggle: src/test/test_http_smuggle.c src/http/httpd.c @echo "[LD] $@" @$(CC) $(CFLAGS) -o $@ src/test/test_http_smuggle.c $(LDFLAGS) -lwolfssl +# Standalone regression test for the httpd_get_request_arg OOB read (F-5258). +build/test-http-arg-oob:CFLAGS+=-Wno-cpp -DWOLFSSL_DEBUG -DWOLFSSL_WOLFIP -DWOLFIP_ENABLE_HTTP -Isrc/http +build/test-http-arg-oob: src/test/test_http_arg_oob.c src/http/httpd.c + @mkdir -p build || true + @echo "[LD] $@" + @$(CC) $(CFLAGS) -o $@ src/test/test_http_arg_oob.c $(LDFLAGS) -lwolfssl + build/%.o: src/%.c @mkdir -p `dirname $@` || true @echo "[CC] $<" diff --git a/src/http/httpd.c b/src/http/httpd.c index 9dd20de..19b36e7 100644 --- a/src/http/httpd.c +++ b/src/http/httpd.c @@ -560,6 +560,8 @@ int httpd_get_request_arg(struct http_request *req, const char *name, char *valu return 0; } } + if (*q == '\0') // Reached the terminator: do not step past the buffer + break; p = q + 1; // Move to next key-value pair } return -1; // Key not found diff --git a/src/test/test_http_arg_oob.c b/src/test/test_http_arg_oob.c new file mode 100644 index 0000000..e037c96 --- /dev/null +++ b/src/test/test_http_arg_oob.c @@ -0,0 +1,135 @@ +/* test_http_arg_oob.c + * + * Copyright (C) 2024 wolfSSL Inc. + * + * This file is part of wolfIP TCP/IP stack. + * + * wolfIP is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * wolfIP is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335, USA + * + * + * Regression test for F-5258: httpd_get_request_arg must not walk past the end + * of the query/body buffer. When the buffer is exactly full the terminating + * NUL lands on the last byte of the array; the old "p = q + 1" idiom then + * advanced one byte past the array into the adjacent struct member (headers for + * GET, body_len for POST), causing it to interpret that memory as further + * key=value pairs. The parser must instead stop at the NUL and report the key + * as not found. + */ + +#include +#include +#include + +/* Pull in the unit under test. */ +#include "httpd.c" + +/* --- stubs for the wolfIP / wolfSSL symbols referenced by httpd.c --------- */ +int wolfIP_sock_socket(struct wolfIP *s, int d, int t, int p) +{ (void)s; (void)d; (void)t; (void)p; return -1; } +int wolfIP_sock_bind(struct wolfIP *s, int fd, const struct wolfIP_sockaddr *a, socklen_t l) +{ (void)s; (void)fd; (void)a; (void)l; return -1; } +int wolfIP_sock_listen(struct wolfIP *s, int fd, int b) +{ (void)s; (void)fd; (void)b; return -1; } +int wolfIP_sock_accept(struct wolfIP *s, int fd, struct wolfIP_sockaddr *a, socklen_t *l) +{ (void)s; (void)fd; (void)a; (void)l; return -1; } +int wolfIP_sock_send(struct wolfIP *s, int fd, const void *b, size_t l, int f) +{ (void)s; (void)fd; (void)b; (void)f; return (int)l; } +int wolfIP_sock_recv(struct wolfIP *s, int fd, void *b, size_t l, int f) +{ (void)s; (void)fd; (void)b; (void)l; (void)f; return -1; } +int wolfIP_sock_close(struct wolfIP *s, int fd) +{ (void)s; (void)fd; return 0; } +void wolfIP_register_callback(struct wolfIP *s, int fd, tsocket_cb cb, void *arg) +{ (void)s; (void)fd; (void)cb; (void)arg; } +int wolfSSL_SetIO_wolfIP(WOLFSSL *ssl, int fd) +{ (void)ssl; (void)fd; return 0; } +int wolfSSL_SetIO_wolfIP_CTX(WOLFSSL_CTX *ctx, struct wolfIP *s) +{ (void)ctx; (void)s; return 0; } + +#define CHECK(cond) do { if (!(cond)) { \ + printf("FAIL %s:%d: %s\n", __FILE__, __LINE__, #cond); failures++; } } while (0) + +int main(void) +{ + int failures = 0; + char value[64]; + int r; + + /* 1. GET with a query that exactly fills the buffer (255 bytes, NUL at + * query[HTTP_QUERY_LEN-1]) and no '=' anywhere. A header-shaped + * "target=INJECTED" string sits in the adjacent headers field. The key + * must NOT be found: the parser must stop at the NUL rather than reading + * headers as a continuation of the query. */ + { + struct http_request req; + memset(&req, 0, sizeof(req)); + strcpy(req.method, "GET"); + memset(req.query, 'A', HTTP_QUERY_LEN - 1); + req.query[HTTP_QUERY_LEN - 1] = '\0'; + strcpy(req.headers, "target=INJECTED"); + + memset(value, 0, sizeof(value)); + r = httpd_get_request_arg(&req, "target", value, sizeof(value)); + CHECK(r == -1); /* key not found */ + CHECK(value[0] == '\0'); /* nothing copied out of headers */ + } + + /* 2. POST with a body that exactly fills the buffer (1023 bytes, NUL at + * body[HTTP_BODY_LEN-1]) and no '='. Must not walk into body_len. */ + { + struct http_request req; + memset(&req, 0, sizeof(req)); + strcpy(req.method, "POST"); + memset(req.body, 'B', HTTP_BODY_LEN - 1); + req.body[HTTP_BODY_LEN - 1] = '\0'; + req.body_len = HTTP_BODY_LEN - 1; + + memset(value, 0, sizeof(value)); + r = httpd_get_request_arg(&req, "x", value, sizeof(value)); + CHECK(r == -1); + CHECK(value[0] == '\0'); + } + + /* 3. Normal lookups still work: a real query argument is returned. */ + { + struct http_request req; + memset(&req, 0, sizeof(req)); + strcpy(req.method, "GET"); + strcpy(req.query, "a=1&target=ok&b=2"); + + memset(value, 0, sizeof(value)); + r = httpd_get_request_arg(&req, "target", value, sizeof(value)); + CHECK(r == 0); + CHECK(strcmp(value, "ok") == 0); + } + + /* 4. Last argument (no trailing '&') still resolves correctly. */ + { + struct http_request req; + memset(&req, 0, sizeof(req)); + strcpy(req.method, "GET"); + strcpy(req.query, "a=1&last=end"); + + memset(value, 0, sizeof(value)); + r = httpd_get_request_arg(&req, "last", value, sizeof(value)); + CHECK(r == 0); + CHECK(strcmp(value, "end") == 0); + } + + if (failures == 0) + printf("test_http_arg_oob: all checks passed\n"); + else + printf("test_http_arg_oob: %d check(s) failed\n", failures); + return failures ? 1 : 0; +} From 285437e32cbba564cc8e153757ac50765b48f50d Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Fri, 5 Jun 2026 17:08:56 +0200 Subject: [PATCH 11/13] F-5012: hold DNS wait slot until reaped to prevent concurrent getaddrinfo 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. --- src/port/posix/bsd_socket.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/port/posix/bsd_socket.c b/src/port/posix/bsd_socket.c index 0949b7d..990cb6c 100644 --- a/src/port/posix/bsd_socket.c +++ b/src/port/posix/bsd_socket.c @@ -198,6 +198,7 @@ enum wolfip_dns_wait_type { struct wolfip_dns_wait_ctx { pthread_mutex_t mutex; pthread_cond_t cond; + int busy; int pending; enum wolfip_dns_wait_type type; int status; @@ -209,6 +210,7 @@ static struct wolfip_dns_wait_ctx dns_wait_ctx = { PTHREAD_MUTEX_INITIALIZER, PTHREAD_COND_INITIALIZER, 0, + 0, DNS_WAIT_NONE, 0, 0, @@ -733,10 +735,11 @@ static int wolfip_dns_error_to_eai(int err) static int wolfip_dns_begin_wait(enum wolfip_dns_wait_type type) { pthread_mutex_lock(&dns_wait_ctx.mutex); - if (dns_wait_ctx.pending) { + if (dns_wait_ctx.busy) { pthread_mutex_unlock(&dns_wait_ctx.mutex); return EAI_AGAIN; } + dns_wait_ctx.busy = 1; dns_wait_ctx.pending = 1; dns_wait_ctx.type = type; dns_wait_ctx.status = EAI_FAIL; @@ -749,6 +752,7 @@ static void wolfip_dns_abort_wait(int status) { pthread_mutex_lock(&dns_wait_ctx.mutex); dns_wait_ctx.pending = 0; + dns_wait_ctx.busy = 0; dns_wait_ctx.type = DNS_WAIT_NONE; dns_wait_ctx.status = status; pthread_cond_signal(&dns_wait_ctx.cond); @@ -766,6 +770,7 @@ static int wolfip_dns_wait(enum wolfip_dns_wait_type type, uint32_t *ip_out, cha int err = pthread_cond_timedwait(&dns_wait_ctx.cond, &dns_wait_ctx.mutex, &ts); if (err == ETIMEDOUT) { dns_wait_ctx.pending = 0; + dns_wait_ctx.busy = 0; dns_wait_ctx.type = DNS_WAIT_NONE; pthread_mutex_unlock(&dns_wait_ctx.mutex); return EAI_AGAIN; @@ -773,6 +778,8 @@ static int wolfip_dns_wait(enum wolfip_dns_wait_type type, uint32_t *ip_out, cha } if (dns_wait_ctx.type != type) { int status = dns_wait_ctx.status ? dns_wait_ctx.status : EAI_FAIL; + dns_wait_ctx.type = DNS_WAIT_NONE; + dns_wait_ctx.busy = 0; pthread_mutex_unlock(&dns_wait_ctx.mutex); return status; } @@ -784,6 +791,7 @@ static int wolfip_dns_wait(enum wolfip_dns_wait_type type, uint32_t *ip_out, cha wolfip_strlcpy(name_out, dns_wait_ctx.name, name_len); } dns_wait_ctx.type = DNS_WAIT_NONE; + dns_wait_ctx.busy = 0; pthread_mutex_unlock(&dns_wait_ctx.mutex); return status; } From e335905da5143dde4834301892459948c496fe78 Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Fri, 5 Jun 2026 17:20:00 +0200 Subject: [PATCH 12/13] F-4950: negate wolfIP error code when setting errno in POSIX shim macros 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. --- Makefile | 11 ++++++ src/port/posix/bsd_socket.c | 6 +-- src/test/test_posix_errno.c | 73 +++++++++++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 3 deletions(-) create mode 100644 src/test/test_posix_errno.c diff --git a/Makefile b/Makefile index e306fb7..673d01f 100644 --- a/Makefile +++ b/Makefile @@ -332,6 +332,17 @@ build/packet_ping: $(OBJ) build/port/posix/bsd_socket.o build/test/packet_ping.o @echo "[LD] $@" @$(CC) $(CFLAGS) -o $@ $(BEGIN_GROUP) $(^) $(LDFLAGS) $(END_GROUP) +# F-4950 regression: test_posix_errno.c #includes bsd_socket.c directly, so the +# shim object must not be linked again here. +build/test-posix-errno: $(OBJ) build/test/test_posix_errno.o + @echo "[LD] $@" + @$(CC) $(CFLAGS) -o $@ $(BEGIN_GROUP) $(^) $(LDFLAGS) $(END_GROUP) + +.PHONY: posix-errno-test +posix-errno-test: build/test-posix-errno + @echo "[RUN] $<" + @./build/test-posix-errno + build/test-wolfssl:CFLAGS+=-Wno-cpp -DWOLFSSL_DEBUG -DWOLFSSL_WOLFIP build/test-httpd:CFLAGS+=-Wno-cpp -DWOLFSSL_DEBUG -DWOLFSSL_WOLFIP -Isrc/http diff --git a/src/port/posix/bsd_socket.c b/src/port/posix/bsd_socket.c index 990cb6c..a09632e 100644 --- a/src/port/posix/bsd_socket.c +++ b/src/port/posix/bsd_socket.c @@ -538,7 +538,7 @@ static int wolfip_wait_for_event_locked(struct wolfip_fd_entry *entry, short wai if (__wolfip_internal >= 0) { \ int __wolfip_retval = wolfIP_sock_##call(IPSTACK, __wolfip_internal, ## __VA_ARGS__); \ if (__wolfip_retval < 0) { \ - errno = __wolfip_retval; \ + errno = -__wolfip_retval; \ pthread_mutex_unlock(&wolfIP_mutex); \ return -1; \ } \ @@ -585,7 +585,7 @@ static int wolfip_wait_for_event_locked(struct wolfip_fd_entry *entry, short wai } \ } while (__wolfip_retval == -EAGAIN); \ if (__wolfip_retval < 0) { \ - errno = __wolfip_retval; \ + errno = -__wolfip_retval; \ pthread_mutex_unlock(&wolfIP_mutex); \ return -1; \ } \ @@ -1544,7 +1544,7 @@ static int wolfip_accept_common(int sockfd, struct sockaddr *addr, socklen_t *ad } } while (internal_ret == -EAGAIN); if (internal_ret < 0) { - errno = internal_ret; + errno = -internal_ret; pthread_mutex_unlock(&wolfIP_mutex); return -1; } diff --git a/src/test/test_posix_errno.c b/src/test/test_posix_errno.c new file mode 100644 index 0000000..60d81ed --- /dev/null +++ b/src/test/test_posix_errno.c @@ -0,0 +1,73 @@ +/* test_posix_errno.c + * + * Copyright (C) 2024 wolfSSL Inc. + * + * This file is part of wolfIP TCP/IP stack. + * + * wolfIP is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * wolfIP is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335, USA + */ + +/* Regression test for F-4950: the POSIX shim macros must translate a negative + * wolfIP return code into a positive errno, exactly as the hand-written paths + * (bind/close/send/...) already do. Driving getpeername() through the + * conditional_steal_call() macro on a wolfIP-managed fd exercises the + * "errno = -ret" conversion: with the sign bug present errno is set to the raw + * negative code and the assertion below fails. */ + +#include +#include +#include +#include +#include + +/* Pull in the shim itself so we can reach its file-static state + * (in_the_stack / IPSTACK). The library constructor runs at load; without + * CAP_NET_ADMIN its TAP setup simply fails and returns, leaving the host_* + * passthrough pointers populated, which is all this test needs. */ +#include "../port/posix/bsd_socket.c" + +int main(void) +{ + struct sockaddr_in peer; + socklen_t peerlen = 0; /* deliberately too small -> internal error path */ + int fd; + int ret; + + /* The library constructor already initialised the static stack via + * wolfIP_init_static(); make sure it is present and take over the shim + * path so socket()/getpeername() are served by wolfIP, not libc. */ + if (!IPSTACK) + wolfIP_init_static(&IPSTACK); + in_the_stack = 0; + + fd = socket(AF_INET, SOCK_STREAM, 0); + assert(fd >= 0); + + errno = 0; + memset(&peer, 0, sizeof(peer)); + ret = getpeername(fd, (struct sockaddr *)&peer, &peerlen); + + printf("getpeername ret=%d errno=%d (%s)\n", ret, errno, + errno > 0 ? strerror(errno) : "negative/raw"); + + /* The call must fail... */ + assert(ret == -1); + /* ...and errno must be a real positive errno value, never the raw negative + * wolfIP code (the F-4950 defect). */ + assert(errno > 0); + + printf("F-4950 regression test passed\n"); + return 0; +} From 7a11ba805dd895ee060b842f1aa8f6ee97fcaa0d Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Fri, 5 Jun 2026 18:00:59 +0200 Subject: [PATCH 13/13] Addressed Copilot's comments --- Makefile | 1 + src/test/test_http_smuggle.c | 9 ++++++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 673d01f..f19704e 100644 --- a/Makefile +++ b/Makefile @@ -183,6 +183,7 @@ EXE=build/tcpecho build/tcp_netcat_poll build/tcp_netcat_select \ build/test-evloop build/test-dns build/test-wolfssl-forwarding \ build/test-ttl-expired build/test-wolfssl build/test-httpd \ build/test-http-smuggle build/test-http-arg-oob \ + build/test-posix-errno \ build/ipfilter-logger \ build/test-esp build/esp-server ifeq ($(UNAME_S),Linux) diff --git a/src/test/test_http_smuggle.c b/src/test/test_http_smuggle.c index 1f03fc4..3ec238c 100644 --- a/src/test/test_http_smuggle.c +++ b/src/test/test_http_smuggle.c @@ -70,13 +70,20 @@ static int upload_handler(struct httpd *httpd, struct http_client *hc, struct ht static int run(struct httpd *httpd, const char *raw, size_t len) { struct http_client hc; + /* Copy into a writable scratch buffer that mirrors the production recv + * buffer, so the test never hands a read-only string literal to the + * parser - safe even if parse_http_request ever normalizes in-place. */ + uint8_t buf[HTTP_RECV_BUF_LEN]; + if (len > sizeof(buf)) + len = sizeof(buf); + memcpy(buf, raw, len); memset(&hc, 0, sizeof(hc)); hc.httpd = httpd; hc.client_sd = 1; hc.ssl = NULL; handler_calls = 0; handler_body_len = 0; - return parse_http_request(&hc, (uint8_t *)raw, len); + return parse_http_request(&hc, buf, len); } #define CHECK(cond) do { if (!(cond)) { \