Commit ee11ec12 authored by Simon McVittie's avatar Simon McVittie

_dbus_read_socket_with_unix_fds: do not accept extra fds in cmsg padding

This addresses CVE-2014-3635.

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.

Bug: https://bugs.freedesktop.org/show_bug.cgi?id=83622Reviewed-by: 's avatarAlban Crequy <alban.crequy@collabora.co.uk>
parent f70c0e98
......@@ -320,6 +320,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
......@@ -359,18 +365,49 @@ _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;
_dbus_assert(cm->cmsg_len <= CMSG_LEN(*n_fds * sizeof(int)));
*n_fds = (cm->cmsg_len - CMSG_LEN(0)) / sizeof(int);
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);
size_t fds_to_use;
/* Every non-negative int fits in a size_t without truncation,
* and we already know that *n_fds is non-negative, so
* casting (size_t) *n_fds is OK */
_DBUS_STATIC_ASSERT (sizeof (size_t) >= sizeof (int));
if (_DBUS_LIKELY (payload_len_fds <= (size_t) *n_fds))
{
/* The fds in the payload will fit in our buffer */
fds_to_use = payload_len_fds;
}
else
{
/* Too many fds in the payload. This shouldn't happen
* any more because we're setting m.msg_controllen to
* the exact number we can accept, but be safe and
* truncate. */
fds_to_use = (size_t) *n_fds;
/* 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 = fds_to_use; i < payload_len_fds; i++)
{
close (payload[i]);
}
}
memcpy(fds, CMSG_DATA(cm), *n_fds * sizeof(int));
memcpy (fds, payload, fds_to_use * sizeof (int));
found = TRUE;
/* This cannot overflow because we have chosen fds_to_use
* to be <= *n_fds */
*n_fds = (int) fds_to_use;
/* 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 < fds_to_use; i++)
_dbus_fd_set_close_on_exec(fds[i]);
break;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment