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

[Geert Stappers]

On Sat, Mar 01, 2025 at 10:59:56PM +0000, Simon Kelley wrote:
> On 28/02/2025 06:47, Helge Deller wrote:
> > Helge Deller <deller at 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 <deller at gmx.de>
> > Reported-by: Dominik Derigs <dl6er at 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