summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorSimon McVittie <simon.mcvittie@collabora.co.uk>2014-09-09 12:44:22 +0100
committerSimon McVittie <simon.mcvittie@collabora.co.uk>2014-09-15 12:31:14 +0100
commitee11ec12566afda5dee8a3a834274421a20661de (patch)
tree1b20a13556d94eb9083cdec1d69d08bb7d1b2b36
parentf70c0e98c5cc6eaae4727d14c389e2504e79e694 (diff)
downloaddbus-ee11ec12566afda5dee8a3a834274421a20661de.tar.gz
_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=83622 Reviewed-by: Alban Crequy <alban.crequy@collabora.co.uk>
-rw-r--r--dbus/dbus-sysdeps-unix.c49
1 files changed, 43 insertions, 6 deletions
diff --git a/dbus/dbus-sysdeps-unix.c b/dbus/dbus-sysdeps-unix.c
index e81e52c3..fe891ab7 100644
--- a/dbus/dbus-sysdeps-unix.c
+++ b/dbus/dbus-sysdeps-unix.c
@@ -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;