ipc: Use mkdtemp for more secure IPC files

Use mkdtemp makes sure that IPC files are only visible to the
owning (client) process and do not use predictable names outside
of that.

This is not meant to be the last word on the subject, it's mainly a
simple way of making the current libqb more secure. Importantly, it's
backwards compatible with an old server.

It calls rmdir on the directory created by mkdtemp way too often, but
it seems to be the only way to be sure that things get cleaned up on
the various types of server/client exit. I'm sure we can come up with
something tidier for master but I hope this, or something similar, will
be OK for 1.0.x.

lib/ipc_int.h

@@ -160,7 +160,7 @@ enum qb_ipcs_connection_state {
 	QB_IPCS_CONNECTION_SHUTTING_DOWN,
 };
 
-#define CONNECTION_DESCRIPTION (34) /* INT_MAX length + 3 */
+#define CONNECTION_DESCRIPTION NAME_MAX
 
 struct qb_ipcs_connection_auth {
 	uid_t uid;
@@ -207,4 +207,6 @@ int32_t qb_ipc_us_sock_error_is_disconnected(int err);
 
 int use_filesystem_sockets(void);
 
+void remove_tempdir(const char *name, size_t namelen);
+
 #endif /* QB_IPC_INT_H_DEFINED */

lib/ipc_setup.c

@@ -642,8 +642,28 @@ handle_new_connection(struct qb_ipcs_service *s,
 	c->auth.gid = c->egid = ugp->gid;
 	c->auth.mode = 0600;
 	c->stats.client_pid = ugp->pid;
+
+#if defined(QB_LINUX) || defined(QB_CYGWIN)
+	snprintf(c->description, CONNECTION_DESCRIPTION,
+		 "/dev/shm/qb-%d-%d-%d-XXXXXX", s->pid, ugp->pid, c->setup.u.us.sock);
+	if (mkdtemp(c->description) == NULL) {
+		res = errno;
+		goto send_response;
+	}
+	res = chown(c->description, c->auth.uid, c->auth.gid);
+	if (res != 0) {
+		res = errno;
+		goto send_response;
+	}
+
+	/* We can't pass just a directory spec to the clients */
+	strncat(c->description,"/qb", CONNECTION_DESCRIPTION);
+#else
 	snprintf(c->description, CONNECTION_DESCRIPTION,
 		 "%d-%d-%d", s->pid, ugp->pid, c->setup.u.us.sock);
+#endif
+
+
 
 	if (auth_result == 0 && c->service->serv_fns.connection_accept) {
 		res = c->service->serv_fns.connection_accept(c,
@@ -864,3 +884,22 @@ qb_ipcs_us_connection_acceptor(int fd, int revent, void *data)
 	qb_ipcs_uc_recv_and_auth(new_fd, s);
 	return 0;
 }
+
+void remove_tempdir(const char *name, size_t namelen)
+{
+#if defined(QB_LINUX) || defined(QB_CYGWIN)
+	char dirname[PATH_MAX];
+	char *slash;
+	memcpy(dirname, name, namelen);
+
+	slash = strrchr(dirname, '/');
+	if (slash) {
+		*slash = '\0';
+		/* This gets called more than it needs to be really, so we don't check
+		 * the return code. It's more of a desperate attempt to clean up after ourself
+		 * in either the server or client.
+		 */
+		(void)rmdir(dirname);
+	}
+#endif
+}

lib/ipc_shm.c

@@ -239,6 +239,8 @@ qb_ipcs_shm_disconnect(struct qb_ipcs_connection *c)
 			qb_rb_close(qb_rb_lastref_and_ret(&c->request.u.shm.rb));
 		}
 	}
+
+	remove_tempdir(c->description, CONNECTION_DESCRIPTION);
 }
 
 static int32_t
@@ -285,11 +287,11 @@ qb_ipcs_shm_connect(struct qb_ipcs_service *s,
 	qb_util_log(LOG_DEBUG, "connecting to client [%d]", c->pid);
 
 	snprintf(r->request, NAME_MAX, "%s-request-%s",
-		 s->name, c->description);
+		 c->description, s->name);
 	snprintf(r->response, NAME_MAX, "%s-response-%s",
-		 s->name, c->description);
+		 c->description, s->name);
 	snprintf(r->event, NAME_MAX, "%s-event-%s",
-		 s->name, c->description);
+		 c->description, s->name);
 
 	res = qb_ipcs_shm_rb_open(c, &c->request,
 				  r->request);

