Only read one message at a time if there are fds pending

systemd-logind's OpenSession() API call returns a fd. If there is a
flood of new sessions, it is possible that by the time we finish reading
message 1, message 2 will already be in our incoming buffer and so on.
This results in systemd-logind consistently having one or more fds enqueued
for an extended period, which we interpret as a denial of service
attack, and handle by kicking it off the bus (at least until we worked
around the resulting logind failure by making uid 0 immune to that
particular anti-DoS mechanism, but that workaround doesn't work for
other uids).

To avoid this without the complexity of tracking multiple countdowns
per connection (one for each message with fds), we can avoid reading
any additional messages while we already have a message with a fd
attached pending processing. To avoid stalling, we have to read the rest
of any partial message we might have, but we stop after that.
Assuming we are able to get rid of the pending fds within a reasonable
time, we'll eventually drain the incoming queue to a level of 0 bytes
and 0 fds, at which point the countdown stops.

To make this actually work, we need fd.o #95619 to be fixed first, so
that when we receive more fds and restart the countdown, it restarts
with its correct time remaining.

Bug: https://bugs.freedesktop.org/show_bug.cgi?id=95263
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Tested-by: Kai-Heng Feng

diff --git a/dbus/dbus-message-internal.h b/dbus/dbus-message-internal.h
index 00259bf63b1418a491425cf5635083c7fa632fef..c912042091a1628b7fc8d49fa16b7b5bcb422a80 100644 (file)
--- a/dbus/dbus-message-internal.h
+++ b/dbus/dbus-message-internal.h
@@ -73,7 +73,9 @@ void               _dbus_message_loader_unref                 (DBusMessageLoader
 
 DBUS_PRIVATE_EXPORT
 void               _dbus_message_loader_get_buffer            (DBusMessageLoader  *loader,
-                                                               DBusString        **buffer);
+                                                               DBusString        **buffer,
+                                                               int                *max_to_read,
+                                                               dbus_bool_t        *may_read_unix_fds);
 DBUS_PRIVATE_EXPORT
 void               _dbus_message_loader_return_buffer         (DBusMessageLoader  *loader,
                                                                DBusString         *buffer);

