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

[Simon Kelley]

On 28/02/2025 06:47, Helge Deller wrote:
> * Helge Deller <deller at gmx.de>:
>> On 2/27/25 23:59, Helge Deller wrote:
>>> * Helge Deller <deller at gmx.de>:
>>>>> How to reproduce:
>>>>>
>>>>> sudo mkdir -p /srv/tftp && sudo touch /srv/tftp/abc
>>>>> sudo src/dnsmasq -d -q --port=0 --enable-tftp --tftp-root=/srv/tftp
>>>>>
>>>>> then run:
>>>>>
>>>>> atftp localhost
>>>>> get abc
>>>>>
>>>>> This sends the following packet:
>>>>>
>>>>> 00 01 61 62 63 00 6F 63 74 65 74 00
>>>>>
>>>>> len is 12 which is correct. We just start at p = packet  + 2 as this
>>>>> is where the interesting stuff (the filename) starts after the TFTP
>>>>> opcode. Said patch sets end to 14 which is two bytes past the data.
>>>>> When next() tries to access *end to check if it is zero, it may fail
>>>>> as random data (and not 0) is found there. I don't know why this
>>>>> patch fixed the issue for Helge but to me this looks like a reading
>>>>> out of bounds in any case.
>>>>
>>>> Seems you are right.
>>>>
>>>>> Maybe he was just lucky to have a legit
>>>>> zero byte two bytes beyond the end of packet. I may totally be on
>>>>> the wrong track here but this looks quite obviously wrong to me and
>>>>> reverting the patch is an instant fix for me.
>>>>
>>>> I think next() is doing something wrong, as it failed with
>>>> "unsupported request from XYZ" for me.
>>>
>>> The next() function checks if the packet ends with a 0-byte.
>>> This check is wrong, as any bytes could be beyond the null-terminated
>>> filename and the null-terminated mode string.
>>> On my hppa box the initial packet is 516 bytes long and seems to be
>>> filled with random data.
>>>
>>> This patch reverts my previous patch and fixes the check in the next() function.
>>> Could you please test it as well?
>>
>> The patch below works for me.
>> Still, it will need more teaking to avoid accessing memory
>> outside of the packet (which may happen in strlen()).
>> I'll send a new patch tomorrow.
> 
> 
> 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)
> 
> 
> 


Completely agree with your analysis, the real problem is next() which 
probably fails when the packet has padding past the end of the expected 
data.

I took the liberty of re-writing your patch to suit my taste, and to 
retain the existing behaviour of returning NULL when it finds a 
zero-length string. Otherwise, it does the same as your new code, unless 
I've
made a mistake.

https://thekelleys.org.uk/gitweb/?p=dnsmasq.git;a=commit;h=9df1bd0cc1580fe3d2dd2c0fd21050e528d178c5


Cheers,

Simon.