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

[Geert Stappers]

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