[PATCH] Protect messaging_dgm against bug 13150

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Protect messaging_dgm against bug 13150

Samba - samba-technical mailing list
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:[hidden email]

patch.txt (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Protect messaging_dgm against bug 13150

Samba - samba-technical mailing list
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:[hidden email]

> From c725a74076d91a2a993a651d5a19cb622ffbc098 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <[hidden email]>
> 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 <[hidden email]>
> ---
>  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
>