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

[Helge Deller]

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.

Helge


>
> diff --git a/src/tftp.c b/src/tftp.c
> index 637a566..fef4e77 100644
> --- a/src/tftp.c
> +++ b/src/tftp.c
> @@ -360,8 +360,8 @@ 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)) ||
>         !(mode = next(&p, end)) ||
> @@ -743,14 +743,19 @@ static void free_transfer(struct tftp_transfer *transfer)
>   static char *next(char **p, char *end)
>   {
>     char *ret = *p;
> +  char *n;
>     size_t len;
>
> -  if (*(end-1) != 0 ||
> -      *p == end ||
> +  if (*p == end ||
>         (len = strlen(ret)) == 0)
>       return NULL;
>
> -  *p += len + 1;
> +  /* check that string ends inside packet */
> +  n = *p + len + 1;
> +  if (n > end)
> +	return NULL;
> +
> +  *p = n;
>     return ret;
>   }
>
>
>