In mojo::Channel::Deserialize:

  uint32_t extra_header_size = header->num_header_bytes - sizeof(Header);
#if defined(OS_WIN)
  uint32_t max_handles = extra_header_size / sizeof(HandleEntry);
#elif defined(OS_MACOSX) && !defined(OS_IOS)
  uint32_t max_handles = (extra_header_size - sizeof(MachPortsExtraHeader)) /
      sizeof(MachPortsEntry);
#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));

There is no check that |header->num_header_bytes| is greater than sizeof(Header), which means extra_header_size could underflow. (For OS_MACOSX, there is also an issue as there is no check that |extra_header_size| is greater than sizeof(MachPortsExtraHeader).

This leads to a large |max_handles| value that gets passed to Channel::Message's constructor. For 32-bit builds, where size_t has the same size as uint32_t, this leads to integer overflow when determining the size for allocating the message buffer:

#if defined(OS_WIN)
  // On Windows we serialize HANDLEs into the extra header space.
  extra_header_size = max_handles_ * sizeof(HandleEntry);
#elif defined(OS_MACOSX) && !defined(OS_IOS)
  // On OSX, some of the platform handles may be mach ports, which are
  // serialised into the message buffer. Since there could be a mix of fds and
  // mach ports, we store the mach ports as an <index, port> pair (of uint32_t),
  // so that the original ordering of handles can be re-created.
  if (max_handles) {
    extra_header_size =
        sizeof(MachPortsExtraHeader) + (max_handles * sizeof(MachPortsEntry));
  }
...
size_ = sizeof(Header) + extra_header_size + payload_size;

Leading to buffer overflows afterwards. For the PoC I attached (patch), this gives me an access violation here:

...
  if (max_handles_ > 0) {
#if defined(OS_WIN)
    handles_ = reinterpret_cast<HandleEntry*>(mutable_extra_header());
    // Initialize all handles to invalid values.
    for (size_t i = 0; i < max_handles_; ++i)
      handles_[i].handle = base::win::HandleToUint32(INVALID_HANDLE_VALUE); <--

...

(1b98.1f8): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
eax=07372420 ebx=00000010 ecx=000252f8 edx=3fffffff esi=074b5580 edi=00000000
eip=63143653 esp=062ef7e0 ebp=062ef7e8 iopl=0         nv up ei ng nz ac pe cy
cs=0023  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00010297
chrome_62560000!mojo::edk::Channel::Message::Message+0x93:
63143653 c70488ffffffff  mov     dword ptr [eax+ecx*4],0FFFFFFFFh ds:002b:07407000=????????
0:014> k
 # ChildEBP RetAddr  
00 062ef7e8 63143786 chrome_62560000!mojo::edk::Channel::Message::Message+0x93 [d:\src\chromium\src\mojo\edk\system\channel.cc @ 104]
01 062ef80c 63145043 chrome_62560000!mojo::edk::Channel::Message::Deserialize+0x66 [d:\src\chromium\src\mojo\edk\system\channel.cc @ 160]
02 062ef8e8 63143bab chrome_62560000!mojo::edk::NodeChannel::OnChannelMessage+0x5a3 [d:\src\chromium\src\mojo\edk\system\node_channel.cc @ 648]
03 062ef9d0 63145f60 chrome_62560000!mojo::edk::Channel::OnReadComplete+0x1fb [d:\src\chromium\src\mojo\edk\system\channel.cc @ 587]
04 062ef9e4 63145ed1 chrome_62560000!mojo::edk::`anonymous namespace'::ChannelWin::OnReadDone+0x20 [d:\src\chromium\src\mojo\edk\system\channel_win.cc @ 243]
05 062efaac 6296a189 chrome_62560000!mojo::edk::`anonymous namespace'::ChannelWin::OnIOCompleted+0x71 [d:\src\chromium\src\mojo\edk\system\channel_win.cc @ 232]
06 (Inline) -------- chrome_62560000!base::MessagePumpForIO::WaitForIOCompletion+0x29 [d:\src\chromium\src\base\message_loop\message_pump_win.cc @ 782]
07 062efadc 6296945e chrome_62560000!base::MessagePumpForIO::WaitForWork+0x49 [d:\src\chromium\src\base\message_loop\message_pump_win.cc @ 764]
08 062efbc0 62969f4a chrome_62560000!base::MessagePumpForIO::DoRunLoop+0xae [d:\src\chromium\src\base\message_loop\message_pump_win.cc @ 726]
09 062efbe8 62924981 chrome_62560000!base::MessagePumpWin::Run+0x4a [d:\src\chromium\src\base\message_loop\message_pump_win.cc @ 143]
0a 062efbf4 62951a85 chrome_62560000!base::MessageLoop::RunHandler+0x11 [d:\src\chromium\src\base\message_loop\message_loop.cc @ 457]
0b 062efc18 6292495d chrome_62560000!base::RunLoop::Run+0x65 [d:\src\chromium\src\base\run_loop.cc @ 36]
0c 062efc3c 62939afb chrome_62560000!base::MessageLoop::Run+0x1d [d:\src\chromium\src\base\message_loop\message_loop.cc @ 296]
0d 062efc44 635009ff chrome_62560000!base::Thread::Run+0xb [d:\src\chromium\src\base\threading\thread.cc @ 205]
0e 062efd10 63501166 chrome_62560000!content::BrowserThreadImpl::IOThreadRun+0x1f [d:\src\chromium\src\content\browser\browser_thread_impl.cc @ 224]
0f 062efddc 62939ead chrome_62560000!content::BrowserThreadImpl::Run+0x166 [d:\src\chromium\src\content\browser\browser_thread_impl.cc @ 259]
10 062efe14 6294b4a2 chrome_62560000!base::Thread::ThreadMain+0x11d [d:\src\chromium\src\base\threading\thread.cc @ 258]
11 062efe30 757338f4 chrome_62560000!base::`anonymous namespace'::ThreadFunc+0x82 [d:\src\chromium\src\base\threading\platform_thread_win.cc @ 85]
12 062efe44 76fa5de3 KERNEL32!BaseThreadInitThunk+0x24
13 062efe8c 76fa5dae ntdll!__RtlUserThreadStart+0x2f
14 062efe9c 00000000 ntdll!_RtlUserThreadStart+0x1b

I *think* on 64-bit there is no obvious way that this could lead to memory corruption (because |max_handles| is truncated to 32-bit before being promoted to size_t again for the allocation size calculation), so realistically this only affects 32-bit Windows builds right now.
 

Deleted: blah.patch 4.2 KB

blah.patch 4.2 KB Download