Re: [BUG][PATCH] Someone described a remote DoS Vulnerability in telnetd (dereference NULL pointer ---> SEGV)

[Guillem Jover]

[ Resending with To trimmed. ]

Hi!

On Tue, 2022-08-30 at 22:57:51 +0200, Guillem Jover wrote:
> On Sun, 2022-08-28 at 14:40:44 +0200, Erik Auerswald wrote:
> > On Sat, Aug 27, 2022 at 07:37:15PM +0200, Erik Auerswald wrote:
> > > someone has described a remote DoS vulnerability in
> > > many telnetd implementations that I just happened to
> > > stumble over:
> > > 
> > > https://pierrekim.github.io/blog/2022-08-24-2-byte-dos-freebsd-netbsd-telnetd-netkit-telnetd-inetutils-telnetd-kerberos-telnetd.html
> > > 
> > > The vulnerability is a NULL pointer dereference when
> > > reading either of two two byte sequences:
> > > 
> > > 1: 0xff 0xf7
> > > 2: 0xff 0xf8
> > > 
> > > The blog shows GNU Inetutils' telnetd as vulnerable:
> > > 
> > > https://pierrekim.github.io/blog/2022-08-24-2-byte-dos-freebsd-netbsd-telnetd-netkit-telnetd-inetutils-telnetd-kerberos-telnetd.html#remote-dos-inetutils
> 
> This has been assigned CVE-2022-39028 (I think from the Debian pool),
> after I reported it to the Debian security team.

While it might have been nice to get this in the commit message, I
think it would still be nice to add a reference in the NEWS. :)

> > > [...]
> > > In GNU Inetutils, the code lines to dereference table
> > > entries without first checking for NULL are in lines
> > > 321 and 323 of file "telnetd/state.c".  The variable
> > > "ch" declared in line 315 of this file needs to be
> > > initialized to "(cc_t) (_POSIX_VDISABLE)", because it
> > > may not be assigned any value if the table is not yet
> > > initialized.
> > > 
> > > References:
> > > 
> > > line 315: 
> > > https://git.savannah.gnu.org/cgit/inetutils.git/tree/telnetd/state.c#n315
> > > line 321: 
> > > https://git.savannah.gnu.org/cgit/inetutils.git/tree/telnetd/state.c#n321
> > > line 323: 
> > > https://git.savannah.gnu.org/cgit/inetutils.git/tree/telnetd/state.c#n323
> > > 
> > > I have attached a completely untested, not even compile
> > > tested, patch to do this (just the code changes, no NEWS
> > > or commit log or anything).  Please test before committing.
> > 
> > I have tested the patch now, it compiles and prevents the
> > crash by preventing the NULL pointer dereference.
> 
> Thanks, I included this the other day in an upload to Debian sid, and
> I'm preparing updates for the Debian stable and oldstable releases too.

Thanks,
Guillem

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

[Erik Auerswald]

Hi Simon,

On Tue, Sep 06, 2022 at 08:05:04PM +0200, Simon Josefsson wrote:
> Erik Auerswald  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
>From 0cb957adf678cb32936e5e9ad5727c8ad5e28825 Mon Sep 17 00:00:00 2001
From: Erik Auerswald 
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
.

* 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:
 .
 
+** tftp
+
+*** Avoid crashing when given unexpected or invalid commands from tty.
+Reported by AiDai in
+.
+
 * 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