[PATCH] Protect messaging_dgm against bug 13150

[Volker Lendecke]

Hi!

Bug 13150 could happen to messaging_dgm too. Protect against that.

Review appreciated!

Thanks, Volker

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de
-------------- next part --------------
From c725a74076d91a2a993a651d5a19cb622ffbc098 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sat, 25 Nov 2017 16:47:24 +0100
Subject: [PATCH] messaging_dgm: Protect against fork without reinit

In the wake of bug 13150 we've discussed that this could happen even
without clustering. This adds code to make sure that whenever messaging
is used the pid and the files used match.

It's pretty heavy-weight, thus I made it DEVELOPER only. My gut feeling
is that the getsockname is cheap, but the stat call might be a bit too
expensive.

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/lib/messages_dgm.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 90 insertions(+)

diff --git a/source3/lib/messages_dgm.c b/source3/lib/messages_dgm.c
index 9d87746fa2c..15c83f75c47 100644
--- a/source3/lib/messages_dgm.c
+++ b/source3/lib/messages_dgm.c
@@ -1138,6 +1138,88 @@ static int messaging_dgm_context_destructor(struct messaging_dgm_context *c)
 	return 0;
 }
 
+static void messaging_dgm_validate(struct messaging_dgm_context *ctx)
+{
+#ifdef DEVELOPER
+	pid_t pid = getpid();
+	struct sockaddr_storage addr;
+	socklen_t addrlen = sizeof(addr);
+	struct sockaddr_un *un_addr;
+	struct sun_path_buf pathbuf;
+	struct stat st1, st2;
+	int ret;
+
+	/*
+	 * Protect against using the wrong messaging context after a
+	 * fork without reinit_after_fork.
+	 */
+
+	ret = getsockname(ctx->sock, (struct sockaddr *)&addr, &addrlen);
+	if (ret == -1) {
+		DBG_ERR("getsockname failed: %s\n", strerror(errno));
+		goto fail;
+	}
+	if (addr.ss_family != AF_UNIX) {
+		DBG_ERR("getsockname returned family %d\n",
+			(int)addr.ss_family);
+		goto fail;
+	}
+	un_addr = (struct sockaddr_un *)&addr;
+
+	ret = snprintf(pathbuf.buf, sizeof(pathbuf.buf),
+		       "%s/%u", ctx->socket_dir.buf, (int)pid);
+	if (ret < 0) {
+		DBG_ERR("snprintf failed: %s\n", strerror(errno));
+		goto fail;
+	}
+	if ((size_t)ret >= sizeof(pathbuf.buf)) {
+		DBG_ERR("snprintf returned %d chars\n", (int)ret);
+		goto fail;
+	}
+
+	if (strcmp(pathbuf.buf, un_addr->sun_path) != 0) {
+		DBG_ERR("sockname wrong: Expected %s, got %s\n",
+			pathbuf.buf, un_addr->sun_path);
+		goto fail;
+	}
+
+	ret = snprintf(pathbuf.buf, sizeof(pathbuf.buf),
+		       "%s/%u", ctx->lockfile_dir.buf, (int)pid);
+	if (ret < 0) {
+		DBG_ERR("snprintf failed: %s\n", strerror(errno));
+		goto fail;
+	}
+	if ((size_t)ret >= sizeof(pathbuf.buf)) {
+		DBG_ERR("snprintf returned %d chars\n", (int)ret);
+		goto fail;
+	}
+
+	ret = stat(pathbuf.buf, &st1);
+	if (ret == -1) {
+		DBG_ERR("stat failed: %s\n", strerror(errno));
+		goto fail;
+	}
+	ret = fstat(ctx->lockfile_fd, &st2);
+	if (ret == -1) {
+		DBG_ERR("fstat failed: %s\n", strerror(errno));
+		goto fail;
+	}
+
+	if ((st1.st_dev != st2.st_dev) || (st1.st_ino != st2.st_ino)) {
+		DBG_ERR("lockfile differs, expected (%d/%d), got (%d/%d)\n",
+			(int)st2.st_dev, (int)st2.st_ino,
+			(int)st1.st_dev, (int)st1.st_ino);
+		goto fail;
+	}
+
+	return;
+fail:
+	abort();
+#else
+	return;
+#endif
+}
+
 static void messaging_dgm_recv(struct messaging_dgm_context *ctx,
 			       struct tevent_context *ev,
 			       uint8_t *msg, size_t msg_len,
@@ -1162,6 +1244,8 @@ static void messaging_dgm_read_handler(struct tevent_context *ev,
 	uint8_t msgbuf[msgbufsize];
 	uint8_t buf[MESSAGING_DGM_FRAGMENT_LENGTH];
 
+	messaging_dgm_validate(ctx);
+
 	if ((flags & TEVENT_FD_READ) == 0) {
 		return;
 	}
@@ -1336,6 +1420,8 @@ int messaging_dgm_send(pid_t pid,
 		return ENOTCONN;
 	}
 
+	messaging_dgm_validate(ctx);
+
 	ret = messaging_dgm_out_get(ctx, pid, &out);
 	if (ret != 0) {
 		return ret;
@@ -1385,6 +1471,8 @@ int messaging_dgm_get_unique(pid_t pid, uint64_t *unique)
 		return EBADF;
 	}
 
+	messaging_dgm_validate(ctx);
+
 	if (pid == getpid()) {
 		/*
 		 * Protect against losing our own lock
@@ -1486,6 +1574,8 @@ int messaging_dgm_wipe(void)
 		return ENOTCONN;
 	}
 
+	messaging_dgm_validate(ctx);
+
 	/*
 	 * We scan the socket directory and not the lock directory. Otherwise
 	 * we would race against messaging_dgm_lockfile_create's open(O_CREAT)
-- 
2.11.0