On Thu, Feb 27, 2025 at 12:51:48PM +0100, Dominik Derigs wrote:
> Hey Simon,
>
> 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
$ echo -e \x61\x62\x63 null \x6F\x63\x74\x65\x74 null
abc null octet null
$
> 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. 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.
With the revert, with
diff --git a/src/tftp.c b/src/tftp.c
index 637a566..831d2f2 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)) ||
I can avoid 'dnsmasq-tftp[23533]: unsupported request from 127.0.0.1'
> Best,
> Dominik
Groeten
Geert Stappers
--
Silence is hard to parse
_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
