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

[Helge Deller]

Hi Dominik,

On 2/27/25 12:51, Dominik Derigs via Dnsmasq-discuss wrote:
> this patch, part of the current rc5 breaks TFTP for me. IMO we are
> now reading *beyond* the end of the received packet. I was able to
> trace this down to this patch I am replying to being the cause.
> Bisecting successfully restores normal operation before and failure
> after this patch.
>
> 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.
I'll do some more testing.

Thanks!
Helge
  > Best,
> Dominik
>
> Am 2/6/25 um 6:50 PM schrieb Simon Kelley:
>> Great, thanks. patch applied and rc2 tagged.
>>
>>
>> Cheers,
>>
>> Simon.
>>
>>
>>
>>
>> On 2/6/25 12:02, Helge Deller wrote:
>>> Some of my PA-RISC UNIX machines boot remotely via tftp, but dnsmasq
>>> randomly fails to deliver (the identical file) to some of the machines.
>>>
>>> I traced the issue and basically dnsmasq fails with error "unsupported
>>> request from IP.x.y.z" (line 366 in tftp.c).
>>>
>>> Here is an example package which is sent (516 hex bytes):
>>> 76 6d 6c 69 6e 75 78 00 6f 63 74 65 74 00 12 74 10 3c 00 00 00 00 00 01
>>> a9 24 00 00 00 00 00 00 1e 38 00 00 00 00 00 00 1c a0 00 00 00 00 00 00
>>> 1d 08 00 00 00 00 00 00 1d 28 00 00 00 00 00 00 08 00 00 00 00 00 00 00
>>> 03 d8 00 00 00 00 00 00 00 00 00 00 00 00 00 00 1d 30 00 00 00 02 ff e0
>>> 00 00 00 00 03 60 a8 49 55 93 00 00 00 01 f0 d4 21 e4 00 00 00 00 00 00
>>> 1d 78 00 00 00 f0 f0 d8 51 38 00 00 00 f0 f0 d4 21 c0 00 00 00 00 00 00
>>> 00 00 00 00 00 00 00 01 aa b8 00 00 00 f0 f0 e9 62 7c 00 00 00 00 00 00
>>> 03 01 ff ff ff ff ff ff 03 00 ff ff ff ff ff ff ff ff 00 00 00 00 00 00
>>> 00 03 00 00 00 00 00 00 00 05 00 00 00 00 00 00 00 04 ff ff ff ff ff ff
>>> ff ff 00 00 00 00 00 00 00 00 ff ff ff ff ff ff ff ff 00 00 00 00 00 00
>>> 00 05 00 00 00 00 00 00 1e 38 00 00 00 00 00 00 00 60 00 00 00 00 00 01
>>> a6 68 00 00 00 00 00 00 00 03 00 00 00 00 00 00 00 ff 00 00 00 00 00 00
>>> 00 03 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 02 00 00 00 00 00 00
>>> 00 00 00 00 00 f0 f0 d8 4f 30 00 00 00 00 00 00 00 01 00 00 00 00 00 00
>>> 00 00 00 00 00 00 00 01 ae ec 00 00 00 00 00 00 1f 70 00 00 00 00 00 00
>>> 1e b8 00 00 03 60 a8 49 55 93 00 00 00 02 18 71 1a 00 00 00 00 00 00 00
>>> 00 03 00 00 00 00 00 00 00 03 00 00 00 00 00 00 1e 38 00 00 00 00 00 00
>>> 00 07 00 00 00 00 00 00 00 00 00 00 00 f0 f0 d2 f0 70 00 00 00 00 00 00
>>> 1f c0 00 00 00 f0 f0 d4 0b e8 00 00 00 00 00 00 00 01 00 00 00 00 00 00
>>> 00 60 ff ff ff fc 00 60 18 00 00 00 00 00 00 00 00 00 00 00 00 f0 f0 d8
>>> 8f d0 00 00 00 00 00 00 1f f8 00 00 00 00 00 00 00 00 00 00 00 f0 f0 d8
>>> 8d b8 00 00 00 00 00 00 1e e8 00 00
>>>
>>> Please note the last 3 bytes: "e8 00 00".
>>> If the 3rd last byte is "00", then dnsmasq works and it fails it it's "e8".
>>>
>>> So, the bug is in line 366 of tftp.c:
>>>     filename = next(&p, end)
>>> Here filename gets the value NULL from next(), because the "end" variable is off-by-2.
>>> The fix is to change line 363 to add an offset of 2:
>>>    end = packet + 2 + len;
>>>
>>> Signed-off-by: Helge Deller <deller at gmx.de>
>>> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2293793
>>>
>>> diff --git a/src/tftp.c b/src/tftp.c
>>> index 831d2f2..637a566 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 + len;
>>> +  end = packet + 2 + len;
>>>         if (ntohs(*((unsigned short *)packet)) != OP_RRQ ||
>>>         !(filename = next(&p, end)) ||