[PATCH] Protect messaging_dgm against bug 13150

[Jeremy Allison]

On Mon, Nov 27, 2017 at 12:18:17PM +0100, Volker Lendecke via samba-technical wrote:
> Hi!
> 
> Bug 13150 could happen to messaging_dgm too. Protect against that.
> 
> Review appreciated!

LGTM. RB+ with a couple of changes from (int) -> (unsigned) when
creating the paths using (pid) to match the %u format. Pushed.

> -- 
> 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

> 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
>