[mojo-edk] Better validation of untrusted message data

mojo/edk/system/channel_win.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 <stdint.h>
 #include <windows.h>
 
 #include <algorithm>
@@ skipping 96 matching lines @@
         reject_writes_ = write_error = true;
     }
     if (write_error) {
       // Do not synchronously invoke OnError(). Write() may have been called by
       // the delegate and we don't want to re-enter it.
       io_task_runner_->PostTask(FROM_HERE,
                                 base::Bind(&ChannelWin::OnError, this));
     }
   }
 
-  ScopedPlatformHandleVectorPtr GetReadPlatformHandles(
+  bool GetReadPlatformHandles(
       size_t num_handles,
       const void* extra_header,
-      size_t extra_header_size) override {
+      size_t extra_header_size,
+      ScopedPlatformHandleVectorPtr* handles) override {
     size_t handles_size = sizeof(PlatformHandle) * num_handles;

[Oliver Chang, 2016/05/16 17:11:34]

|num_handles| here is a size_t, which means that it's possible for this
calculation to overflow (similar in channel_posix.cc).

It seems that mojo::edk::Channel::Message::Header::num_handles is uint16_t, but
could we still get a sanity check for num_handles <=
numeric_limits<uint16_t>::max()?

[Ken Rockot(use gerrit already), 2016/05/16 17:15:32]

Done

     if (handles_size > extra_header_size)
-      return nullptr;
-
-    ScopedPlatformHandleVectorPtr handles(
-        new PlatformHandleVector(num_handles));
-    memcpy(handles->data(), extra_header, handles_size);
-    return handles;
+      return false;
+    DCHECK(extra_header);
+    handles->reset(new PlatformHandleVector(num_handles));
+    memcpy((*handles)->data(), extra_header, handles_size);
+    return true;
   }
 
  private:
   // May run on any thread.
   ~ChannelWin() override {
     // This is intentionally not 0. If another object is constructed on top of
     // this memory, it is likely to initialise values to 0. Using a non-zero
     // value lets us detect the difference between just destroying, and
     // re-allocating the memory.
     sentinel_ = UINTPTR_MAX;
@@ skipping 173 matching lines @@
 // static
 scoped_refptr<Channel> Channel::Create(
     Delegate* delegate,
     ScopedPlatformHandle platform_handle,
     scoped_refptr<base::TaskRunner> io_task_runner) {
   return new ChannelWin(delegate, std::move(platform_handle), io_task_runner);
 }
 
 }  // namespace edk
 }  // namespace mojo