svn commit: r349890 - head/contrib/telnet/telnet

[Shawn Webb]

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 at 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?

Thanks,

-- 
Shawn Webb
Cofounder / Security Engineer
HardenedBSD

Tor-ified Signal:    +1 443-546-8752
Tor+XMPP+OTR:        lattera at is.a.hacker.sx
GPG Key ID:          0xFF2E67A277F8E1FA
GPG Key Fingerprint: D206 BB45 15E0 9C49 0CF9  3633 C85B 0AF8 AB23 0FB2
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20190710/c193d7f6/attachment.sig>