fixing the ftp crashes found via fuzzer (was: Re: [PATCH 3/3] telnet: Avoid command evaluation crashes.)

[Erik Auerswald]

Hi,

On Mon, Sep 19, 2022 at 10:31:15PM +0200, Erik Auerswald wrote:
> On Sat, Sep 03, 2022 at 07:07:52PM +0200, Erik Auerswald wrote:
> > On Sat, Sep 03, 2022 at 05:39:45PM +0200, Simon Josefsson wrote:
> > > [...]
> > > did you notice some fuzzing report that wasn't fixed?
> > 
> > I think the following reports have not yet been addressed:
> > 
> > * Problems found in ftp (the code did not change since the reports):
> > 
> >   * Untrusted Pointer Dereference in domacro() at 
> > inetutils/ftp/domacro.c:186
> >     https://lists.gnu.org/archive/html/bug-inetutils/2021-12/msg00003.html
> >     (https://savannah.gnu.org/bugs/?61722)
> 
> This one is a signed integer overflow leading to an out of bounds buffer
> read, possibly followed by an out of bounds buffer write.
> 
> Simple reproducer:
> 
>     printf -- 'macdef x\n$999999999999999999\n\n$x\n' | ./ftp/ftp
> 
> I have an initial fix, but want to take a better look at the issue and
> hopefully come up with a better solution.
> 
> >   * Infinite Loop in domacro at domacro.c:258
> >     https://lists.gnu.org/archive/html/bug-inetutils/2021-12/msg00005.html
> >     https://savannah.gnu.org/bugs/?61724
> >     https://lists.gnu.org/archive/html/bug-inetutils/2021-12/msg00008.html
> 
> This is a feature of the macro functionality in the ftp client.
> Macros can call macros which allows recursion.  This trivially allows
> to create infinite recursion until the stack is exhausted and ftp crashes.
> 
> Simple reproducer:
> 
>     printf -- 'macdef X\n$X\n\n$X\n' | ./ftp/ftp
> 
> I am not sure if this is worth "fixing", and what exactly would constitute
> a "fix".
> 
> >   * A heap-buffer-overflow in another () at cmds.c:202
> >     https://lists.gnu.org/archive/html/bug-inetutils/2021-12/msg00016.html
> 
> This one is a bit more involved.  It seems as if the strcpy in line
> 266 can overflow the destination buffer, because that buffer can be
> changed during macro execution via the another() function.  I have not
> yet understood the exact mechanism, and do not have a simple reproducer.
> Neither do I have a fix.

I think I have a fix for this.  If the buffer changed and is now too small,
reallocate it with a sufficient size.

> >   * NULL Pointer Dereference in setnmap() at cmds.c:2303
> >     https://lists.gnu.org/archive/html/bug-inetutils/2021-12/msg00004.html
> >     https://savannah.gnu.org/bugs/?61723
> >     https://lists.gnu.org/archive/html/bug-inetutils/2021-12/msg00013.html
> 
> The setnmap() function only works correctly with space characters
> as argument separators, but makeargv() works with both space and tab
> characters.  This can result in a NULL pointer dereference when makeargv()
> splits arguments on a tab, but setnmap() searches for a space which is
> not there.
> 
> Simple reproducer:
> 
>     printf -- 'nmap x\ty\n' | ./ftp/ftp
> 
> I have an initial fix for the fuzzer input and the above reproducer,
> but also a slightly different reproducer that also crashes ftp.  A fix
> similar to that for the fuzzer report fixes the second crash, but breaks
> some ftp client functionality, so this needs more work.

I'll try to commit and push regression tests and fixes for the first,
third, and fourth problem during the weekend.

What do you all think regarding recursive macros (the second problem)?

Br,
Erik