Cherry-pick 303205@main (bb3b3f72e7e2). https://bugs.webkit.org/show_bug.cgi?id=302649 [GLib] Drop WTF_ALLOW_UNSAFE_BUFFER_USAGE in SocketConnection::readMessage() https://bugs.webkit.org/show_bug.cgi?id=302649 Reviewed by Michael Catanzaro. Use spans over the received message buffer to parse the different elements of the message, advancing over the input idiomatically using consumeAndReinterpretCastTo(), consumeSpan(), and skip(). Note that the message body size is always extended from uint32_t to a size_t, and that the value is never modified and therefore it does not need to be Checked<size_t> because there is no arithmetic performed on it. While at it, check and detect for messages missing the message name delimiter, too short messages, or messages larger than 512 MiB (assuming that no message will ever need to be that big). When an invalid message is detected, use the new didReceiveInvalidMessage() helper to report it, close the connection, and discard the read buffer. * Source/WTF/wtf/glib/SocketConnection.cpp: (WTF::SocketConnection::didReceiveInvalidMessage): Added. (WTF::SocketConnection::readMessage): Modernize to use spans and add additional sanity checks. * Source/WTF/wtf/glib/SocketConnection.h: Added didReceiveInvalidMessage declaration. Canonical link: https://commits.webkit.org/303205@main Canonical link: https://commits.webkit.org/290945.412@webkitglib/2.48
diff --git a/Source/WTF/wtf/glib/SocketConnection.cpp b/Source/WTF/wtf/glib/SocketConnection.cpp index d1185ae..e46ac5b 100644 --- a/Source/WTF/wtf/glib/SocketConnection.cpp +++ b/Source/WTF/wtf/glib/SocketConnection.cpp
@@ -25,6 +25,7 @@ #include <wtf/ByteOrder.h> #include <wtf/CheckedArithmetic.h> #include <wtf/FastMalloc.h> +#include <wtf/Logging.h> #include <wtf/RunLoop.h> namespace WTF { @@ -59,6 +60,14 @@ SocketConnection::~SocketConnection() = default; +bool SocketConnection::didReceiveInvalidMessage(const CString& message) +{ + RELEASE_LOG_FAULT(Process, "Received invalid message (%s), closing SocketConnection", message.data()); + close(); + m_readBuffer.shrink(0); + return false; +} + bool SocketConnection::read() { while (true) { @@ -109,43 +118,54 @@ #endif } +#define MESSAGE_CHECK(assertion, message) do { \ + if (!(assertion)) [[unlikely]] \ + return didReceiveInvalidMessage(message); \ +} while (0) + bool SocketConnection::readMessage() { + // Ensure we have enough data to read the message size. if (m_readBuffer.size() < sizeof(uint32_t)) return false; - WTF_ALLOW_UNSAFE_BUFFER_USAGE_BEGIN // GLib port. - auto* messageData = m_readBuffer.data(); - WTF_ALLOW_UNSAFE_BUFFER_USAGE_END - uint32_t bodySizeHeader; - memcpy(&bodySizeHeader, messageData, sizeof(uint32_t)); - messageData += sizeof(uint32_t); - bodySizeHeader = ntohl(bodySizeHeader); - Checked<size_t> bodySize = bodySizeHeader; - MessageFlags flags; - memcpy(&flags, messageData, sizeof(MessageFlags)); - messageData += sizeof(MessageFlags); - auto messageSize = sizeof(uint32_t) + sizeof(MessageFlags) + bodySize; + auto messageData = m_readBuffer.span(); + const size_t bodySize = ntohl(consumeAndReinterpretCastTo<uint32_t>(messageData)); + + // The smallest possible message has no parameters, one character for the message + // name (an empty name is invalid), and a null terminator at the end of the name. + static auto constexpr MinimumMessageBodySize = 2; + MESSAGE_CHECK(bodySize >= MinimumMessageBodySize, "message body too small"); + + static auto constexpr MaximumMessageBodySize = 512 * MB; + MESSAGE_CHECK(bodySize <= MaximumMessageBodySize, "message body too big"); + + // Ensure the whole message has been read from the socket. + const size_t messageSize = sizeof(uint32_t) + sizeof(MessageFlags) + bodySize; if (m_readBuffer.size() < messageSize) { m_readBuffer.reserveCapacity(messageSize); return false; } - Checked<size_t> messageNameLength = strlen(messageData); - messageNameLength++; - if (m_readBuffer.size() < messageNameLength) { - ASSERT_NOT_REACHED(); - return false; - } + const auto flags = consumeAndReinterpretCastTo<MessageFlags>(messageData); - const auto it = m_messageHandlers.find(messageData); + // Ensure that the span covers only the first message in the read buffer, and + // that parsing the message does not step onto the next one in the buffer. + messageData = messageData.first(bodySize); + + const auto nullIndex = find(messageData, '\0'); + MESSAGE_CHECK(nullIndex != notFound, "message name delimiter missing"); + + const CString messageName(consumeSpan(messageData, nullIndex)); + ASSERT(messageData.front() == '\0'); + skip(messageData, 1); + + const auto it = m_messageHandlers.find(messageName); if (it != m_messageHandlers.end()) { - messageData += messageNameLength.value(); GRefPtr<GVariant> parameters; if (!it->value.first.isNull()) { GUniquePtr<GVariantType> variantType(g_variant_type_new(it->value.first.data())); - size_t parametersSize = bodySize.value() - messageNameLength.value(); - parameters = g_variant_new_from_data(variantType.get(), messageData, parametersSize, FALSE, nullptr, nullptr); + parameters = g_variant_new_from_data(variantType.get(), messageData.data(), messageData.size(), FALSE, nullptr, nullptr); if (messageIsByteSwapped(flags)) parameters = adoptGRef(g_variant_byteswap(parameters.get())); } @@ -155,10 +175,8 @@ } if (m_readBuffer.size() > messageSize) { - WTF_ALLOW_UNSAFE_BUFFER_USAGE_BEGIN // GLib port. - std::memmove(m_readBuffer.data(), m_readBuffer.data() + messageSize.value(), m_readBuffer.size() - messageSize.value()); - WTF_ALLOW_UNSAFE_BUFFER_USAGE_END - m_readBuffer.shrink(m_readBuffer.size() - messageSize.value()); + memmoveSpan(m_readBuffer.mutableSpan(), m_readBuffer.subspan(messageSize)); + m_readBuffer.shrink(m_readBuffer.size() - messageSize); } else m_readBuffer.shrink(0); @@ -168,6 +186,8 @@ return true; } +#undef MESSAGE_CHECK + void SocketConnection::sendMessage(const char* messageName, GVariant* parameters) { GRefPtr<GVariant> adoptedParameters = parameters;
diff --git a/Source/WTF/wtf/glib/SocketConnection.h b/Source/WTF/wtf/glib/SocketConnection.h index 94ddde7..fde497b 100644 --- a/Source/WTF/wtf/glib/SocketConnection.h +++ b/Source/WTF/wtf/glib/SocketConnection.h
@@ -50,6 +50,7 @@ private: WTF_EXPORT_PRIVATE SocketConnection(GRefPtr<GSocketConnection>&&, const MessageHandlers&, gpointer); + bool didReceiveInvalidMessage(const CString& message); bool read(); bool readMessage(); void write();