From 098b6a18808e644ede1b56c18732722031ee1251 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Tue, 17 Jan 2017 15:13:36 +0000 Subject: [PATCH 2/2] 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 Tested-by: Kai-Heng Feng --- dbus/dbus-message-internal.h | 4 +- dbus/dbus-message-util.c | 10 ++--- dbus/dbus-message.c | 90 +++++++++++++++++++++++++++++++++++++++++++- dbus/dbus-transport-socket.c | 20 +++++++--- dbus/dbus-transport.c | 8 +++- 5 files changed, 117 insertions(+), 15 deletions(-) diff --git a/dbus/dbus-message-internal.h b/dbus/dbus-message-internal.h index 00259bf6..c9120420 100644 --- 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 60133e11..74020610 100644 --- 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 da3bb9db..0a27f529 100644 --- 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 88fde42b..0b8efe7f 100644 --- 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 6c6869c2..9ff9bef6 100644 --- 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); -- 2.11.0