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

[Shawn Webb]

On Fri, Jul 12, 2019 at 01:30:25AM +1000, Bruce Evans wrote:
> On Thu, 11 Jul 2019, Enji Cooper wrote:
> 
> > > On Jul 10, 2019, at 3:36 PM, Philip Paeps <philip at FreeBSD.org> wrote:
> > > 
> > > Author: philip
> > > Date: Wed Jul 10 22:36:14 2019
> > > New Revision: 349896
> > > URL: https://svnweb.freebsd.org/changeset/base/349896
> > > 
> > > Log:
> > >  telnet: fix minor style violation
> > > 
> > >  While here also fix a very unlikely NULL pointer dereference.
> 
> I see no fix here.
> 
> > > Modified: head/contrib/telnet/telnet/commands.c
> > > ==============================================================================
> > > --- head/contrib/telnet/telnet/commands.c	Wed Jul 10 22:23:59 2019	(r349895)
> > > +++ head/contrib/telnet/telnet/commands.c	Wed Jul 10 22:36:14 2019	(r349896)
> > > @@ -45,6 +45,7 @@ __FBSDID("$FreeBSD$");
> > > #include <sys/socket.h>
> > > #include <netinet/in.h>
> > > 
> > > +#include <assert.h>
> > > #include <ctype.h>
> > > #include <err.h>
> > > #include <errno.h>
> > > @@ -1654,11 +1655,13 @@ env_init(void)
> > > 		|| (strncmp((char *)ep->value, "unix:", 5) == 0))) {
> > > 		char hbuf[256+1];
> > > 		char *cp2 = strchr((char *)ep->value, ':');
> > > +                size_t buflen;
> 
> This adds a different type of style bug (indentation via spaces instead of
> tabs).
> 
> > > 
> > > 		gethostname(hbuf, sizeof(hbuf));
> > > 		hbuf[sizeof(hbuf)-1] = '\0';
> > > -                unsigned int buflen = strlen(hbuf) + strlen(cp2) + 1;
> > > + 		buflen = strlen(hbuf) + strlen(cp2) + 1;
> > > 		cp = (char *)malloc(sizeof(char)*buflen);
> > > +		assert(cp != NULL);
> > 
> > This will unfortunately still segfault if assert is compiled out of the system as a no-op (-DNDEBUG).
> 
> telnet is unlikely to be compiled with -DNDEBUG, since it didn't use assert()
> before and this commit doesn't change its Makefile to control its new use
> of assert().
> 
> Any it doesn't fix the error either.  On must arches, it turns a nice
> restartable null pointer trap into an unrestartable abort().  The program
> crashes in both cases.

We're getting into theory, since this particular bug isn't vulnerable
to this particular issue I'm about to bring up, but it is possible to
map at NULL on FreeBSD, given a sysctl has been explicitly toggled by
an administrative user.

I've found it's best to adhere to good defensive programming
techniques, even in cases where doing so may not make immediate sense.
Future developers, even the code's original author(s), may be grateful
later as they make changes.

Thus, even if this particular potential NULL pointer dereference isn't
exploitable in any meaningful way, adherence to defensive programming
practices will help both now and later.

One thing I love about FreeBSD is how it strives to deliver high
quality, correct code. It seems strange that more characters have been
written in emails about the lack of asprintf usage than it would've
taken to actually write the patch to fix it.

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-all/attachments/20190711/05076e5b/attachment.sig>