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
|$
Groeten
Geert Stappers
--
Silence is hard to parse
_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
