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
_______________________________________________
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