diff --git a/dbus/dbus-message-util.c b/dbus/dbus-message-util.c
index 60133e11b744ef7b4559fcc5a566c08deda0ab86..740206108a51f659a1f17fd357f71bfd20e7c716 100644 (file)
--- a/dbus/dbus-message-util.c
+++ b/dbus/dbus-message-util.c
@@ -489,7 +489,7 @@ dbus_internal_do_not_use_try_message_data (const DBusString    *data,
     {
       DBusString *buffer;
 
-      _dbus_message_loader_get_buffer (loader, &buffer);
+      _dbus_message_loader_get_buffer (loader, &buffer, NULL, NULL);
       _dbus_string_append_byte (buffer,
                                 _dbus_string_get_byte (data, i));
       _dbus_message_loader_return_buffer (loader, buffer);
@@ -508,7 +508,7 @@ dbus_internal_do_not_use_try_message_data (const DBusString    *data,
   {
     DBusString *buffer;
 
-    _dbus_message_loader_get_buffer (loader, &buffer);
+    _dbus_message_loader_get_buffer (loader, &buffer, NULL, NULL);
     _dbus_string_copy (data, 0, buffer,
                        _dbus_string_get_length (buffer));
     _dbus_message_loader_return_buffer (loader, buffer);
@@ -529,7 +529,7 @@ dbus_internal_do_not_use_try_message_data (const DBusString    *data,
     {
       DBusString *buffer;
 
-      _dbus_message_loader_get_buffer (loader, &buffer);
+      _dbus_message_loader_get_buffer (loader, &buffer, NULL, NULL);
       _dbus_string_append_byte (buffer,
                                 _dbus_string_get_byte (data, i));
       if ((i+1) < len)
@@ -1497,7 +1497,7 @@ _dbus_message_test (const char *test_data_dir)
     {
       DBusString *buffer;
 
-      _dbus_message_loader_get_buffer (loader, &buffer);
+      _dbus_message_loader_get_buffer (loader, &buffer, NULL, NULL);
       _dbus_string_append_byte (buffer, data[i]);
       _dbus_message_loader_return_buffer (loader, buffer);
     }
@@ -1508,7 +1508,7 @@ _dbus_message_test (const char *test_data_dir)
     {
       DBusString *buffer;
 
-      _dbus_message_loader_get_buffer (loader, &buffer);
+      _dbus_message_loader_get_buffer (loader, &buffer, NULL, NULL);
       _dbus_string_append_byte (buffer, data[i]);
       _dbus_message_loader_return_buffer (loader, buffer);
     }

diff --git a/dbus/dbus-message.c b/dbus/dbus-message.c
index da3bb9db0f0bb338124112f5933b48aeae995ccf..0a27f5291cad960d830f44dd534ffde9c1486635 100644 (file)
--- a/dbus/dbus-message.c
+++ b/dbus/dbus-message.c
@@ -4037,13 +4037,99 @@ _dbus_message_loader_unref (DBusMessageLoader *loader)
  */
 void
 _dbus_message_loader_get_buffer (DBusMessageLoader  *loader,
-                                 DBusString        **buffer)
+                                 DBusString        **buffer,
+                                 int                *max_to_read,
+                                 dbus_bool_t        *may_read_fds)
 {
   _dbus_assert (!loader->buffer_outstanding);
 
   *buffer = &loader->data;
 
   loader->buffer_outstanding = TRUE;
+
+  if (max_to_read != NULL)
+    {
+#ifdef HAVE_UNIX_FD_PASSING
+      int offset = 0;
+      int remain;
+      int byte_order;
+      int fields_array_len;
+      int header_len;
+      int body_len;
+#endif
+
+      *max_to_read = DBUS_MAXIMUM_MESSAGE_LENGTH;
+      *may_read_fds = TRUE;
+
+#ifdef HAVE_UNIX_FD_PASSING
+      /* If we aren't holding onto any fds, we can read as much as we want
+       * (fast path). */
+      if (loader->n_unix_fds == 0)
+        return;
+
+      /* Slow path: we have a message with some fds in it. We don't want
+       * to start on the next message until this one is out of the way;
+       * otherwise a legitimate sender can keep us processing messages
+       * containing fds, until we disconnect it for having had fds pending
+       * for too long, a limit that is in place to stop malicious senders
+       * from setting up recursive fd-passing that takes up our quota and
+       * will never go away. */
+
+      remain = _dbus_string_get_length (&loader->data);
+
+      while (remain > 0)
+        {
+          DBusValidity validity = DBUS_VALIDITY_UNKNOWN;
+          int needed;
+
+          /* If 0 < remain < DBUS_MINIMUM_HEADER_SIZE, then we've had at
+           * least the first byte of a message, but we don't know how
+           * much more to read. Only read the rest of the
+           * DBUS_MINIMUM_HEADER_SIZE for now; then we'll know. */
+          if (remain < DBUS_MINIMUM_HEADER_SIZE)
+            {
+              *max_to_read = DBUS_MINIMUM_HEADER_SIZE - remain;
+              *may_read_fds = FALSE;
+              return;
+            }
+
+          if (!_dbus_header_have_message_untrusted (loader->max_message_size,
+                                                    &validity,
+                                                    &byte_order,
+                                                    &fields_array_len,
+                                                    &header_len,
+                                                    &body_len,
+                                                    &loader->data,
+                                                    offset,
+                                                    remain))
+            {
+              /* If a message in the buffer is invalid, we're going to
+               * disconnect the sender anyway, so reading an arbitrary amount
+               * is fine. */
+              if (validity != DBUS_VALID)
+                return;
+
+              /* We have a partial message, with the
+               * DBUS_MINIMUM_HEADER_SIZE-byte fixed part of the header (which
+               * lets us work out how much more we need), but no more. Read
+               * the rest of the message. */
+              needed = header_len + body_len;
+              _dbus_assert (needed > remain);
+              *max_to_read = needed - remain;
+              *may_read_fds = FALSE;
+              return;
+            }
+
+          /* Skip over entire messages until we have less than a message
+           * remaining. */
+          needed = header_len + body_len;
+          _dbus_assert (needed > DBUS_MINIMUM_HEADER_SIZE);
+          _dbus_assert (remain >= needed);
+          remain -= needed;
+          offset += needed;
+        }
+#endif
+    }
 }
 
 /**
@@ -4865,7 +4951,7 @@ dbus_message_demarshal (const char *str,
   if (loader == NULL)
     return NULL;
 
-  _dbus_message_loader_get_buffer (loader, &buffer);
+  _dbus_message_loader_get_buffer (loader, &buffer, NULL, NULL);
 
   if (!_dbus_string_append_len (buffer, str, len))
     goto fail_oom;

diff --git a/dbus/dbus-transport-socket.c b/dbus/dbus-transport-socket.c
index 88fde42bd4b2839d580da80ab5839ebeba4674cd..0b8efe7fa8525a7ecdafd66328aadac7fb6e9276 100644 (file)
--- a/dbus/dbus-transport-socket.c
+++ b/dbus/dbus-transport-socket.c
@@ -796,7 +796,9 @@ do_reading (DBusTransport *transport)
       if (bytes_read > 0)
         {
           _dbus_message_loader_get_buffer (transport->loader,
-                                           &buffer);
+                                           &buffer,
+                                           NULL,
+                                           NULL);
 
           if (!_dbus_auth_decode_data (transport->auth,
                                        &socket_transport->encoded_incoming,
@@ -819,11 +821,19 @@ do_reading (DBusTransport *transport)
     }
   else
     {
+      int max_to_read = DBUS_MAXIMUM_MESSAGE_LENGTH;
+      dbus_bool_t may_read_unix_fds = TRUE;
+
       _dbus_message_loader_get_buffer (transport->loader,
-                                       &buffer);
+                                       &buffer,
+                                       &max_to_read,
+                                       &may_read_unix_fds);
+
+      if (max_to_read > socket_transport->max_bytes_read_per_iteration)
+        max_to_read = socket_transport->max_bytes_read_per_iteration;
 
 #ifdef HAVE_UNIX_FD_PASSING
-      if (DBUS_TRANSPORT_CAN_SEND_UNIX_FD(transport))
+      if (DBUS_TRANSPORT_CAN_SEND_UNIX_FD(transport) && may_read_unix_fds)
         {
           int *fds;
           unsigned int n_fds;
@@ -838,7 +848,7 @@ do_reading (DBusTransport *transport)
 
           bytes_read = _dbus_read_socket_with_unix_fds(socket_transport->fd,
                                                        buffer,
-                                                       socket_transport->max_bytes_read_per_iteration,
+                                                       max_to_read,
                                                        fds, &n_fds);
           saved_errno = _dbus_save_socket_errno ();
 
@@ -851,7 +861,7 @@ do_reading (DBusTransport *transport)
 #endif
         {
           bytes_read = _dbus_read_socket (socket_transport->fd,
-                                          buffer, socket_transport->max_bytes_read_per_iteration);
+                                          buffer, max_to_read);
           saved_errno = _dbus_save_socket_errno ();
         }
 

diff --git a/dbus/dbus-transport.c b/dbus/dbus-transport.c
index 6c6869c20bd4a9086468b1b90fc7cdf158cabf84..9ff9bef6d6296d7a5e3d6a9cac9509027ae0e43f 100644 (file)
--- a/dbus/dbus-transport.c
+++ b/dbus/dbus-transport.c
@@ -1029,7 +1029,9 @@ recover_unused_bytes (DBusTransport *transport)
         }
       
       _dbus_message_loader_get_buffer (transport->loader,
-                                       &buffer);
+                                       &buffer,
+                                       NULL,
+                                       NULL);
       
       orig_len = _dbus_string_get_length (buffer);
       
@@ -1061,7 +1063,9 @@ recover_unused_bytes (DBusTransport *transport)
       dbus_bool_t succeeded;
 
       _dbus_message_loader_get_buffer (transport->loader,
-                                       &buffer);
+                                       &buffer,
+                                       NULL,
+                                       NULL);
 
 #ifdef DBUS_ENABLE_VERBOSE_MODE
       orig_len = _dbus_string_get_length (buffer);