On 3/1/25 23:59, Simon Kelley wrote:
On 28/02/2025 06:47, Helge Deller wrote:
* Helge Deller <del...@gmx.de>:
On 2/27/25 23:59, Helge Deller wrote:
* Helge Deller <del...@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 <del...@gmx.de>
Reported-by: Dominik Derigs <dl...@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
_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
