[mojo-edk] Better validation of untrusted message data

mojo/edk/system/channel.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "mojo/edk/system/channel.h"
 
 #include <string.h>
 
 #include <algorithm>
 #include <limits>
@@ skipping 132 matching lines @@
   if (header->num_bytes < header->num_header_bytes) {
     DLOG(ERROR) << "Decoding invalid message: " << header->num_bytes << " < "
                 << header->num_header_bytes;
     return nullptr;
   }
 
   uint32_t extra_header_size = header->num_header_bytes - sizeof(Header);
 #if defined(OS_WIN)
   uint32_t max_handles = extra_header_size / sizeof(PlatformHandle);
 #elif defined(OS_MACOSX) && !defined(OS_IOS)
   uint32_t max_handles = extra_header_size / sizeof(MachPortsEntry);

[Anand Mistry (off Chromium), 2016/05/16 04:27:48]

Hm. This reminds me. This check is incorrect. It should be:
max_handles = (extra_header_size - sizeof(MachPortsExtraHeader)) /
sizeof(MachPortsEntry);

[Ken Rockot(use gerrit already), 2016/05/16 04:39:14]

Done

 #endif
   if (header->num_handles > max_handles) {
     DLOG(ERROR) << "Decoding invalid message:" << header->num_handles
                 << " > " << max_handles;
     return nullptr;
   }
 
   MessagePtr message(new Message(data_num_bytes - header->num_header_bytes,
                                  max_handles));
   DCHECK_EQ(message->data_num_bytes(), data_num_bytes);
   DCHECK_EQ(message->extra_header_size(), extra_header_size);
   DCHECK_EQ(message->header_->num_header_bytes, header->num_header_bytes);
 
-  // Copy all payload bytes.
-  memcpy(message->mutable_payload(),
-         static_cast<const char*>(data) + header->num_header_bytes,
-         data_num_bytes - header->num_header_bytes);
-  // Copy extra header bytes.
-  memcpy(message->mutable_extra_header(),
-         static_cast<const char*>(data) + sizeof(Header),
-         message->extra_header_size());
+  if (data_num_bytes > header->num_header_bytes) {

[Anand Mistry (off Chromium), 2016/05/16 04:27:48]

These only protect against 0-byte memcpy's, which I presume memcpy would already
handle cheaply. I personally don't think it's worth it, but at the same time, I
don't care much. Take this as a comment, not a request.

[Ken Rockot(use gerrit already), 2016/05/16 04:39:14]

Yeah - mostly I'm just uncomfortable with ever passing an invalid non-null
pointer to anyone. And when the length is zero, data + num_header_bytes is an
invalid pointer. I realize memcpy probably won't ever do anything with it, but
still... :)

+    // Copy all payload bytes.
+    memcpy(message->mutable_payload(),
+           static_cast<const char*>(data) + header->num_header_bytes,
+           data_num_bytes - header->num_header_bytes);
+  }
+
+  if (message->extra_header_size()) {
+    // Copy extra header bytes.
+    memcpy(message->mutable_extra_header(),
+           static_cast<const char*>(data) + sizeof(Header),
+           message->extra_header_size());
+  }
+
   message->header_->num_handles = header->num_handles;
 
   return message;
 #endif
 }
 
 size_t Channel::Message::payload_size() const {
 #if defined(OS_CHROMEOS) || defined(OS_ANDROID)
   return header_->num_bytes - sizeof(Header);
 #else
@@ skipping 338 matching lines @@
       return true;
     }
 
 #if defined(OS_CHROMEOS) || defined(OS_ANDROID)
     size_t extra_header_size = 0;
     const void* extra_header = nullptr;
     size_t payload_size = header->num_bytes - sizeof(Message::Header);
     void* payload = payload_size ? const_cast<Message::Header*>(&header[1])
                                  : nullptr;
 #else
+    if (header->num_header_bytes < sizeof(Message::Header) ||
+        header->num_header_bytes > header->num_bytes)

[Anand Mistry (off Chromium), 2016/05/16 04:27:48]

Can you add a LOG(ERROR) here, similar to the one for the header->num_bytes
check above. It is, after all, and exceptional condition which should never
happen.

[Ken Rockot(use gerrit already), 2016/05/16 04:39:14]

Done - though this can certainly happen with a malicious message input

[Anand Mistry (off Chromium), 2016/05/16 04:45:50]

Thanks. I'm assuming malicious input is not normal :P

+      return false;
     size_t extra_header_size =
         header->num_header_bytes - sizeof(Message::Header);
-    const void* extra_header = header + 1;
+    const void* extra_header = extra_header_size ? header + 1 : nullptr;
     size_t payload_size = header->num_bytes - header->num_header_bytes;
     void* payload =
         payload_size ? reinterpret_cast<Message::Header*>(
                            const_cast<char*>(read_buffer_->occupied_bytes()) +
                            header->num_header_bytes)
                      : nullptr;
 #endif  // defined(OS_CHROMEOS) || defined(OS_ANDROID)
 
     ScopedPlatformHandleVectorPtr handles;
     if (header->num_handles > 0) {
-      handles = GetReadPlatformHandles(header->num_handles, extra_header,
-                                       extra_header_size);
+      if (!GetReadPlatformHandles(header->num_handles, extra_header,
+                                 extra_header_size, &handles)) {
+        return false;
+      }
+
       if (!handles) {
         // Not enough handles available for this message.
         break;
       }
     }
 
     // We've got a complete message! Dispatch it and try another.
     if (header->message_type != Message::Header::MessageType::NORMAL) {
-      OnControlMessage(header->message_type, payload, payload_size,
-                       std::move(handles));
+      if (!OnControlMessage(header->message_type, payload, payload_size,
+                            std::move(handles))) {
+        return false;
+      }
       did_dispatch_message = true;
     } else if (delegate_) {
       delegate_->OnChannelMessage(payload, payload_size, std::move(handles));
       did_dispatch_message = true;
     }
 
     read_buffer_->Discard(header->num_bytes);
   }
 
   *next_read_size_hint = did_dispatch_message ? 0 : kReadBufferSize;
   return true;
 }
 
 void Channel::OnError() {
   if (delegate_)
     delegate_->OnChannelError();
 }
 
+bool Channel::OnControlMessage(Message::Header::MessageType message_type,
+                               const void* payload,
+                               size_t payload_size,
+                               ScopedPlatformHandleVectorPtr handles) {
+  return false;
+}
+
 }  // namespace edk
 }  // namespace mojo