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

[Erik Auerswald]

Hello Simon,

On Sat, Jul 09, 2022 at 02:36:41PM +0200, Simon Josefsson wrote:
> Erik Auerswald <auerswal@unix-ag.uni-kl.de> writes:
> > On Fri, Jul 08, 2022 at 08:55:18AM +0200, Erik Auerswald wrote:
> >> On Fri, Jul 08, 2022 at 12:58:37AM +0200, Simon Josefsson wrote:
> >> 
> >> > Thanks for preparing these [...]
> >> 
> >> Thanks for applying my patches!
> >> [...]
> > The code looks good.  As intended the two genget() changes are not
> > included, everything else is.  The NEWS entries look good, too.
> 
> Thank you!  I don't care strongly about the genget patches,

I might separate the genget() changes out into two patches (they pertain
to two distinct issues) with individual rationales since they pertain
to two different possible robustness improvements.

> I mostly have a general concern that we should minimize changes with
> no externally testable difference.

I agree that the genget() changes lack tests.  I am optimistic that I
can write tests, probably by checking the error messages.

> OTOH, the NetBSD folks patched the telnet bugs in a different way than
> we did, and I'm not sure how to handle that...  we could adopt their
> solution, or stick with ours.
> 
> https://cvsweb.netbsd.org/bsdweb.cgi/src/usr.bin/telnet/commands.c.diff?r1=1.79&r2=1.80&only_with_tag=MAIN

[The https:// link did not work for me (timeout), using http:// instead
 did, so looked at that.]

I would suggest to keep our solution, because the NetBSD "fix" ignores
the actual problem of setcmd() and unsetcmd() happily following a NULL
pointer into chaos and madness.  They only guard against attempting to
(un)set the ' ' setting value.

Their changes also ignore the makeargv() issue, since it does not
pertain to their fork.  They already have a different fix implemented
with an addarg() function not found in the GNU Inetutils telnet client
commands.c file.

While the FreeBSD output looks as if they might be using a makeargv()
solution similar to NetBSD, OpenBSD obviously has a different one.
(I did not look at the code to verify this, just at the output found in
<https://lists.gnu.org/archive/html/bug-inetutils/2022-07/msg00021.html>.)

> Let's see if the FreeBSD and OpenBSD folks apply it or come with
> something different...
> 
> Maybe the goal of harmonizing the various implementations is just a
> pipe dream...

It seems to me as if there are already quite a few differences in the
code bases.  :-/

Br,
Erik
-- 
The right tool for the job is often the tool you are already using -
adding new tools has a higher cost than many people appreciate.
                        -- John Carmack