Re: [PATCH 3/3] telnet: Avoid command evaluation crashes.

[Erik Auerswald]

Hi Simon,

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.

>   * 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.

That's all for now.

Br,
Erik
-- 
The computing scientist’s main challenge is not to get confused by
the complexities of his own making.
                        -- Edsger W. Dijkstra