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

[Erik Auerswald]

Hi Simon,

On Fri, Jul 08, 2022 at 09:51:28AM +0200, Simon Josefsson wrote:
> Erik Auerswald <auerswal@unix-ag.uni-kl.de> writes:
> 
> > I'll try to look into this over the weekend, but I cannot promise
> > anything.  But this patch series should just address the same issues
> > as the patches I sent before, perhaps with some smaller differences.
> > It's been some time and I do not remember all the details.
> 
> I have merged your NEWS entries now.  There are some remaining patches
> that aren't committed, but I'm not sure they are as critical anymore and
> if we change ancient code it should preferably come with a self-check
> that excercise the code path.

I had seen your email concerning such a change, but did not yet have
sufficient time to look at it in detail.

I want to look at your new tests, perhaps I can learn how to add some
tests to verify the genget() functionality.

OK, looking at the patches now... references:
https://lists.gnu.org/archive/html/bug-inetutils/2022-02/msg00011.html
https://lists.gnu.org/archive/html/bug-inetutils/2022-02/msg00012.html

Invoking a command exercises the genget() function (as far as I remember).
That would be a basis for tests.

The interactive help for telnet commands uses " " table entries to
display empty lines.  That could be used as basis for a test, too.

Any duplicated entry is shadowed by the first such entry.  Assuming that
the second entry is intended for use, this does not work anyway.

All existing " " (a single space as command name) entries look like
empty lines in the table that should never be matched.  The first such
line shadows all following such entries.

These changes are intended to harden against possibly overlooked existing
problems and to make it less likely to introduce similar problems in
the future, by producing failures early during development.  The known
crashes not prevented by these issues in genget() should have been fixed
via my other patches.  Thus they are not pressing and we can take our
time evaluating them.

Since they are not known to fix any existing problem, but they do change
some functionality deep inside the telnet client (and the telnet daemon
via encryption as well), there is some regression risk.  I have not
tested encrypted telnet and do not have a Kerberos setup available.
I do not expect any problem there (genget() is used to access the
"encryptions" table), but I cannot test that.

> >> Btw, it is generally preferrably to have one commit solve one bug
> >> report instead of doing several bug reports in one commit like your
> >> proposed patch did.
> >
> > Yes, I agree.  I rolled everything up into three mostly independent
> > patches to make it easier for me to maintain the patches externally,
> > and to need to write less commit messages. ;-)
> 
> Do you maintain your patchset publicly, or for internal uses only?

It is publicly available at:
https://www.unix-ag.uni-kl.de/~auerswal/gnu-inetutils/

I did not change it since the original posting to the bug-inetutils
mailing list.  I did add a patch against inetutils 2.2 based on the
patches against git for convenient internal use.

Thanks,
Erik
-- 
Be water, my friend.
                        -- Bruce Lee