privsep: enforce message boundaries with MSG_EOR on our messages (#533)

privsep: enforce message boundaries with MSG_EOR on our messages

The nature of the SOCK_SEQPACKET, that privsep modules uses, is stream.
See:

https://pubs.opengroup.org/onlinepubs/9799919799/functions/V2_chap02.html#tag_16_10_06

To guarantee that a reader will never read two messages in one read
operation, the writer shall put end of record markers.

The problem exposed itself in FreeBSD 15.0 that started to follow the
specification better than before.

Other SOCK_SEQPACKET usage considerations: a) as long as our reader
provides a receive buffer that would fit the largest message our writer
would ever send, we are good with regards to not a reading a partial
message b) as long as our writer always write full messages with one
write, we don't need use of MSG_WAITALL in reader.

Fixes #530

Co-authored-by: Roy Marples <roy@marples.name>

diff --git a/src/dhcpcd.c b/src/dhcpcd.c
index 3e65141e164224dcc1076ad28bfb1844541d6b68..0251ee85590b52df62d2301a8fa22ef3d93cb068 100644 (file)
--- a/src/dhcpcd.c
+++ b/src/dhcpcd.c
@@ -393,7 +393,7 @@ dhcpcd_daemonise(struct dhcpcd_ctx *ctx)
 
        eloop_event_delete(ctx->eloop, ctx->fork_fd);
        exit_code = EXIT_SUCCESS;
-       if (write(ctx->fork_fd, &exit_code, sizeof(exit_code)) == -1)
+       if (send(ctx->fork_fd, &exit_code, sizeof(exit_code), MSG_EOR) == -1)
                logerr(__func__);
        close(ctx->fork_fd);
        ctx->fork_fd = -1;
@@ -1449,8 +1449,8 @@ dhcpcd_signal_cb(int sig, void *arg)
 
        if (sig != SIGCHLD && ctx->options & DHCPCD_FORKED) {
                if (sig != SIGHUP &&
-                   write(ctx->fork_fd, &sig, sizeof(sig)) == -1)
-                       logerr("%s: write", __func__);
+                   send(ctx->fork_fd, &sig, sizeof(sig), MSG_EOR) == -1)
+                       logerr("%s: send", __func__);
                return;
        }
 
@@ -2712,8 +2712,8 @@ exit1:
 #ifdef USE_SIGNALS
        /* If still attached, detach from the launcher */
        if (ctx.options & DHCPCD_STARTED && ctx.fork_fd != -1) {
-               if (write(ctx.fork_fd, &i, sizeof(i)) == -1)
-                       logerr("%s: write", __func__);
+               if (send(ctx.fork_fd, &i, sizeof(i), MSG_EOR) == -1)
+                       logerr("%s: send", __func__);
        }
 #endif
 

diff --git a/src/logerr.c b/src/logerr.c
index 98cd1c7e6c6299a20e0607c858372b2dda845c1a..7e8038bda003283c65af0e324076934841c63852 100644 (file)
--- a/src/logerr.c
+++ b/src/logerr.c
@@ -26,7 +26,9 @@
  * SUCH DAMAGE.
  */
 
+#include <sys/socket.h>
 #include <sys/time.h>
+
 #include <errno.h>
 #include <stdbool.h>
 #include <stdarg.h>
@@ -215,18 +217,25 @@ vlogmessage(int pri, const char *fmt, va_list args)
        int len = 0;
 
        if (ctx->log_fd != -1) {
+               pid_t pid = getpid();
                char buf[LOGERR_SYSLOGBUF];
-               pid_t pid;
-
-               memcpy(buf, &pri, sizeof(pri));
-               pid = getpid();
-               memcpy(buf + sizeof(pri), &pid, sizeof(pid));
-               len = vsnprintf(buf + sizeof(pri) + sizeof(pid),
-                   sizeof(buf) - sizeof(pri) - sizeof(pid),
-                   fmt, args);
-               if (len != -1)
-                       len = (int)write(ctx->log_fd, buf,
-                           ((size_t)++len) + sizeof(pri) + sizeof(pid));
+               struct iovec iov[] = {
+                       { .iov_base = &pri, .iov_len = sizeof(pri) },
+                       { .iov_base = &pid, .iov_len = sizeof(pid) },
+                       { .iov_base = buf },
+               };
+
+               len = vsnprintf(buf, sizeof(buf), fmt, args);
+               if (len != -1) {
+                       if ((size_t)len >= sizeof(buf))
+                               len = (int)sizeof(buf) - 1;
+                       iov[2].iov_len = (size_t)(len + 1);
+                       struct msghdr msg = {
+                               .msg_iov = iov,
+                               .msg_iovlen = sizeof(iov) / sizeof(iov[0]),
+                       };
+                       len = (int)sendmsg(ctx->log_fd, &msg, MSG_EOR);
+               }
                return len;
        }
 
