[Dnsmasq-discuss] [PATCH] TFTP off-by-2 bugfix - CRASH

[Helge Deller]

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 <deller at gmx.de>
>> Reported-by: Dominik Derigs <dl6er at 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 at 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 at 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