On 3/1/25 17:49, Geert Stappers wrote:
On Fri, Feb 28, 2025 at 07:47:08AM +0100, Helge Deller via Dnsmasq-discuss wrote:Here is an updated patch. Please verify, and Simon, please install if you think it's Ok. Helge -------------------- [PATCH] Fix TFTP loading from real-world HP workstation This patch reverts commit 368ceff6e099 ("TFTP off-by-2 bugfix") which wrongly assumed there was an off-by-two issue in the packet length. Instead it turns out, that the internal funtion next(), which extracts a NUL-terminated string starting from the current packet position is buggy. This function checks the very last byte of the packet if it is zero in order to prevent strings reaching outside beyond the packet. On my machine (HP C8000 workstation), the initial packet is 516 bytes long and there is no requirement that the last byte is zero, as long as all strings are properly NUL-terminated and all are stored fully inside the packet (way earlier than reaching the last byte of the packet). This patch fixes the next() function, so that it searches for the terminating NUL-byte starting from the current position and avoids accessing any memory outside of the packet. For that it avoids using strlen() and instead uses a simple loop. Tested with my HP C8000 workstation, and via trivial get command. Signed-off-by: Helge Deller <del...@gmx.de> Reported-by: Dominik Derigs <dl...@dl6er.de> diff --git a/src/tftp.c b/src/tftp.c index 637a566..8420652 100644 --- a/src/tftp.c +++ b/src/tftp.c @@ -360,7 +360,7 @@ void tftp_request(struct listener *listen, time_t now) } p = packet + 2; - end = packet + 2 + len; + end = packet + len; if (ntohs(*((unsigned short *)packet)) != OP_RRQ || !(filename = next(&p, end)) || @@ -743,15 +743,21 @@ static void free_transfer(struct tftp_transfer *transfer) static char *next(char **p, char *end) { char *ret = *p; - size_t len; - - if (*(end-1) != 0 || - *p == end || - (len = strlen(ret)) == 0) - return NULL; - - *p += len + 1; - return ret; + char *n = *p; + + /* as long as end of packet not reached: */ + while (n < end) { + if (*n) { + /* contiue while terminating NUL char not found yet */ + ++n; + continue; + } + /* NUL char found. Update start pointer and return start of string. */ + *p = n + 1; + return ret; + } + /* end of packet reached, but NUL-terminated string not found: */ + return NULL; } static void sanitise(char *buf)|stappers@alpaca:~/src/dnsmasq |$ patch -p1 < /tmp/helge |patching file src/tftp.c |Hunk #1 FAILED at 360. |Hunk #2 FAILED at 743. |2 out of 2 hunks FAILED -- saving rejects to file src/tftp.c.rej |stappers@alpaca:~/src/dnsmasq
My stupid mail provider (GMX) started a few months back to modify spaces/indenting in my emails. Probably some kind of virus scanner thing. Sorry. Helge _______________________________________________ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