@@ -390,24 +399,33 @@ int
 logreadfd(int fd)
 {
        struct logctx *ctx = &_logctx;
-       char buf[LOGERR_SYSLOGBUF];
        int len, pri;
-
-       len = (int)read(fd, buf, sizeof(buf));
-       if (len == -1)
+       pid_t pid;
+       char buf[LOGERR_SYSLOGBUF] = { '\0' };
+       struct iovec iov[] = {
+               { .iov_base = &pri, .iov_len = sizeof(pri) },
+               { .iov_base = &pid, .iov_len = sizeof(pid) },
+               { .iov_base = buf,  .iov_len = sizeof(buf) },
+       };
+       struct msghdr msg = {
+               .msg_iov = iov,
+               .msg_iovlen = sizeof(iov) / sizeof(iov[0])
+       };
+
+       len = (int)recvmsg(fd, &msg, MSG_WAITALL);
+       if (len == -1 || len == 0)
                return -1;
-
-       /* Ensure we have pri, pid and a terminator */
-       if (len < (int)(sizeof(pri) + sizeof(pid_t) + 1) ||
-           buf[len - 1] != '\0')
-       {
-               errno = EINVAL;
+       /* Ensure we received the minimum and at least one character to log */
+       if ((size_t)len < sizeof(pri) + sizeof(pid) + 1 ||
+           msg.msg_flags & MSG_TRUNC) {
+               errno = EMSGSIZE;
                return -1;
        }
+       /* Ensure what we receive is NUL terminated */
+       buf[(size_t)len - (sizeof(pri) + sizeof(pid)) - 1] = '\0';
 
-       memcpy(&pri, buf, sizeof(pri));
-       memcpy(&ctx->log_pid, buf + sizeof(pri), sizeof(ctx->log_pid));
-       logmessage(pri, "%s", buf + sizeof(pri) + sizeof(ctx->log_pid));
+       ctx->log_pid = pid;
+       logmessage(pri, "%s", buf);
        ctx->log_pid = 0;
        return len;
 }

diff --git a/src/logerr.h b/src/logerr.h
index 08c3c133a938355196840e6ae53e633dcda7998e..a6507c26e9178ff386bcccb714e604ee33ba576d 100644 (file)
--- a/src/logerr.h
+++ b/src/logerr.h
@@ -76,7 +76,7 @@ __printflike(2, 3) void logerrmessage(int pri, const char *fmt, ...);
 #define logerr(...)    log_err(__VA_ARGS__)
 #define logerrx(...)   log_errx(__VA_ARGS__)
 
-/* For logging in a chroot */
+/* For logging in a chroot using SOCK_SEQPACKET */
 int loggetfd(void);
 void logsetfd(int);
 int logreadfd(int);

diff --git a/src/privsep-root.c b/src/privsep-root.c
index 4e6b9538f1a018cdb45282381b70b5189686023d..35dec006d740b2a1c5eb15e86dbdc349489ceb01 100644 (file)
--- a/src/privsep-root.c
+++ b/src/privsep-root.c
@@ -210,6 +210,7 @@ ps_root_writeerror(struct dhcpcd_ctx *ctx, ssize_t result,
                { .iov_base = &psr, .iov_len = sizeof(psr) },
                { .iov_base = data, .iov_len = len },
        };
+       struct msghdr msg = { .msg_iov = iov, .msg_iovlen = __arraycount(iov) };
        ssize_t err;
        int fd = PS_ROOT_FD(ctx);
 
@@ -217,7 +218,7 @@ ps_root_writeerror(struct dhcpcd_ctx *ctx, ssize_t result,
        logdebugx("%s: result %zd errno %d", __func__, result, errno);
 #endif
 
-       err = writev(fd, iov, __arraycount(iov));
+       err = sendmsg(fd, &msg, MSG_EOR);
 
        /* Error sending the message? Try sending the error of sending. */
        if (err == -1) {
@@ -227,7 +228,7 @@ ps_root_writeerror(struct dhcpcd_ctx *ctx, ssize_t result,
                psr.psr_errno = errno;
                iov[1].iov_base = NULL;
                iov[1].iov_len = 0;
-               err = writev(fd, iov, __arraycount(iov));
+               err = sendmsg(fd, &msg, MSG_EOR);
        }
 
        return err;

diff --git a/src/privsep.c b/src/privsep.c
index 7b6fbbbc3a514fe5d30e7361f83edfec4b640639..380a4f6231f732359e282e31fbd609bbf1a587c7 100644 (file)
--- a/src/privsep.c
+++ b/src/privsep.c
@@ -895,7 +895,7 @@ ps_sendpsmmsg(struct dhcpcd_ctx *ctx, int fd,
                { .iov_base = NULL, },  /* payload 2 */
                { .iov_base = NULL, },  /* payload 3 */
        };
-       int iovlen;
+       struct msghdr m = { .msg_iov = iov, .msg_iovlen = 1 };
        ssize_t len;
 
        if (msg != NULL) {
@@ -909,6 +909,7 @@ ps_sendpsmmsg(struct dhcpcd_ctx *ctx, int fd,
                iovp->iov_base = msg->msg_name;
                iovp->iov_len = msg->msg_namelen;
                iovp++;
+               m.msg_iovlen++;
 
                cmsg_padlen =
                    CALC_CMSG_PADLEN(msg->msg_controllen, msg->msg_namelen);
@@ -916,25 +917,26 @@ ps_sendpsmmsg(struct dhcpcd_ctx *ctx, int fd,
                iovp->iov_len = cmsg_padlen;
                iovp->iov_base = cmsg_padlen != 0 ? padding : NULL;
                iovp++;
+               m.msg_iovlen++;
 
                iovp->iov_base = msg->msg_control;
                iovp->iov_len = msg->msg_controllen;
-               iovlen = 4;
+               iovp++;
+               m.msg_iovlen++;
 
                for (i = 0; i < (int)msg->msg_iovlen; i++) {
-                       if ((size_t)(iovlen + i) > __arraycount(iov)) {
+                       if ((size_t)(m.msg_iovlen++) > __arraycount(iov)) {
                                errno = ENOBUFS;
                                return -1;
                        }
-                       iovp++;
                        iovp->iov_base = msg->msg_iov[i].iov_base;
                        iovp->iov_len = msg->msg_iov[i].iov_len;
+                       iovp++;
                }
-               iovlen += i;
-       } else
-               iovlen = 1;
+       }
+
+       len = sendmsg(fd, &m, MSG_EOR);
 
-       len = writev(fd, iov, iovlen);
        if (len == -1) {
                if (ctx->options & DHCPCD_FORKED &&
                    !(ctx->options & DHCPCD_PRIVSEPROOT))
@@ -1028,6 +1030,7 @@ ps_sendcmdmsg(int fd, uint16_t cmd, const struct msghdr *msg)
                { .iov_base = &psm, .iov_len = sizeof(psm) },
                { .iov_base = data, .iov_len = 0 },
        };
+       struct msghdr m = { .msg_iov = iov, .msg_iovlen = __arraycount(iov) };
        size_t dl = sizeof(data);
        socklen_t cmsg_padlen =
            CALC_CMSG_PADLEN(msg->msg_controllen, msg->msg_namelen);
@@ -1063,7 +1066,8 @@ ps_sendcmdmsg(int fd, uint16_t cmd, const struct msghdr *msg)
            psm.ps_namelen + psm.ps_controllen + psm.ps_datalen + cmsg_padlen;
        if (psm.ps_datalen != 0)
                memcpy(p, msg->msg_iov[0].iov_base, psm.ps_datalen);
-       return writev(fd, iov, __arraycount(iov));
+
+       return sendmsg(fd, &m, MSG_EOR);
 
 nobufs:
        errno = ENOBUFS;
@@ -1089,7 +1093,7 @@ ps_recvmsg(int rfd, unsigned short events, uint16_t cmd, int wfd)
        if (!(events & ELE_READ))
                logerrx("%s: unexpected event 0x%04x", __func__, events);
 
-       len = recvmsg(rfd, &msg, 0);
+       len = recvmsg(rfd, &msg, MSG_WAITALL);
        if (len == -1) {
                logerr("%s: recvmsg", __func__);
                return len;