Date: Thu, 11 Jul 2019 23:25:24 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Alexey Dokuchaev <danfe@freebsd.org> Cc: Philip Paeps <philip@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r349890 - head/contrib/telnet/telnet Message-ID: <20190711221158.N1533@besplex.bde.org> In-Reply-To: <20190711014729.GB23621@FreeBSD.org> References: <201907101742.x6AHg4os016752@repo.freebsd.org> <20190711014729.GB23621@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20190711221158.N1533>