Mojo: (respin) NodeChannel should require a minimum size when using multiple versions

When we added support for versioned message we accidentally introduced a
behavior where messages can be shorter than expected. This is what the
behavior should be to allow older messages to be received. However,
we know the minimum size is the oldest message version. We should
never be allowed to go smaller than that.

This second attempt will explicitly shutdown the channel in the test to
avoid a UAF on Mac.

BUG=chromium:1149805

Change-Id: I0a1daaf69db7ca74963803f8a92c31bfea3aa0df
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2547479
Auto-Submit: Brian Geffon <bgeffon@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Commit-Queue: Brian Geffon <bgeffon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828922}

mojo/core/node_channel.cc

@@ -190,13 +190,16 @@ Channel::MessagePtr CreateMessage(MessageType type,
   return msg_ptr;
 }
 
-template <typename DataType>
-bool GetMessagePayload(const void* bytes,
-                       size_t num_bytes,
-                       DataType* out_data) {
+// This method takes a second template argument which is another datatype which
+// represents the smallest size this payload can be to be considered valid this
+// MUST be used when there is more than one version of a message to specify the
+// oldest version of the message.
+template <typename DataType, typename MinSizedDataType>
+bool GetMessagePayloadMinimumSized(const void* bytes,
+                                   size_t num_bytes,
+                                   DataType* out_data) {
   static_assert(sizeof(DataType) > 0, "DataType must have non-zero size.");
-  // We should have at least 1 byte to contribute towards DataType.
-  if (num_bytes <= sizeof(Header))
+  if (num_bytes < sizeof(Header) + sizeof(MinSizedDataType))
     return false;
 
   // Always make sure that the full object is zeored and default constructed as
@@ -211,6 +214,14 @@ bool GetMessagePayload(const void* bytes,
   return true;
 }
 
+template <typename DataType>
+bool GetMessagePayload(const void* bytes,
+                       size_t num_bytes,
+                       DataType* out_data) {
+  return GetMessagePayloadMinimumSized<DataType, DataType>(bytes, num_bytes,
+                                                           out_data);
+}
+
 }  // namespace
 
 // static

mojo/core/node_channel_unittest.cc

@@ -5,6 +5,7 @@
 #include "mojo/core/node_channel.h"
 
 #include "base/callback_helpers.h"
+#include "base/logging.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/message_loop/message_pump_type.h"
 #include "base/test/task_environment.h"
@@ -21,6 +22,7 @@ namespace {
 
 using NodeChannelTest = testing::Test;
 using ports::NodeName;
+using testing::_;
 
 scoped_refptr<NodeChannel> CreateNodeChannel(NodeChannel::Delegate* delegate,
                                              PlatformChannelEndpoint endpoint) {
@@ -67,6 +69,51 @@ TEST_F(NodeChannelTest, DestructionIsSafe) {
   error_loop.Run();
 }
 
+TEST_F(NodeChannelTest, MessagesCannotBeSmallerThanOldestVersion) {
+  base::test::TaskEnvironment task_environment;
+
+  PlatformChannel channel;
+  MockNodeChannelDelegate local_delegate;
+  auto local_channel =
+      CreateNodeChannel(&local_delegate, channel.TakeLocalEndpoint());
+  local_channel->Start();
+  MockNodeChannelDelegate remote_delegate;
+  auto remote_channel =
+      CreateNodeChannel(&remote_delegate, channel.TakeRemoteEndpoint());
+  remote_channel->Start();
+
+  base::RunLoop loop;
+
+  // It's a bad message and shouldn't be passed to the delegate.
+  EXPECT_CALL(local_delegate, OnRequestPortMerge(_, _, _)).Times(0);
+
+  // This good message should go through after.
+  const NodeName kRemoteNodeName{123, 456};
+  const NodeName kToken{987, 654};
+  EXPECT_CALL(local_delegate,
+              OnAcceptInvitee(ports::kInvalidNodeName, kRemoteNodeName, kToken))
+      .WillRepeatedly([&] {
+    loop.Quit(); });
+
+  // 1 byte is not enough to contain the oldest version of the request port
+  // merge payload, it should be discarded.
+  int payload_size = 1;
+  int capacity = /*sizeof(header)=*/8 + payload_size;
+  auto message =
+      std::make_unique<Channel::Message>(capacity, capacity, /*num_handles=*/0);
+
+  // Set the type of this message as REQUEST_PORT_MERGE (6)
+  *reinterpret_cast<uint32_t*>(message->mutable_payload()) = 6;
+
+  // This short message should be ignored.
+  remote_channel->SendChannelMessage(std::move(message));
+  remote_channel->AcceptInvitee(kRemoteNodeName, kToken);
+  loop.Run();
+
+  remote_channel->ShutDown();
+  local_channel->ShutDown();
+}
+
 }  // namespace
 }  // namespace core
 }  // namespace mojo