Date: Wed, 10 Jul 2019 16:45:06 -0600 From: Warner Losh <imp@bsdimp.com> To: Shawn Webb <shawn.webb@hardenedbsd.org> Cc: Justin Hibbits <chmeeedalf@gmail.com>, Philip Paeps <philip@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, svn-src-head <svn-src-head@freebsd.org> Subject: Re: svn commit: r349890 - head/contrib/telnet/telnet Message-ID: <CANCZdfoQX8N2TzeZdGb1BiUCkub8wCxBByk3fUUojOjGRA0cTw@mail.gmail.com> In-Reply-To: <20190710224341.i3rbhapmpdw6gkbd@mutt-hbsd> References: <201907101742.x6AHg4os016752@repo.freebsd.org> <20190710195548.kdftfemj3icarcxo@mutt-hbsd> <20190710151944.0fd94ec3@titan.knownspace> <20190710202218.yc3lcd6tsql3zkyr@mutt-hbsd> <20190710222918.vj4creizlubdzgw3@mutt-hbsd> <CANCZdfoDR9UTU3opp%2BA=WSgioJH3LJ4Je1AjQHDTAt%2Byni3NWQ@mail.gmail.com> <20190710224341.i3rbhapmpdw6gkbd@mutt-hbsd>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Jul 10, 2019 at 4:43 PM Shawn Webb <shawn.webb@hardenedbsd.org> wrote: > On Wed, Jul 10, 2019 at 04:40:25PM -0600, Warner Losh wrote: > > On Wed, Jul 10, 2019 at 4:29 PM Shawn Webb <shawn.webb@hardenedbsd.org> > > wrote: > > > > > On Wed, Jul 10, 2019 at 04:22:18PM -0400, Shawn Webb wrote: > > > > On Wed, Jul 10, 2019 at 03:19:44PM -0500, Justin Hibbits wrote: > > > > > On Wed, 10 Jul 2019 15:55:48 -0400 > > > > > Shawn Webb <shawn.webb@hardenedbsd.org> wrote: > > > > > > > > > > > On Wed, Jul 10, 2019 at 05:42:04PM +0000, Philip Paeps wrote: > > > > > > > Author: philip > > > > > > > Date: Wed Jul 10 17:42:04 2019 > > > > > > > New Revision: 349890 > > > > > > > URL: https://svnweb.freebsd.org/changeset/base/349890 > > > > > > > > > > > > > > Log: > > > > > > > telnet: fix a couple of snprintf() buffer overflows > > > > > > > > > > > > > > Obtained from: Juniper Networks > > > > > > > MFC after: 1 week > > > > > > > > > > > > > > Modified: > > > > > > > head/contrib/telnet/telnet/commands.c > > > > > > > head/contrib/telnet/telnet/telnet.c > > > > > > > head/contrib/telnet/telnet/utilities.c > > > > > > > > > > > > > > Modified: head/contrib/telnet/telnet/commands.c > > > > > > > > > > > ============================================================================== > > > > > > > --- head/contrib/telnet/telnet/commands.c Wed Jul 10 > > > > > > > 17:21:59 2019 (r349889) +++ > > > > > > > head/contrib/telnet/telnet/commands.c Wed Jul 10 17:42:04 > > > > > > > 2019 (r349890) @@ -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); > > > > > > > + gethostname(hbuf, sizeof(hbuf)); > > > > > > > + hbuf[sizeof(hbuf)-1] = '\0'; > > > > > > > + unsigned int buflen = strlen(hbuf) + > strlen(cp2) + > > > > > > > 1; > > > > > > > > > > > > buflen should be defined with the rest of the variables in the > code > > > > > > block above this one. > > > > > > > > > > Agreed. > > > > > > > > > > > > > > > > > > + cp = (char *)malloc(sizeof(char)*buflen); > > > > > > > > > > > > Lack of NULL check here leads to > > > > > > > > > > > > > + snprintf((char *)cp, buflen, "%s%s", hbuf, > cp2); > > > > > > > > > > > > potential NULL pointer deref here. > > > > > > > > > > I'm not sure if this is actually a problem. env_init() is called > > > > > exactly once, at the beginning of main(), and the environment size > is > > > > > fully constrained by the OS. > > > > > > > > > > That said, this file it the only one in this component that does > not > > > > > check the return value of malloc(). All other uses, outside of > this > > > > > file, check and error. > > > > > > > > While fixing the style(9) violation above, we could still take care > of > > > > the potential NULL deref at the same time. If anything, just for code > > > > correctness reasons? > > > > > > Here's a patch: > > > > > > https://gist.github.com/579685c0252673c3ad92d2536c3486c7 > > > > > > Any reason to not use asprintf instead of malloc + snprintf? > > Because the existing code already used malloc + snprintf. And this is > contrib/telnet/telnet, which arguably should be `rm -rf`ed. ;) > > The bike shed is now glow-in-the-dark neon green. > I'm not in the rm-rf telnet camp... I use it far too often to connect to a port with good terminal support to want to do that... Warner
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfoQX8N2TzeZdGb1BiUCkub8wCxBByk3fUUojOjGRA0cTw>