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 <del...@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)) ||
_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
