On 11/11/2019 11:41 AM, Stefan Hajnoczi wrote:
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.
We will add a check to ensure that msg->size is less than msg->data1 if
msg->bytestream is not set.
2. An attacker can allocate data2, all mpqemu_msg_recv() callers
need to free it to prevent memory leaks.
We will address this memory leak.
3. mpqemu_msg_recv() callers generally do not validate untrusted msg
fields. All the code needs to be audited.
mpqemu_msg_recv() callers validate the num_fds field. But we will add
more fields for validation by the callers.
Thanks!
--
Jag
Stefan
