svn commit: r295247 - user/ngie/bsnmp_cleanup/contrib/bsnmp/lib

[NGie Cooper]

> On Feb 5, 2016, at 15:48, Jilles Tjoelker <jilles at stack.nl> wrote:
> 
> 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.

EEXIST isn’t documented as a valid error with bind(2):

     The following errors are specific to binding addresses in the UNIX
     domain.

     [ENOTDIR]    A component of the path prefix is not a directory.

     [ENAMETOOLONG]
                  A component of a pathname exceeded 255 characters, or an
                  entire path name exceeded 1023 characters.

     [ENOENT]     A prefix component of the path name does not exist.

     [ELOOP]      Too many symbolic links were encountered in translating the
                  pathname.

     [EIO]        An I/O error occurred while making the directory entry or
                  allocating the inode.

     [EROFS]      The name would reside on a read-only file system.

     [EISDIR]     An empty pathname was specified.

Testing it out, it fails with EADDRINUSE — yeah, bad choice. I’ll revert the commit and mute the warning.
Thanks!
-Ngie

$ cat ~/test_bind.c 
#include <sys/types.h>
#include <sys/socket.h>
#include <sys/un.h>
#include <err.h>
#include <fcntl.h>
#include <string.h>
#include <unistd.h>

#define LOCAL_SOCK      "/tmp/my.sock"

int
main(void)
{
        struct sockaddr_un n;
        int s;

        close(open(LOCAL_SOCK, O_CREAT|O_WRONLY));

        s = socket(AF_LOCAL, SOCK_DGRAM, 0);
        if (s == -1)
                err(1, "socket");

        n.sun_family = AF_LOCAL;
        strlcpy(n.sun_path, LOCAL_SOCK, sizeof(n.sun_path));

        if (bind(s, (struct sockaddr*)&n, SUN_LEN(&n)) == -1)
                err(1, "bind");

        unlink(LOCAL_SOCK);

        close(s);

        return (0);
}
$ ~/test_bind 
test_bind: bind: Address already in use