From owner-svn-src-head@freebsd.org Thu Jul 11 13:25:38 2019 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 40ABE15D28D1; Thu, 11 Jul 2019 13:25:38 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail105.syd.optusnet.com.au (mail105.syd.optusnet.com.au [211.29.132.249]) by mx1.freebsd.org (Postfix) with ESMTP id 89E6B76F09; Thu, 11 Jul 2019 13:25:37 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail105.syd.optusnet.com.au (Postfix) with ESMTPS id F1BDF3607A7; Thu, 11 Jul 2019 23:25:26 +1000 (AEST) Date: Thu, 11 Jul 2019 23:25:24 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Alexey Dokuchaev cc: Philip Paeps , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r349890 - head/contrib/telnet/telnet In-Reply-To: <20190711014729.GB23621@FreeBSD.org> Message-ID: <20190711221158.N1533@besplex.bde.org> References: <201907101742.x6AHg4os016752@repo.freebsd.org> <20190711014729.GB23621@FreeBSD.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=FNpr/6gs c=1 sm=1 tr=0 cx=a_idp_d a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=jpOVt7BSZ2e4Z31A5e1TngXxSK0=:19 a=kj9zAlcOel0A:10 a=6I5d2MoRAAAA:8 a=EprUL-5I6KnjuIzbal4A:9 a=YoCYy-C6q4L7z-oV:21 a=y7qSRRag8CzNf-xG:21 a=CjuIK1q_8ugA:10 a=IjZwj45LgO3ly-622nXo:22 X-Rspamd-Queue-Id: 89E6B76F09 X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.94 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; REPLY(-4.00)[]; NEURAL_HAM_SHORT(-0.94)[-0.937,0] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Jul 2019 13:25:38 -0000 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 , 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