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

2022-09-08 Thread Simon Josefsson via Bug reports for the GNU Internet utilities 
Erik Auerswald  writes:

> 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?

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 
> 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))


signature.asc
Description: PGP signature

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

2022-09-07 Thread 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

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

2022-09-06 Thread Simon Josefsson via Bug reports for the GNU Internet utilities 
Erik Auerswald  writes:

> Hi,
>
> 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!  Integrating oss-fuzz would be too...

Re BSD tools: perhaps one way to proceed here is to start to sync code
so we at least have similar code bases to look at?  Maybe we can find
some code that is sufficiently similar so that we can simply setup
scripts to keep the code in sync for the future.  And hopefully make the
set of code that is kept in sync automatically larger and larger.  The
CVE-2019-0053 bug we discovered now was fixed in FreeBSD back in 2005...
I'm sure there are plenty of more discoveries like this waiting for us.
Having more code in sync helps with this.

/Simon


signature.asc
Description: PGP signature

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

2022-09-04 Thread Erik Auerswald 

Hi,

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?

Thanks,
Erik

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

2022-09-04 Thread Erik Auerswald 

Hi,

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?


I think the following reports have not yet been addressed:


I do intend to look into these issues, but I cannot say when...


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

There might be an opportunity to refactor the code to avoid
having to fix the same problem again and again….


[...]


Thanks,
Erik