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

[Erik Auerswald]

Hi Guillem,

On 03.09.22 20:08, Guillem Jover wrote:
> [ Removed Erik from To, as last time my mail was rejected by the mail
>    server, and might then not get delivered by mailman as duplicate. ]

:-(

I have sent this email to bug-inetutils@gnu.org only to avoid this
kind of problem.

> You might want to take a look at:
>
>    
> <https://git.hadrons.org/cgit/debian/pkgs/inetutils.git/tree/debian/patches/0004-telnet-Add-checks-for-option-reply-parsing-limits.patch>

Thanks for pointing out that patch.  Without it telnet crashes when
it starts the log in process:

    $ ./telnet/telnet -l`perl -e 'print "A"x5000'` localhost 4711
    Trying 127.0.0.1...
    Connected to localhost.
    Escape character is '^]'.
    realloc(): invalid next size
    Aborted (core dumped)

With the patch, it does not crash:

    $ ./telnet/telnet -l`perl -e 'print "A"x5000'` localhost 4711
    Trying 127.0.0.1...
    Connected to localhost.
    Escape character is '^]'.
    [...]
    login: Cannot possibly work without effective root
    Connection closed by foreign host.

This crash does not happen with the "tests/addrpeek" test binary:

    $ ./telnet/telnet -l`perl -e 'print "A"x5000'` localhost 4711
    Trying 127.0.0.1...
    Connected to localhost.
    Escape character is '^]'.
    Your address is 127.0.0.1.
    Connection closed by foreign host.

Thus it is not trivial to add a test to GNU Inetutils.  That is
no reason for not fixing the bug, of course.

@Simon: if you think it is OK to add this patch to GNU Inetutils,
        feel free to just go ahead and do so.

I have to admit that I have a hard time understanding the details
of the patch.  Thus I'll try to analyze it:

The patch changes only one file, "telnet/telnet.c".

The OPT_REPLY_SIZE is changed from 256 to depend on SUBBUFSIZE.
Currently that sets OPT_REPLY_SIZE to 2 * 256 = 512:

    -#define OPT_REPLY_SIZE     256
    +#define OPT_REPLY_SIZE     (2 * SUBBUFSIZE)

It seems to me as if this is not really required to fix the
crash, but rather to support larger option values.  Is there
a specific reason to use twice the SUBBUFSIZE for OPT_REPLY_SIZE?

The global pointer *opt_reply is changed from implicit
initialization to 0 to explicit initialization to NULL:

    -unsigned char *opt_reply;
    +unsigned char *opt_reply = NULL;

In line 1719, opt reply is compared with NULL, thus it seems
correct to initialize it to NULL.  But there seems to be quite
a bit of code that assumes NULL to have the value 0 (or at
least to be "false").  Thus I wonder if similar changes would
be advisable all over the code….

Two bounds checks are introduced to env_opt_add():

    +     if (opt_replyp + (2 + 2) > opt_replyend)
    +       return;

The code guarded by this check adds two bytes to the contents
of opt_reply[OPT_REPLY_SIZE].  That could be one of the "2"
values.

    +     if (opt_replyp + (1 + 2 + 2) > opt_replyend)
    +       return;

The code guarded by this check adds one byte to the contents
of opt_reply[OPT_REPLY_SIZE].  That could be the "1" value.

A bounds check is introduced to env_opt_end():

    +  if (opt_replyp + 2 > opt_replyend)
    +    return;

The code guarded by this check adds two bytes to the contents
of opt_reply[OPT_REPLY_SIZE].  That fits with the "2" value.

This might be the other "2" value from the two checks above
in env_opt_add().  I do not understand the second "2" value
in the second bounds check in env_opt_add(), though.

What did I miss?

Of course, I may have made mistakes reading the code.

Then there is the nagging issue that I did not see how these
changes prevent the 5000 A bytes from overflowing the now
512 byte buffer.  Could it be that there are other bounds
checks that should be adjusted as well to account for this
overhead of up to five bytes?  In addition to, not as a
replacement of, the checks from the patch.

Anyway, the patch seems OK to me after looking more closely
at the details.

> The implementations in the BSDs are probably worth checking for
> similar old fixes.

I have to concur… any volunteers? ;-)

Thanks,
Erik