Re: TFTP client crash seems to be caused by missing bounds check in makeargv()

[Simon Josefsson]

Erik Auerswald <auerswal@unix-ag.uni-kl.de> writes:

> Hi Simon,
>
> On Tue, Sep 06, 2022 at 08:05:04PM +0200, Simon Josefsson wrote:
>> Erik Auerswald <auerswal@unix-ag.uni-kl.de> writes:
>> > On 04.09.22 17:34, Erik Auerswald wrote:
>> >> On 03.09.22 19:07, 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?
>> >>> [...]
>> >>> * Problems found in tftp (the code did not change since the report):
>> >>>
>> >>>    * Untrusted Pointer Dereference in getcmd() at
>> >>> inetutils/src/tftp.c:878
>> >>>      
>> >>> https://lists.gnu.org/archive/html/bug-inetutils/2021-12/msg00018.html
>> >> That seems to be a missing bounds check in makeargv(), similar
>> >> to the old, now fixed, code in telnet.
>> >> I'll look into creating a nice reproducer instead of the one
>> >> found by the fuzzer, adding a test case, and fixing the bug.
>> >
>> > That is harder than expected….  Is there a reason *not* to use
>> > the crash input found by the fuzzer in a test for GNU Inetutils?
>> 
>> More testing would be great!
>
> I expect to find the time to finalize this during the coming weekend.
> I intend to use perl to write the fuzzer-generated test input provided
> by AiDai into the tftp client, similar to the telnet tests you have
> added for the respective crash bugs.
>
> After adding the test case I intend to commit the attached patch for tftp.
>
> What do you think?

Looks good to me, please add it.

Better would be to fix arbitrary limitations like this, but I'm mixed
about introducing yet further changes (and new bugs..) compared to BSD
tools.  For now I prefer fixing problems in the most minimal way and
similar way that has been fixed in other implementations.

/Simon

> Thanks,
> Erik
>
> From 0cb957adf678cb32936e5e9ad5727c8ad5e28825 Mon Sep 17 00:00:00 2001
> From: Erik Auerswald <auerswal@unix-ag.uni-kl.de>
> Date: Sun, 4 Sep 2022 17:36:22 +0200
> Subject: [PATCH] tftp: ignore excess arguments
>
> When given too many arguments to a command at the tftp cli,
> the buffer used to hold the arguments would overflow.  This
> could result in a crash.
>
> The problem was reported by AiDai in
> <https://lists.gnu.org/archive/html/bug-inetutils/2021-12/msg00018.html>.
>
> * src/tftp.c (makeargv): Do not overflow argument buffer.
> ---
>  NEWS       |  6 ++++++
>  src/tftp.c | 10 +++++++++-
>  2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/NEWS b/NEWS
> index 07115db1..6edeabea 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -8,6 +8,12 @@ GNU inetutils NEWS -- history of user-visible changes.
>  are 0xff 0xf7 (IAC EC) or 0xff 0xf8 (IAC EL).  Reported in:
>  
> <https://pierrekim.github.io/blog/2022-08-24-2-byte-dos-freebsd-netbsd-telnetd-netkit-telnetd-inetutils-telnetd-kerberos-telnetd.html>.
>  
> +** tftp
> +
> +*** Avoid crashing when given unexpected or invalid commands from tty.
> +Reported by AiDai in
> +<https://lists.gnu.org/archive/html/bug-inetutils/2021-12/msg00018.html>.
> +
>  * Noteworthy changes in release 2.3 (2022-07-08) [stable]
>  
>  ** telnet
> diff --git a/src/tftp.c b/src/tftp.c
> index 42abbb4a..6b1e209e 100644
> --- a/src/tftp.c
> +++ b/src/tftp.c
> @@ -122,7 +122,10 @@ static int fromatty;
>  char mode[32];
>  char line[200];
>  int margc;
> -char *margv[20];
> +
> +#define TFTP_MAX_ARGS 20
> +
> +char *margv[TFTP_MAX_ARGS];
>  char *prompt = "tftp";
>  jmp_buf toplevel;
>  void intr (int signo);
> @@ -914,6 +917,11 @@ makeargv (void)
>       cp++;
>        if (*cp == '\0')
>       break;
> +      if (margc + 1 >= TFTP_MAX_ARGS)
> +     {
> +       fprintf (stderr, "Ignoring excess arguments.\n");
> +       break;
> +     }
>        *argp++ = cp;
>        margc += 1;
>        while (*cp != '\0' && !isspace (*cp))

signature.asc
Description: PGP signature