229 lines
7.2 KiB
Diff
229 lines
7.2 KiB
Diff
From 6a4067c1d1764d93d255eccecfd8bf9f43cb0b4d Mon Sep 17 00:00:00 2001
|
|
From: Christine Caulfield <ccaulfie@redhat.com>
|
|
Date: Mon, 8 Apr 2019 16:24:19 +0100
|
|
Subject: [PATCH] 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 | 4 +++-
|
|
lib/ipc_setup.c | 39 +++++++++++++++++++++++++++++++++++++++
|
|
lib/ipc_shm.c | 9 ++++++---
|
|
lib/ipc_socket.c | 13 ++++++++++---
|
|
lib/ipcs.c | 3 ++-
|
|
lib/ringbuffer.c | 4 ++--
|
|
lib/unix.c | 4 +++-
|
|
7 files changed, 65 insertions(+), 11 deletions(-)
|
|
|
|
diff --git a/lib/ipc_int.h b/lib/ipc_int.h
|
|
index 9cd06cfe..c8904487 100644
|
|
--- a/lib/ipc_int.h
|
|
+++ b/lib/ipc_int.h
|
|
@@ -161,7 +161,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;
|
|
@@ -208,4 +208,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 */
|
|
diff --git a/lib/ipc_setup.c b/lib/ipc_setup.c
|
|
index 0e169643..43dc3e78 100644
|
|
--- a/lib/ipc_setup.c
|
|
+++ b/lib/ipc_setup.c
|
|
@@ -643,8 +643,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,
|
|
@@ -865,3 +885,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
|
|
+}
|
|
diff --git a/lib/ipc_shm.c b/lib/ipc_shm.c
|
|
index 9f237b6e..758a2b51 100644
|
|
--- a/lib/ipc_shm.c
|
|
+++ b/lib/ipc_shm.c
|
|
@@ -265,6 +265,9 @@ qb_ipcs_shm_disconnect(struct qb_ipcs_connection *c)
|
|
c->setup.u.us.sock = -1;
|
|
}
|
|
}
|
|
+
|
|
+ remove_tempdir(c->description, CONNECTION_DESCRIPTION);
|
|
+
|
|
end_disconnect:
|
|
sigaction(SIGBUS, &old_sa, NULL);
|
|
}
|
|
@@ -313,11 +316,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);
|
|
diff --git a/lib/ipc_socket.c b/lib/ipc_socket.c
|
|
index 1f7cde38..59492323 100644
|
|
--- a/lib/ipc_socket.c
|
|
+++ b/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,
|
|
diff --git a/lib/ipcs.c b/lib/ipcs.c
|
|
index 4a375fca..29f3431b 100644
|
|
--- a/lib/ipcs.c
|
|
+++ b/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
|
|
diff --git a/lib/ringbuffer.c b/lib/ringbuffer.c
|
|
index 8852ff5b..f85ad979 100644
|
|
--- a/lib/ringbuffer.c
|
|
+++ b/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);
|
|
diff --git a/lib/unix.c b/lib/unix.c
|
|
index 3c8f327c..49701a33 100644
|
|
--- a/lib/unix.c
|
|
+++ b/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;
|