On Sat, Mar 01, 2025 at 10:59:56PM +0000, Simon Kelley wrote:
> On 28/02/2025 06:47, Helge Deller wrote:
> > Helge Deller <del...@gmx.de>:
> > > ....
> > > 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.
>
> 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
>
With that change inplace, I was able to do "tftp".
(with previous versiontest on TFTP failed)
Something else:
For getting git tag v2.91rc6 visible, is a `git push --tags` needed.
> Cheers,
> Simon.
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
