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

[Helge Deller]

On 3/1/25 23:59, Simon Kelley wrote:
> 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.

Good!

> 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

I'm absolutely fine with your rewrite, and I've just
successfully tested your patch (I tested git head).
My HP C8000 still boots with this change.

So, thank you for fixing it!

Helge