Date: Wed, 24 Jul 2019 18:50:54 -0700 From: Conrad Meyer <cem@freebsd.org> To: Philip Paeps <philip@freebsd.org> Cc: 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: <CAG6CVpXj_QayDQvLvSi29QHDfesoPuSaA%2BTRdV6RXEUAPZmagA@mail.gmail.com> In-Reply-To: <201907101742.x6AHg4os016752@repo.freebsd.org> References: <201907101742.x6AHg4os016752@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Philip, Sorry I'm late to the party. Unless I am mistaken, most of these are *not* overflows or vulnerabilities of any sort. On Wed, Jul 10, 2019 at 10:42 AM Philip Paeps <philip@freebsd.org> 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 >... > --- 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; > + cp = (char *)malloc(sizeof(char)*buflen); > + snprintf((char *)cp, buflen, "%s%s", hbuf, cp2); This patch makes no functional change, except gethostname()'s 2nd parameter is now 257 instead of 256. It was not an overflow before and the formatted malloc contents are identical. > Modified: head/contrib/telnet/telnet/telnet.c > ============================================================================== > --- head/contrib/telnet/telnet/telnet.c Wed Jul 10 17:21:59 2019 (r349889) > +++ head/contrib/telnet/telnet/telnet.c Wed Jul 10 17:42:04 2019 (r349890) > @@ -785,7 +785,7 @@ suboption(void) > name = gettermname(); > len = strlen(name) + 4 + 2; > if (len < NETROOM()) { > - sprintf(temp, "%c%c%c%c%s%c%c", IAC, SB, TELOPT_TTYPE, > + snprintf(temp, sizeof(temp), "%c%c%c%c%s%c%c", IAC, SB, TELOPT_TTYPE, > TELQUAL_IS, name, IAC, SE); This one actually overflowed before. But the new behavior isn't much better. Truncating the formatted string arbitrarily is still wrong; it would be better to errx() or assert() or abort(). > @@ -807,7 +807,7 @@ suboption(void) > > TerminalSpeeds(&ispeed, &ospeed); > > - sprintf((char *)temp, "%c%c%c%c%ld,%ld%c%c", IAC, SB, TELOPT_TSPEED, > + snprintf((char *)temp, sizeof(temp), "%c%c%c%c%ld,%ld%c%c", IAC, SB, TELOPT_TSPEED, > TELQUAL_IS, ospeed, ispeed, IAC, SE); > len = strlen((char *)temp+4) + 4; /* temp[3] is 0 ... */ Unless I'm miscounting, this could not overflow (on any FreeBSD system with 64-bit or smaller long)... > Modified: head/contrib/telnet/telnet/utilities.c > ============================================================================== > --- head/contrib/telnet/telnet/utilities.c Wed Jul 10 17:21:59 2019 (r349889) > +++ head/contrib/telnet/telnet/utilities.c Wed Jul 10 17:42:04 2019 (r349890) > @@ -629,7 +629,7 @@ printsub(char direction, unsigned char *pointer, int l > } > { > char tbuf[64]; > - sprintf(tbuf, "%s%s%s%s%s", > + snprintf(tbuf, sizeof(tbuf), "%s%s%s%s%s", > pointer[2]&MODE_EDIT ? "|EDIT" : "", > pointer[2]&MODE_TRAPSIG ? "|TRAPSIG" : "", > pointer[2]&MODE_SOFT_TAB ? "|SOFT_TAB" : "", This one could not overflow before either. I think most of this change is an unnecessary regression, and the actual overflow should be fixed in a better way anyway. Thanks, Conrad
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAG6CVpXj_QayDQvLvSi29QHDfesoPuSaA%2BTRdV6RXEUAPZmagA>