lib/ipc_socket.c

@@ -374,6 +374,10 @@ qb_ipcc_us_disconnect(struct qb_ipcc_connection *c)
 			free(base_name);
 		}
 	}
+
+	/* Last-ditch attempt to tidy up after ourself */
+	remove_tempdir(c->request.u.us.shared_file_name, PATH_MAX);
+
 	qb_ipcc_us_sock_close(c->event.u.us.sock);
 	qb_ipcc_us_sock_close(c->request.u.us.sock);
 	qb_ipcc_us_sock_close(c->setup.u.us.sock);
@@ -765,7 +769,10 @@ qb_ipcs_us_disconnect(struct qb_ipcs_connection *c)
 	    c->state == QB_IPCS_CONNECTION_ACTIVE) {
 		munmap(c->request.u.us.shared_data, SHM_CONTROL_SIZE);
 		unlink(c->request.u.us.shared_file_name);
+
+
 	}
+	remove_tempdir(c->description, CONNECTION_DESCRIPTION);
 }
 
 static int32_t
@@ -784,9 +791,9 @@ qb_ipcs_us_connect(struct qb_ipcs_service *s,
 	c->request.u.us.sock = c->setup.u.us.sock;
 	c->response.u.us.sock = c->setup.u.us.sock;
 
-	snprintf(r->request, NAME_MAX, "qb-%s-control-%s",
-		 s->name, c->description);
-	snprintf(r->response, NAME_MAX, "qb-%s-%s", s->name, c->description);
+	snprintf(r->request, NAME_MAX, "%s-control-%s",
+		 c->description, s->name);
+	snprintf(r->response, NAME_MAX, "%s-%s", c->description, s->name);
 
 	fd_hdr = qb_sys_mmap_file_open(path, r->request,
 				       SHM_CONTROL_SIZE,

lib/ipcs.c

@@ -642,12 +642,13 @@ qb_ipcs_disconnect(struct qb_ipcs_connection *c)
 				scheduled_retry = 1;
 			}
 		}
-
+		remove_tempdir(c->description, CONNECTION_DESCRIPTION);
 		if (scheduled_retry == 0) {
 			/* This removes the initial alloc ref */
 			qb_ipcs_connection_unref(c);
 		}
 	}
+
 }
 
 static void

lib/ringbuffer.c

@@ -166,7 +166,7 @@ qb_rb_open_2(const char *name, size_t size, uint32_t flags,
 	/*
 	 * Create a shared_hdr memory segment for the header.
 	 */
-	snprintf(filename, PATH_MAX, "qb-%s-header", name);
+	snprintf(filename, PATH_MAX, "%s-header", name);
 	fd_hdr = qb_sys_mmap_file_open(path, filename,
 				       shared_size, file_flags);
 	if (fd_hdr < 0) {
@@ -217,7 +217,7 @@ qb_rb_open_2(const char *name, size_t size, uint32_t flags,
 	 * They have to be separate.
 	 */
 	if (flags & QB_RB_FLAG_CREATE) {
-		snprintf(filename, PATH_MAX, "qb-%s-data", name);
+		snprintf(filename, PATH_MAX, "%s-data", name);
 		fd_data = qb_sys_mmap_file_open(path,
 						filename,
 						real_size, file_flags);

lib/unix.c

@@ -81,7 +81,9 @@ qb_sys_mmap_file_open(char *path, const char *file, size_t bytes,
 		(void)strlcpy(path, file, PATH_MAX);
 	} else {
 #if defined(QB_LINUX) || defined(QB_CYGWIN)
-		snprintf(path, PATH_MAX, "/dev/shm/%s", file);
+		/* This is only now called when talking to an old libqb
+		   where we need to add qb- to the name */
+		snprintf(path, PATH_MAX, "/dev/shm/qb-%s", file);
 #else
 		snprintf(path, PATH_MAX, "%s/%s", SOCKETDIR, file);
 		is_absolute = path;