Re: TFTP client crash seems to be caused by missing bounds check in makeargv()
Erik Auerswald <[email protected]>
| Newsgroups | gmane.comp.gnu.inetutils.bugs |
|---|---|
| Message-ID | <[email protected]> |
Hi Simon, On Tue, Sep 06, 2022 at 08:05:04PM +0200, Simon Josefsson wrote: > Erik Auerswald <[email protected]> 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? Thanks, Erik
0001-tftp-ignore-excess-arguments.patch
(text/x-diff, 1.8 KB)
From 0cb957adf678cb32936e5e9ad5727c8ad5e28825 Mon Sep 17 00:00:00 2001 From: Erik Auerswald <[email protected]> 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)) -- 2.17.1