Skip site navigation (1)Skip section navigation (2)
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>