Attachment 105996 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

0002-_dbus_read_socket_with_unix_fds-do-not-accept-extra-.patch (text/plain), 4.24 KB, created by Simon McVittie on 2014-09-09 18:31:18 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2014-09-09 18:31:18 UTC

Size: 4.24 KB

patch

obsolete

>From 41a42fb051b0b4fadaa22a38a48fed5797de2ffb Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>Date: Tue, 9 Sep 2014 12:44:22 +0100
>Subject: [PATCH 2/3] _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.
>
>Bug: https://bugs.freedesktop.org/show_bug.cgi?id=83622
>Reviewed-by: Alban Crequy <alban.crequy@collabora.co.uk>
>---
> dbus/dbus-sysdeps-unix.c | 49 ++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 43 insertions(+), 6 deletions(-)
>
>diff --git a/dbus/dbus-sysdeps-unix.c b/dbus/dbus-sysdeps-unix.c
>index 170d865..b772eaf 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,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;
>-- 
>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.