Date: Sat, 6 Feb 2016 21:14:12 -0800 From: NGie Cooper <yaneurabeya@gmail.com> To: Jilles Tjoelker <jilles@stack.nl> Cc: Garrett Cooper <ngie@FreeBSD.org>, src-committers@freebsd.org, svn-src-user@freebsd.org Subject: Re: svn commit: r295247 - user/ngie/bsnmp_cleanup/contrib/bsnmp/lib Message-ID: <75BF6D6C-C696-4E25-BCAF-64016DE3C287@gmail.com> In-Reply-To: <20160205234838.GB97435@stack.nl> References: <201602040908.u1498buB006713@repo.freebsd.org> <20160205234838.GB97435@stack.nl>
next in thread | previous in thread | raw e-mail | index | archive | help
> On Feb 5, 2016, at 15:48, Jilles Tjoelker <jilles@stack.nl> wrote: >=20 > 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 >=20 >> Log: >> Use mkstemp(3) instead of mktemp(3) when creating temporary files to = fix >> the security pragma >=20 >> Modified: >> user/ngie/bsnmp_cleanup/contrib/bsnmp/lib/snmpclient.c >=20 >> Modified: user/ngie/bsnmp_cleanup/contrib/bsnmp/lib/snmpclient.c >> = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >> --- 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); >>=20 >> - if (mktemp(snmp_client.local_path) =3D=3D NULL) { >> + if (mkstemp(snmp_client.local_path) =3D=3D -1) { >> seterr(&snmp_client, "%s", strerror(errno)); >> (void)close(snmp_client.fd); >> snmp_client.fd =3D -1; >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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=E2=80=99t 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 =E2=80=94 yeah, bad choice. = I=E2=80=99ll revert the commit and mute the warning. Thanks! -Ngie $ cat ~/test_bind.c=20 #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 =3D socket(AF_LOCAL, SOCK_DGRAM, 0); if (s =3D=3D -1) err(1, "socket"); n.sun_family =3D AF_LOCAL; strlcpy(n.sun_path, LOCAL_SOCK, sizeof(n.sun_path)); if (bind(s, (struct sockaddr*)&n, SUN_LEN(&n)) =3D=3D -1) err(1, "bind"); unlink(LOCAL_SOCK); close(s); return (0); } $ ~/test_bind=20 test_bind: bind: Address already in use=
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?75BF6D6C-C696-4E25-BCAF-64016DE3C287>