On Thu, Oct 24, 2019 at 05:08:48AM -0400, Jagannathan Raman wrote:
> +int mpqemu_msg_recv(MPQemuLinkState *s, MPQemuMsg *msg, MPQemuChannel *chan)
> +{
> + int rc;
> + uint8_t *data;
> + union {
> + char control[CMSG_SPACE(REMOTE_MAX_FDS * sizeof(int))];
> + struct cmsghdr align;
> + } u;
> + struct msghdr hdr;
> + struct cmsghdr *chdr;
> + size_t fdsize;
> + int sock = chan->sock;
> + QemuMutex *lock = &chan->recv_lock;
> +
> + struct iovec iov = {
> + .iov_base = (char *) msg,
> + .iov_len = MPQEMU_MSG_HDR_SIZE,
> + };
> +
> + memset(&hdr, 0, sizeof(hdr));
> + memset(&u, 0, sizeof(u));
> +
> + hdr.msg_iov = &iov;
> + hdr.msg_iovlen = 1;
> + hdr.msg_control = &u;
> + hdr.msg_controllen = sizeof(u);
> +
> + qemu_mutex_lock(lock);
> +
> + do {
> + rc = recvmsg(sock, &hdr, 0);
> + } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
> +
> + if (rc < 0) {
> + qemu_log_mask(LOG_REMOTE_DEBUG, "%s - recvmsg rc is %d, errno is %d,"
> + " sock %d\n", __func__, rc, errno, sock);
> + qemu_mutex_unlock(lock);
> + return rc;
> + }
> +
> + msg->num_fds = 0;
> + for (chdr = CMSG_FIRSTHDR(&hdr); chdr != NULL;
> + chdr = CMSG_NXTHDR(&hdr, chdr)) {
> + if ((chdr->cmsg_level == SOL_SOCKET) &&
> + (chdr->cmsg_type == SCM_RIGHTS)) {
> + fdsize = chdr->cmsg_len - CMSG_LEN(0);
> + msg->num_fds = fdsize / sizeof(int);
> + if (msg->num_fds > REMOTE_MAX_FDS) {
> + /*
> + * TODO: Security issue detected. Sender never sends more
> + * than REMOTE_MAX_FDS. This condition should be signaled to
> + * the admin
> + */
> + qemu_log_mask(LOG_REMOTE_DEBUG, "%s: Max FDs exceeded\n",
> __func__);
> + return -ERANGE;
> + }
> +
> + memcpy(msg->fds, CMSG_DATA(chdr), fdsize);
> + break;
> + }
> + }
> +
> + if (msg->size && msg->bytestream) {
> + msg->data2 = calloc(1, msg->size);
> + data = msg->data2;
> + } else {
> + data = (uint8_t *)&msg->data1;
> + }
> +
> + if (msg->size) {
> + do {
> + rc = read(sock, data, msg->size);
> + } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
> + }
> +
> + qemu_mutex_unlock(lock);
> +
> + return rc;
> +}This code is still insecure. Until the communication between processes is made secure this series does not meet its goal of providing process isolation. 1. An attacker can overflow msg->data1 easily by setting msg->size but not msg->bytestream. 2. An attacker can allocate data2, all mpqemu_msg_recv() callers need to free it to prevent memory leaks. 3. mpqemu_msg_recv() callers generally do not validate untrusted msg fields. All the code needs to be audited. Stefan
signature.asc
Description: PGP signature
