diff --git a/Makefile b/Makefile index d3e6dcd1..f19704e6 100644 --- a/Makefile +++ b/Makefile @@ -182,6 +182,8 @@ 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-arg-oob \ + build/test-posix-errno \ build/ipfilter-logger \ build/test-esp build/esp-server ifeq ($(UNAME_S),Linux) @@ -331,6 +333,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 @@ -395,6 +408,21 @@ 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 + +# 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 bd1aabb0..19b36e7f 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; } @@ -501,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/port/posix/bsd_socket.c b/src/port/posix/bsd_socket.c index 9d8b03d4..a09632e9 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]; @@ -196,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; @@ -207,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, @@ -393,6 +397,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 +454,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 +492,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; } @@ -518,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; \ } \ @@ -565,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; \ } \ @@ -715,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; @@ -731,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); @@ -748,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; @@ -755,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; } @@ -766,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; } @@ -1476,6 +1502,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 { @@ -1494,8 +1526,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; @@ -1505,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_http_arg_oob.c b/src/test/test_http_arg_oob.c new file mode 100644 index 00000000..e037c965 --- /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; +} diff --git a/src/test/test_http_smuggle.c b/src/test/test_http_smuggle.c new file mode 100644 index 00000000..3ec238cd --- /dev/null +++ b/src/test/test_http_smuggle.c @@ -0,0 +1,174 @@ +/* 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; + /* 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, buf, 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; +} diff --git a/src/test/test_posix_errno.c b/src/test/test_posix_errno.c new file mode 100644 index 00000000..60d81ed6 --- /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; +} diff --git a/src/test/unit/unit_tests_tftp.c b/src/test/unit/unit_tests_tftp.c index fb860528..ee8039d7 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 @@ -670,8 +672,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 +686,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; @@ -777,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; @@ -860,7 +951,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. */ @@ -868,7 +959,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); } @@ -1093,6 +1184,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; @@ -1462,14 +1631,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); } } @@ -1595,7 +1764,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. */ @@ -1603,7 +1772,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. */ @@ -1611,7 +1780,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. */ @@ -1619,7 +1788,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). @@ -1638,7 +1809,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); @@ -1743,6 +1915,108 @@ 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 + * 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 @@ -2043,6 +2317,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 @@ -2108,8 +2434,10 @@ 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_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); @@ -2117,6 +2445,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, @@ -2129,12 +2459,15 @@ 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); + 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); 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 a43723c8..aa7d9ab8 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; @@ -645,8 +657,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); @@ -747,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; @@ -873,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) { @@ -921,6 +949,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) @@ -979,6 +1020,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); @@ -1049,8 +1101,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); @@ -1130,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); }