| Message ID | 20211030164432.1140896-14-jeremy@azazel.net |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | Compiler Warning Fixes | expand |
On Saturday 2021-10-30 18:44, Jeremy Sowden wrote: >If the path is already bound, we close the socket immediately. >diff --git a/input/packet/ulogd_inppkt_UNIXSOCK.c b/input/packet/ulogd_inppkt_UNIXSOCK.c >index f97c2e174b2d..d88609f203c4 100644 >--- a/input/packet/ulogd_inppkt_UNIXSOCK.c >+++ b/input/packet/ulogd_inppkt_UNIXSOCK.c >@@ -479,10 +479,17 @@ static int _create_unix_socket(const char *unix_path) > int s; > struct stat st_dummy; > >+ if (stat(unix_path, &st_dummy) == 0 && st_dummy.st_size > 0) { >+ ulogd_log(ULOGD_ERROR, >+ "ulogd2: unix socket '%s' already exists\n", >+ unix_path); >+ return -1; >+ } >+ That stat call should just be entirely deleted. I fully expect that Coverity's static analyzer (or something like it) is going to flag this piece of code as running afoul of TOCTOU. A foreign event may cause the socket to come into existence between stat() and bind(), and therefore the code needs to handle the bind(2) failure _in any case_, and can report "Oh but it exists" at that time. So even if the stat call is fine from a security POV, it is redundant code.
On 2021-10-30, at 19:33:13 +0200, Jan Engelhardt wrote: > On Saturday 2021-10-30 18:44, Jeremy Sowden wrote: > > If the path is already bound, we close the socket immediately. > > diff --git a/input/packet/ulogd_inppkt_UNIXSOCK.c b/input/packet/ulogd_inppkt_UNIXSOCK.c > > index f97c2e174b2d..d88609f203c4 100644 > > --- a/input/packet/ulogd_inppkt_UNIXSOCK.c > > +++ b/input/packet/ulogd_inppkt_UNIXSOCK.c > > @@ -479,10 +479,17 @@ static int _create_unix_socket(const char *unix_path) > > int s; > > struct stat st_dummy; > > > > + if (stat(unix_path, &st_dummy) == 0 && st_dummy.st_size > 0) { > > + ulogd_log(ULOGD_ERROR, > > + "ulogd2: unix socket '%s' already exists\n", > > + unix_path); > > + return -1; > > + } > > + > > That stat call should just be entirely deleted. > > I fully expect that Coverity's static analyzer (or something like it) > is going to flag this piece of code as running afoul of TOCTOU. Good point. Will remove it instead. J.
diff --git a/input/packet/ulogd_inppkt_UNIXSOCK.c b/input/packet/ulogd_inppkt_UNIXSOCK.c index f97c2e174b2d..d88609f203c4 100644 --- a/input/packet/ulogd_inppkt_UNIXSOCK.c +++ b/input/packet/ulogd_inppkt_UNIXSOCK.c @@ -479,10 +479,17 @@ static int _create_unix_socket(const char *unix_path) int s; struct stat st_dummy; + if (stat(unix_path, &st_dummy) == 0 && st_dummy.st_size > 0) { + ulogd_log(ULOGD_ERROR, + "ulogd2: unix socket '%s' already exists\n", + unix_path); + return -1; + } + s = socket(AF_UNIX, SOCK_STREAM, 0); if (s < 0) { ulogd_log(ULOGD_ERROR, - "ulogd2: could not create unix socket\n"); + "ulogd2: could not create unix socket\n"); return -1; } @@ -490,19 +497,11 @@ static int _create_unix_socket(const char *unix_path) strncpy(server_sock.sun_path, unix_path, sizeof(server_sock.sun_path)); server_sock.sun_path[sizeof(server_sock.sun_path)-1] = '\0'; - if (stat(unix_path, &st_dummy) == 0 && st_dummy.st_size > 0) { - ulogd_log(ULOGD_ERROR, - "ulogd2: unix socket \'%s\' already exists\n", - unix_path); - close(s); - return -1; - } - ret = bind(s, (struct sockaddr *)&server_sock, sizeof(server_sock)); if (ret < 0) { ulogd_log(ULOGD_ERROR, - "ulogd2: could not bind to unix socket \'%s\'\n", - server_sock.sun_path); + "ulogd2: could not bind to unix socket '%s'\n", + server_sock.sun_path); close(s); return -1; } @@ -510,8 +509,8 @@ static int _create_unix_socket(const char *unix_path) ret = listen(s, 10); if (ret < 0) { ulogd_log(ULOGD_ERROR, - "ulogd2: could not bind to unix socket \'%s\'\n", - server_sock.sun_path); + "ulogd2: could not listen to unix socket '%s'\n", + server_sock.sun_path); close(s); return -1; }
If the path is already bound, we close the socket immediately. A couple of error message fixes. Signed-off-by: Jeremy Sowden <jeremy@azazel.net> --- input/packet/ulogd_inppkt_UNIXSOCK.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-)