From owner-svn-src-user@freebsd.org Fri Feb 5 23:48:41 2016 Return-Path: Delivered-To: svn-src-user@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 2DC23A9C180 for ; Fri, 5 Feb 2016 23:48:41 +0000 (UTC) (envelope-from jilles@stack.nl) Received: from mx1.stack.nl (relay02.stack.nl [IPv6:2001:610:1108:5010::104]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client CN "mailhost.stack.nl", Issuer "CA Cert Signing Authority" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id F29D8881; Fri, 5 Feb 2016 23:48:40 +0000 (UTC) (envelope-from jilles@stack.nl) Received: from toad2.stack.nl (toad2.stack.nl [IPv6:2001:610:1108:5010::161]) by mx1.stack.nl (Postfix) with ESMTP id 2726B35930C; Sat, 6 Feb 2016 00:48:39 +0100 (CET) Received: by toad2.stack.nl (Postfix, from userid 1677) id E27E4892B3; Sat, 6 Feb 2016 00:48:38 +0100 (CET) Date: Sat, 6 Feb 2016 00:48:38 +0100 From: Jilles Tjoelker To: Garrett Cooper 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> References: <201602040908.u1498buB006713@repo.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201602040908.u1498buB006713@repo.freebsd.org> User-Agent: Mutt/1.5.23 (2014-03-12) X-BeenThere: svn-src-user@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: "SVN commit messages for the experimental " user" src tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 05 Feb 2016 23:48:41 -0000 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