From 158bf2e77ce26ce3679f55fc7a46aeade3d92413 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Mon, 8 Sep 2014 20:01:20 +0100 Subject: [PATCH] _dbus_read_socket_with_unix_fds: do not accept extra fds in cmsg padding If (*n_fds * sizeof (int) % sizeof (size_t)) is nonzero, then CMSG_SPACE (*n_fds * sizeof (int)) > CMSG_LEN (*n_fds * sizeof (int) because the SPACE includes padding to a size_t boundary, whereas the LEN does not. We have to allocate the SPACE. Previously, we told the kernel that the buffer size we wanted was the SPACE, not the LEN, which meant it was free to fill the padding with additional fds: on a 64-bit platform with 32-bit int, that's one extra fd, if *n_fds happens to be odd. This meant that a malicious sender could send exactly 1 fd too many, which would make us fail an assertion if enabled, or overrun a buffer by 1 fd otherwise. In practice, *n_fds == max_message_unix_fds, which defaults to an even number and is unlikely to be set to an odd number. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=83622 --- dbus/dbus-sysdeps-unix.c | 36 +++++++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 5 deletions(-) diff --git a/dbus/dbus-sysdeps-unix.c b/dbus/dbus-sysdeps-unix.c index 170d865..1b52375 100644 --- a/dbus/dbus-sysdeps-unix.c +++ b/dbus/dbus-sysdeps-unix.c @@ -323,6 +323,12 @@ _dbus_read_socket_with_unix_fds (int fd, m.msg_control = alloca(m.msg_controllen); memset(m.msg_control, 0, m.msg_controllen); + /* Do not include the padding at the end when we tell the kernel + * how much we're willing to receive. This avoids getting + * the padding filled with additional fds that we weren't expecting, + * if a (potentially malicious) sender included them. (fd.o #83622) */ + m.msg_controllen = CMSG_LEN (*n_fds * sizeof(int)); + again: bytes_read = recvmsg(fd, &m, 0 @@ -362,18 +368,38 @@ _dbus_read_socket_with_unix_fds (int fd, for (cm = CMSG_FIRSTHDR(&m); cm; cm = CMSG_NXTHDR(&m, cm)) if (cm->cmsg_level == SOL_SOCKET && cm->cmsg_type == SCM_RIGHTS) { - unsigned i; + size_t i; + int *payload = (int *) CMSG_DATA (cm); + size_t payload_len_bytes = (cm->cmsg_len - CMSG_LEN (0)); + size_t payload_len_fds = payload_len_bytes / sizeof (int); - _dbus_assert(cm->cmsg_len <= CMSG_LEN(*n_fds * sizeof(int))); - *n_fds = (cm->cmsg_len - CMSG_LEN(0)) / sizeof(int); + if (_DBUS_LIKELY (payload_len_fds <= (size_t) *n_fds)) + { + *n_fds = (int) payload_len_fds; + } + else + { + /* Shouldn't happen any more because we're setting + * m.msg_controllen to the exact number we can accept, + * but be safe and truncate the array. Close the + * excess fds to avoid DoS: if they stayed open, + * someone could send us an extra fd per message + * and we'd eventually run out. */ + for (i = (size_t) *n_fds; i < payload_len_fds; i++) + { + close (payload[i]); + } + + payload_len_fds = (size_t) *n_fds; + } - memcpy(fds, CMSG_DATA(cm), *n_fds * sizeof(int)); + memcpy(fds, payload, *n_fds * sizeof(int)); found = TRUE; /* Linux doesn't tell us whether MSG_CMSG_CLOEXEC actually worked, hence we need to go through this list and set CLOEXEC everywhere in any case */ - for (i = 0; i < *n_fds; i++) + for (i = 0; i < payload_len_fds; i++) _dbus_fd_set_close_on_exec(fds[i]); break; -- 2.1.0