[mojo-edk] Better validation of untrusted message data

mojo/edk/system/channel.cc

Index: mojo/edk/system/channel.cc
diff --git a/mojo/edk/system/channel.cc b/mojo/edk/system/channel.cc
index c8d693176c917620207a73335d47f7634eb1aab3..2976a21b8a4b2d5b4a8e289fb0748991998bef30 100644
--- a/mojo/edk/system/channel.cc
+++ b/mojo/edk/system/channel.cc
@@ -164,14 +164,20 @@ Channel::MessagePtr Channel::Message::Deserialize(const void* data,
   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;
@@ -530,9 +536,12 @@ bool Channel::OnReadComplete(size_t bytes_read, size_t *next_read_size_hint) {
     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*>(
@@ -543,8 +552,11 @@ bool Channel::OnReadComplete(size_t bytes_read, size_t *next_read_size_hint) {
 
     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;
@@ -553,8 +565,10 @@ bool Channel::OnReadComplete(size_t bytes_read, size_t *next_read_size_hint) {
 
     // 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));
@@ -573,5 +587,12 @@ void Channel::OnError() {
     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