Attachment 105917 Details for Bug 83622 – _dbus_read_socket_with_unix_fds: do not accept extra fds in cmsg padding

[patch] _dbus_read_socket_with_unix_fds: do not accept extra fds in cmsg padding

0001-_dbus_read_socket_with_unix_fds-do-not-accept-extra-.patch (text/plain), 3.75 KB, created by Simon McVittie on 2014-09-08 19:36:43 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2014-09-08 19:36:43 UTC

Size: 3.75 KB

patch

obsolete

>From 158bf2e77ce26ce3679f55fc7a46aeade3d92413 Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>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
>

Actions: View | Splinter Review

Attachments on bug 83622: 105917 | 105918 | 105975 | 105976 | 105989 | 105990 | 105991 | 105996 | 105997 | 106177 | 106178 | 106187 | 106194

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.