It looks like ports message payloads are not checked in several places:

MessagePipeDispatcher::ReadMessage():

 int rv = node_controller_->node()->GetMessageIf(
      port_,
      [read_any_size, num_bytes, num_handles, &no_space, &may_discard](
          const ports::Message& next_message) {
        const PortsMessage& message =
            static_cast<const PortsMessage&>(next_message);
        DCHECK_GE(message.num_payload_bytes(), sizeof(MessageHeader));

        const MessageHeader* header =
            static_cast<const MessageHeader*>(message.payload_bytes());
        DCHECK_LE(header->header_size, message.num_payload_bytes());


The DCHECKS should not be DCHECKS.

It looks like there are similar issues in:

void DataPipeConsumerDispatcher::UpdateSignalsStateNoLock() {
    ...
    ports::ScopedMessage message;
    do {
      int rv = node_controller_->node()->GetMessageIf(control_port_, nullptr,
                                                      &message);
      if (rv != ports::OK)
        peer_closed_ = true;
      if (message) {
        const DataPipeControlMessage* m =
            static_cast<const DataPipeControlMessage*>(
                message->payload_bytes());

and also in DataPipeProducerDispatcher::UpdateSignalsStateNoLock().

I've attached a patch to set incoming ports message's payload size to 1 in channel_posix, which I used to reproduce buffer overflows:

==29923==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x607000003394 at pc 0x7f48721873f0 bp 
READ of size 4 at 0x607000003394 thread T0 (chrome)
    #0 0x7f48721873ef in operator() out/Default/../../mojo/edk/system/message_pipe_dispatcher.cc:202:21
    #1 0x7f48721873ef in __invoke<(lambda at ../../mojo/edk/system/message_pipe_dispatcher.cc:189:7) &, c
    #2 0x7f48721873ef in __call<(lambda at ../../mojo/edk/system/message_pipe_dispatcher.cc:189:7) &, con
    #3 0x7f48721873ef in std::__1::__function::__func<mojo::edk::MessagePipeDispatcher::ReadMessage(std::
    #4 0x7f48721d19f2 in operator() out/Default/../../buildtools/third_party/libc++/trunk/include/functio
    #5 0x7f48721d19f2 in mojo::edk::ports::MessageQueue::GetNextMessageIf(std::__1::function<bool (mojo::
    #6 0x7f48721d86f1 in mojo::edk::ports::Node::GetMessageIf(mojo::edk::ports::PortRef const&, std::__1:
    #7 0x7f4872184776 in mojo::edk::MessagePipeDispatcher::ReadMessage(std::__1::unique_ptr<mojo::edk::Me
    #8 0x7f487216ccce in mojo::edk::Core::ReadMessageNew(unsigned int, unsigned long*, unsigned int*, uns
    #9 0x7f48721c73bb in MojoReadMessageNew out/Default/../../mojo/edk/embedder/entrypoints.cc:134:10

Also, while running Chrome with this patch, I also found that a use-after-free can be triggered during NodeChannel::DropPeer() (output attached as drop_peer_uaf.txt):

NodeChannel::DropPeer() takes a ports::NodeName as a reference. However, it's possible that this is a reference of NodeChannel's |remote_node_name_|. This means that:

void NodeController::DropPeer(const ports::NodeName& name) {
  DCHECK(io_task_runner_->RunsTasksOnCurrentThread());

  {
    base::AutoLock lock(peers_lock_);
    auto it = peers_.find(name);

    if (it != peers_.end()) {
      ports::NodeName peer = it->first;
      peers_.erase(it);
      DVLOG(1) << "Dropped peer " << peer;
    }

    pending_peer_messages_.erase(name);
    pending_children_.erase(name);

    RecordPeerCount(peers_.size());
    RecordPendingChildCount(pending_children_.size());
  }

  node_->LostConnectionToNode(name);
}

The peers_.erase(it) here can result in the NodeChannel being freed, and thus the |name| reference left pointing to freed memory. (|peers| is a std::unordered_map<ports::NodeName, scoped_refptr<NodeChannel>>), 
 

Deleted: blah.patch 1.7 KB

blah.patch 1.7 KB Download

Deleted: drop_peer_uaf.txt 16.4 KB

drop_peer_uaf.txt 16.4 KB View Download