Changeset 285865 in webkit

Timestamp:

Nov 16, 2021, 8:51:30 AM (5 years ago)

Author:

Chris Dumez

Message:

Do some hardening in IPC::createMessageDecoder()
​https://bugs.webkit.org/show_bug.cgi?id=233148
<rdar://75139294>

Reviewed by Darin Adler.

Do more bound validation insde createMessageDecoder() to make sure we stay within
the bounds of our ReceiveBuffer.

Also, when the body is out of line, set out_of_line.deallocate to false since
we are taking ownership of the memory and will vm_deallocate() it ourselves.
Normally the sender (Connection::sendOutgoingMessage) sets that flag to false but
it is better not to rely on the sender setting a particular flag.

• Platform/IPC/cocoa/ConnectionCocoa.mm:

(IPC::createMessageDecoder):
(IPC::Connection::receiveSourceEventHandler):

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• Platform/IPC/cocoa/ConnectionCocoa.mm (modified) (6 diffs)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2021-11-16  Chris Dumez  <cdumez@apple.com>
+
+        Do some hardening in IPC::createMessageDecoder()
+        https://bugs.webkit.org/show_bug.cgi?id=233148
+        <rdar://75139294>
+
+        Reviewed by Darin Adler.
+
+        Do more bound validation insde createMessageDecoder() to make sure we stay within
+        the bounds of our ReceiveBuffer.
+
+        Also, when the body is out of line, set `out_of_line.deallocate` to false since
+        we are taking ownership of the memory and will vm_deallocate() it ourselves.
+        Normally the sender (Connection::sendOutgoingMessage) sets that flag to false but
+        it is better not to rely on the sender setting a particular flag.
+
+        * Platform/IPC/cocoa/ConnectionCocoa.mm:
+        (IPC::createMessageDecoder):
+        (IPC::Connection::receiveSourceEventHandler):
+
 2021-11-16  J Pascoe  <j_pascoe@apple.com>
 

trunk/Source/WebKit/Platform/IPC/cocoa/ConnectionCocoa.mm

@@ -409 +409 @@
 }
 
-static std::unique_ptr<Decoder> createMessageDecoder(mach_msg_header_t* header)
-{
+static std::unique_ptr<Decoder> createMessageDecoder(mach_msg_header_t* header, size_t bufferSize)
+{
+    if (UNLIKELY(header->msgh_size > bufferSize)) {
+        RELEASE_LOG_FAULT(IPC, "createMessageDecoder: msgh_size is greater than bufferSize (header->msgh_size: %lu, bufferSize: %lu)", static_cast<unsigned long>(header->msgh_size), bufferSize);
+        ASSERT_NOT_REACHED();
+        return nullptr;
+    }
+
     if (!(header->msgh_bits & MACH_MSGH_BITS_COMPLEX)) {
         // We have a simple message.
@@ -427 +433 @@
     mach_msg_size_t numberOfPortDescriptors = body->msgh_descriptor_count;
     ASSERT(numberOfPortDescriptors);
-    if (!numberOfPortDescriptors)
+    if (UNLIKELY(!numberOfPortDescriptors))
         return nullptr;
+
+    auto sizeWithPortDescriptors = CheckedSize { sizeof(mach_msg_header_t) + sizeof(mach_msg_body_t) } + CheckedSize { numberOfPortDescriptors } * sizeof(mach_msg_port_descriptor_t);
+    if (UNLIKELY(sizeWithPortDescriptors.hasOverflowed() || sizeWithPortDescriptors.value() > bufferSize)) {
+        RELEASE_LOG_FAULT(IPC, "createMessageDecoder: Overflow when computing sizeWithPortDescriptors (numberOfPortDescriptors: %lu)", static_cast<unsigned long>(numberOfPortDescriptors));
+        ASSERT_NOT_REACHED();
+        return nullptr;
+    }
 
     uint8_t* descriptorData = reinterpret_cast<uint8_t*>(body + 1);
@@ -446 +459 @@
             return nullptr;
 
-        attachments[numberOfAttachments - i - 1] = Attachment(descriptor->port.name, descriptor->port.disposition);
+        attachments[numberOfAttachments - i - 1] = Attachment { descriptor->port.name, descriptor->port.disposition };
         descriptorData += sizeof(mach_msg_port_descriptor_t);
     }
@@ -458 +471 @@
         uint8_t* messageBody = static_cast<uint8_t*>(descriptor->out_of_line.address);
         size_t messageBodySize = descriptor->out_of_line.size;
+        descriptor->out_of_line.deallocate = false; // We are taking ownership of the memory.
 
         return Decoder::create(messageBody, messageBodySize, [](const uint8_t* buffer, size_t length) {
@@ -466 +480 @@
 
     uint8_t* messageBody = descriptorData;
-    ASSERT(descriptorData >= reinterpret_cast<uint8_t*>(header));
-    auto messageBodySize = CheckedSize { header->msgh_size } - static_cast<size_t>(descriptorData - reinterpret_cast<uint8_t*>(header));
+    ASSERT((reinterpret_cast<uint8_t*>(header) + sizeWithPortDescriptors.value()) == messageBody);
+    auto messageBodySize = header->msgh_size - sizeWithPortDescriptors;
     if (UNLIKELY(messageBodySize.hasOverflowed())) {
-        RELEASE_LOG_FAULT(IPC, "createMessageDecoder: Overflow when computing bodySize (header->msgh_size: %lu, (descriptorData - reinterpret_cast<uint8_t*>(header)): %lu)", static_cast<unsigned long>(header->msgh_size), static_cast<unsigned long>(descriptorData - reinterpret_cast<uint8_t*>(header)));
+        RELEASE_LOG_FAULT(IPC, "createMessageDecoder: Overflow when computing bodySize (header->msgh_size: %lu, sizeWithPortDescriptors: %lu)", static_cast<unsigned long>(header->msgh_size), static_cast<unsigned long>(sizeWithPortDescriptors.value()));
         ASSERT_NOT_REACHED();
         return nullptr;
@@ -538 +552 @@
     }
 
-    std::unique_ptr<Decoder> decoder = createMessageDecoder(header);
+    std::unique_ptr<Decoder> decoder = createMessageDecoder(header, buffer.size());
     if (!decoder)
         return;