svn commit: r349890 - head/contrib/telnet/telnet

[Bruce Evans]

On Thu, 11 Jul 2019, Alexey Dokuchaev wrote:

> On Wed, Jul 10, 2019 at 05:42:04PM +0000, Philip Paeps wrote:
>> New Revision: 349890
>> URL: https://svnweb.freebsd.org/changeset/base/349890
>>
>> Log:
>>   telnet: fix a couple of snprintf() buffer overflows

I see few fixes in this fix.  I see even more style bugs than before.

>> Modified: head/contrib/telnet/telnet/commands.c
>> @@ -1655,10 +1655,11 @@ env_init(void)
>>  		char hbuf[256+1];
>>  		char *cp2 = strchr((char *)ep->value, ':');
>>
>> -		gethostname(hbuf, 256);
>> -		hbuf[256] = '\0';
>> -		cp = (char *)malloc(strlen(hbuf) + strlen(cp2) + 1);
>> -		sprintf((char *)cp, "%s%s", hbuf, cp2);
>
> Would it make sense to add something like __attribute__ ((deprecated))
> to those unsafe functions like gets(), sprintf(), etc.?  Or it would
> cause too much PITA?

sprintf() is safe to use and was used perfectly correctly (except for not
not checking the validity of most of its args: no check of the result of
gethostname() or malloc(), and no check for overflow in
'strlen(hbuf) + strlen(cp2) + 1'.  malloc() is used to provide a buffer
just large enough for sprintf() to not overflow the buffer, modulo the
missing error checking.  Blindly changing to snprintf() does little
to improve this.  It gives a garbage buffer instead of buffer overrun
in some of the error cases.  It is missing error checking.

The bugs visible in the above are:
- hard-coded size for hbuf.  The correct size is {HOST_NAME_MAX} + 1.
   HOST_NAME_MAX is intentionally not defined in FreeBSD's <limits.h>, so
   even unportable POSIX applications have to deal with the full complexity
   of POSIX limits (they can't simply hard-code HOST_NAME_MAX).  POSIX
   requires using sysconf(_SC_HOST_NAME_MAX) and dealing with errors and
   indeterminate values reported by it.  Unfortunately, FreeBSD also has
   MAXHOSTNAMELEN.  sysconf(_SC_HOST_NAME_MAX) just returns this minus 1.
   This is is 256.  The hard-coded 256 in the above is essentially this,
   and adding 1 to this is nonsense.

   telnet uses MAXHOSTNAMELEN for global variables.  It also #defines
   MAXHOSTNAMELEN as 256 if it is not defined in an included header.  So
   if the extra 1 were actually used, then the global variables couldn't
   hold hostnames and there would be bugs elswhere.

   Honestly unportable code would use MAXHOSTNAME everywhere, and not add
   1 to it, and depend on this being large enough.  gethostname() guarantees
   to nul-terminated if it succeeds and the buffer is large enough.

   telnet uses gethostname() in 1 other place.  In telnet.c, it initializes
   the global local_host which has size MAXHOSTNAME.  Of course it doesn't
   check for errors.  It does force nul-termination.

- missing spaces around binary operator in '256+1'

- initialization in declaration of cp2

- bogus cast to char * in declaration of cp2.  This breaks the warning that
   ep->value has the bogus type unsigned char *.  Not many character arrays
   actually need to have type unsigned char *, and if they do then it might
   be an error to pass them to str*() functions since str*() functions are
   only guaranteed to work right for plain char *.

- no error check for gethostname(), bogus dishonestly unportable size for
   hbuf, and partial fixup for these bugs by forcing nul-termination (see above)

- bogus cast of malloc() to char *.  This was more needed 30-40 years ago
   for bad code that doesn't declare malloc() in a standard header or doesn't
   include that header.  Telnet is that old, so this wasn't bogus originally.

- no check for overflow in 'strlen(hbuf) + strlen(cp2) + 1'.  Checking this
   would be silly, but not much sillier than forcing nul-termination after
   not trusting gethostname() and/or our buffer size.  strlen(hbuf) is known
   to be small, and strlen(cp2) must be known to be non-preposterous, else
   overflow is possible.  However, cp2 is a substring of an environment
   variable, so it can be almost as large as the address space if a malicious
   environment can be created.  Perhaps it is limited to ARG_MAX.  Practical
   code could limit its size to 256 or so and statically allocate the whole
   buffer on the stack.  Or don't limit it, and use alloca() or a VLA to
   allocate the whole buffer on the stack.  Paranoid code of course can't
   even have a stack, unless the C runtime checks for stack overflow and
   the program can handle exceptions from that.

- bogus cast of cp arg to char * in sprintf() call.  cp doesn't suffer from
   unsigned poisoning, so already has type char *.

- no cast of cp2 arg in sprintf() call.  Such a cast can only break the
   warning.  It is unclear if %s format can handle args of type unsigned
   char *.  Even if the behaviour is defined, then I think it ends up
   converting unsigned char to char via a type pun.

The string concatenation can be done even more easily and correctly using
strcat(), but style(9) says to use printf() instead of special methods and
that is a good rule for sprintf() too.

Bruce