Date: Sat, 6 Feb 2016 00:48:38 +0100 From: Jilles Tjoelker <jilles@stack.nl> To: Garrett Cooper <ngie@FreeBSD.org> Cc: src-committers@freebsd.org, svn-src-user@freebsd.org Subject: Re: svn commit: r295247 - user/ngie/bsnmp_cleanup/contrib/bsnmp/lib Message-ID: <20160205234838.GB97435@stack.nl> In-Reply-To: <201602040908.u1498buB006713@repo.freebsd.org> References: <201602040908.u1498buB006713@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Feb 04, 2016 at 09:08:37AM +0000, Garrett Cooper wrote: > Author: ngie > Date: Thu Feb 4 09:08:36 2016 > New Revision: 295247 > URL: https://svnweb.freebsd.org/changeset/base/295247 > Log: > Use mkstemp(3) instead of mktemp(3) when creating temporary files to fix > the security pragma > Modified: > user/ngie/bsnmp_cleanup/contrib/bsnmp/lib/snmpclient.c > Modified: user/ngie/bsnmp_cleanup/contrib/bsnmp/lib/snmpclient.c > ============================================================================== > --- user/ngie/bsnmp_cleanup/contrib/bsnmp/lib/snmpclient.c Thu Feb 4 09:07:44 2016 (r295246) > +++ user/ngie/bsnmp_cleanup/contrib/bsnmp/lib/snmpclient.c Thu Feb 4 09:08:36 2016 (r295247) > @@ -1000,7 +1000,7 @@ open_client_local(const char *path) > snprintf(snmp_client.local_path, sizeof(snmp_client.local_path), > "%s", SNMP_LOCAL_PATH); > > - if (mktemp(snmp_client.local_path) == NULL) { > + if (mkstemp(snmp_client.local_path) == -1) { > seterr(&snmp_client, "%s", strerror(errno)); > (void)close(snmp_client.fd); > snmp_client.fd = -1; This shuts up the warning, but I think it will also completely break the client. Since mkstemp() creates a regular file, the subsequent bind() will fail and the client will not start. Also, the file descriptor is leaked. The security check has guessed correctly that mktemp() is being misused, though. If bind() fails because of [EEXIST], it should be retried with a new name to guard against a DoS. Fixing the problem this way will not make the security check happy, though. The problem can be fixed and the security check made happy by creating a temporary directory with mode 700 using mkdtemp() and binding to a name in there. Deletion will be a two-step process. Looking from a higher level, the bind() call may not be needed. It is only needed if the server calls getpeername() or passes non-NULL pointers to accept(), and uses that information. Using the pathname is likely unsafe since it may contain symlinks. -- Jilles Tjoelker
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160205234838.GB97435>