Mojo: Properly check various sizes of incoming messages (on a MessagePipe).

R=tsepez@chromium.org

Review URL: https://codereview.chromium.org/209433009

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@259130 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 136 insertions, 10 deletions

diff --git a/mojo/system/channel.cc b/mojo/system/channel.cc
index e668c87..d26d46d 100644
--- a/mojo/system/channel.cc
+++ b/mojo/system/channel.cc
@@ -166,6 +166,10 @@ Channel::~Channel() {
 }
 
 void Channel::OnReadMessage(const MessageInTransit::View& message_view) {
+  // Note: |ValidateReadMessage()| will call |HandleRemoteError()| if necessary.
+  if (!ValidateReadMessage(message_view))
+    return;
+
   switch (message_view.type()) {
     case MessageInTransit::kTypeMessagePipeEndpoint:
     case MessageInTransit::kTypeMessagePipe:
@@ -187,6 +191,17 @@ void Channel::OnFatalError(FatalError fatal_error) {
   NOTIMPLEMENTED();
 }
 
+bool Channel::ValidateReadMessage(const MessageInTransit::View& message_view) {
+  const char* error_message = NULL;
+  if (!message_view.IsValid(&error_message)) {
+    DCHECK(error_message);
+    HandleRemoteError(error_message);
+    return false;
+  }
+
+  return true;
+}
+
 void Channel::OnReadMessageForDownstream(
     const MessageInTransit::View& message_view) {
   DCHECK(message_view.type() == MessageInTransit::kTypeMessagePipeEndpoint ||
@@ -260,7 +275,8 @@ void Channel::OnReadMessageForChannel(
 }
 
 void Channel::HandleRemoteError(const base::StringPiece& error_message) {
-  // TODO(vtl): Is this how we really want to handle this?
+  // TODO(vtl): Is this how we really want to handle this? Probably we want to
+  // terminate the connection, since it's spewing invalid stuff.
   LOG(WARNING) << error_message;
 }
 
diff --git a/mojo/system/channel.h b/mojo/system/channel.h
index 96c1f8c..8268a94 100644
--- a/mojo/system/channel.h
+++ b/mojo/system/channel.h
@@ -107,6 +107,7 @@ class MOJO_SYSTEM_IMPL_EXPORT Channel
   virtual void OnFatalError(FatalError fatal_error) OVERRIDE;
 
   // Helpers for |OnReadMessage|:
+  bool ValidateReadMessage(const MessageInTransit::View& message_view);
   void OnReadMessageForDownstream(const MessageInTransit::View& message_view);
   void OnReadMessageForChannel(const MessageInTransit::View& message_view);
 
diff --git a/mojo/system/dispatcher.h b/mojo/system/dispatcher.h
index 03865a3..966b6da 100644
--- a/mojo/system/dispatcher.h
+++ b/mojo/system/dispatcher.h
@@ -275,8 +275,10 @@ class MOJO_SYSTEM_IMPL_EXPORT Dispatcher :
   // on a dispatcher attached to a |MessageInTransit| (and in particular not in
   // |CoreImpl|'s handle table).
   // Gets the maximum amount of space that'll be needed to serialize this
-  // dispatcher to the given |Channel|. Returns zero to indicate that this
-  // dispatcher cannot be serialized (to the given |Channel|).
+  // dispatcher to the given |Channel|. This amount must be no greater than
+  // |MessageInTransit::kMaxSerializedDispatcherSize| (message_in_transit.h).
+  // Returns zero to indicate that this dispatcher cannot be serialized (to the
+  // given |Channel|).
   size_t GetMaximumSerializedSize(const Channel* channel) const;
   // Serializes this dispatcher to the given |Channel| by writing to
   // |destination| and then closes this dispatcher. It may write no more than
diff --git a/mojo/system/message_in_transit.cc b/mojo/system/message_in_transit.cc
index 6821b48..c56f4f3 100644
--- a/mojo/system/message_in_transit.cc
+++ b/mojo/system/message_in_transit.cc
@@ -16,6 +16,11 @@
 namespace mojo {
 namespace system {
 
+namespace {
+
+
+}  // namespace
+
 struct MessageInTransit::PrivateStructForCompileAsserts {
   // The size of |Header| must be appropriate to maintain alignment of the
   // following data.
@@ -27,6 +32,14 @@ struct MessageInTransit::PrivateStructForCompileAsserts {
                      0x7fffffffULL,
                  kMaxMessageNumBytes_too_big);
 
+  // We assume (to avoid extra rounding code) that the maximum message (data)
+  // size is a multiple of the alignment.
+  COMPILE_ASSERT(kMaxMessageNumBytes % kMessageAlignment == 0,
+                 kMessageAlignment_not_a_multiple_of_alignment);
+
+  COMPILE_ASSERT(kMaxSerializedDispatcherSize % kMessageAlignment == 0,
+                 kMaxSerializedDispatcherSize_not_a_multiple_of_alignment);
+
   // The size of |HandleTableEntry| must be appropriate to maintain alignment.
   COMPILE_ASSERT(sizeof(HandleTableEntry) % kMessageAlignment == 0,
                  sizeof_MessageInTransit_HandleTableEntry_invalid);
@@ -45,7 +58,12 @@ STATIC_CONST_MEMBER_DEFINITION const MessageInTransit::Subtype
 STATIC_CONST_MEMBER_DEFINITION const MessageInTransit::EndpointId
     MessageInTransit::kInvalidEndpointId;
 STATIC_CONST_MEMBER_DEFINITION const size_t MessageInTransit::kMessageAlignment;
+STATIC_CONST_MEMBER_DEFINITION const size_t
+    MessageInTransit::kMaxSerializedDispatcherSize;
 
+// static
+const size_t MessageInTransit::kMaxSecondaryBufferSize = kMaxMessageNumHandles *
+    (sizeof(HandleTableEntry) + kMaxSerializedDispatcherSize);
 
 MessageInTransit::View::View(size_t message_size, const void* buffer)
     : buffer_(buffer) {
@@ -57,6 +75,24 @@ MessageInTransit::View::View(size_t message_size, const void* buffer)
   DCHECK_EQ(message_size, total_size());
 }
 
+bool MessageInTransit::View::IsValid(const char** error_message) const {
+  // Note: This also implies a check on the |main_buffer_size()|, which is just
+  // |RoundUpMessageAlignment(sizeof(Header) + num_bytes())|.
+  if (num_bytes() > kMaxMessageNumBytes) {
+    *error_message = "Message data payload too large";
+    return false;
+  }
+
+  if (const char* secondary_buffer_error_message =
+          ValidateSecondaryBuffer(num_handles(), secondary_buffer(),
+                                  secondary_buffer_size())) {
+    *error_message = secondary_buffer_error_message;
+    return false;
+  }
+
+  return true;
+}
+
 MessageInTransit::MessageInTransit(Type type,
                                    Subtype subtype,
                                    uint32_t num_bytes,
@@ -178,15 +214,16 @@ void MessageInTransit::SerializeAndCloseDispatchers(Channel* channel) {
   size_t size = handle_table_size;
   for (size_t i = 0; i < dispatchers_->size(); i++) {
     if (Dispatcher* dispatcher = (*dispatchers_)[i].get()) {
-      size += RoundUpMessageAlignment(
+      size_t max_serialized_size =
           Dispatcher::MessageInTransitAccess::GetMaximumSerializedSize(
-              dispatcher, channel));
-      // TODO(vtl): Check for overflow?
+              dispatcher, channel);
+      DCHECK_LE(max_serialized_size, kMaxSerializedDispatcherSize);
+      size += RoundUpMessageAlignment(max_serialized_size);
+      DCHECK_LE(size, kMaxSecondaryBufferSize);
     }
   }
 
   secondary_buffer_ = base::AlignedAlloc(size, kMessageAlignment);
-  // TODO(vtl): Check for overflow?
   secondary_buffer_size_ = static_cast<uint32_t>(size);
   // Entirely clear out the secondary buffer, since then we won't have to worry
   // about clearing padding or unused space (e.g., if a dispatcher fails to
@@ -228,12 +265,12 @@ void MessageInTransit::SerializeAndCloseDispatchers(Channel* channel) {
 void MessageInTransit::DeserializeDispatchers(Channel* channel) {
   DCHECK(!dispatchers_.get());
 
+  // This should have been checked by calling |IsValid()| on the |View| first.
+  DCHECK_LE(num_handles(), kMaxMessageNumHandles);
+
   if (!num_handles())
     return;
 
-  // TODO(vtl): Restrict |num_handles()| to a sane range. (Maybe this should be
-  // done earlier?)
-
   dispatchers_.reset(
       new std::vector<scoped_refptr<Dispatcher> >(num_handles()));
 
@@ -263,6 +300,50 @@ void MessageInTransit::DeserializeDispatchers(Channel* channel) {
   }
 }
 
+// Validates the secondary buffer. Returns null on success, or a human-readable
+// error message on error.
+// static
+const char* MessageInTransit::ValidateSecondaryBuffer(
+    size_t num_handles,
+    const void* secondary_buffer,
+    size_t secondary_buffer_size) {
+  if (!num_handles)
+    return NULL;
+
+  if (num_handles > kMaxMessageNumHandles)
+    return "Message handle payload too large";
+
+  if (secondary_buffer_size > kMaxSecondaryBufferSize)
+    return "Message secondary buffer too large";
+
+  if (secondary_buffer_size < num_handles * sizeof(HandleTableEntry))
+    return "Message secondary buffer too small";
+
+  DCHECK(secondary_buffer);
+  const HandleTableEntry* handle_table =
+      static_cast<const HandleTableEntry*>(secondary_buffer);
+
+  static const char kInvalidSerializedDispatcher[] =
+      "Message contains invalid serialized dispatcher";
+  for (size_t i = 0; i < num_handles; i++) {
+    size_t offset = handle_table[i].offset;
+    if (offset % kMessageAlignment != 0)
+      return kInvalidSerializedDispatcher;
+
+    size_t size = handle_table[i].size;
+    if (size > kMaxSerializedDispatcherSize || size > secondary_buffer_size)
+      return kInvalidSerializedDispatcher;
+
+    // Note: This is an overflow-safe check for |offset + size >
+    // secondary_buffer_size()| (we know that |size <= secondary_buffer_size()|
+    // from the previous check).
+    if (offset > secondary_buffer_size - size)
+      return kInvalidSerializedDispatcher;
+  }
+
+  return NULL;
+}
+
 void MessageInTransit::UpdateTotalSize() {
   DCHECK_EQ(main_buffer_size_ % kMessageAlignment, 0u);
   DCHECK_EQ(secondary_buffer_size_ % kMessageAlignment, 0u);
diff --git a/mojo/system/message_in_transit.h b/mojo/system/message_in_transit.h
index 7a8475d..966f84b 100644
--- a/mojo/system/message_in_transit.h
+++ b/mojo/system/message_in_transit.h
@@ -65,6 +65,10 @@ class MOJO_SYSTEM_IMPL_EXPORT MessageInTransit {
   // quantity (which must be a power of 2).
   static const size_t kMessageAlignment = 8;
 
+  // The maximum size of a single serialized dispatcher. This must be a multiple
+  // of |kMessageAlignment|.
+  static const size_t kMaxSerializedDispatcherSize = 10000;
+
   // Forward-declare |Header| so that |View| can use it:
  private:
   struct Header;
@@ -78,6 +82,18 @@ class MOJO_SYSTEM_IMPL_EXPORT MessageInTransit {
     // |buffer| should be |kMessageAlignment|-byte aligned.
     View(size_t message_size, const void* buffer);
 
+    // Checks the following things versus pre-determined limits:
+    //   - |num_bytes()| and |main_buffer_size()|,
+    //   - |num_handles()| and |secondary_buffer_size()|,
+    //   - the entries in the handle entry table (that the sizes and offsets are
+    //     valid).
+    // Note: It does not check the serialized dispatcher data itself.
+    //
+    // It returns true (and leaves |error_message| alone) if this object appears
+    // to be a valid message (according to the above) and false, pointing
+    // |*error_message| to a suitable error message, if not.
+    bool IsValid(const char** error_message) const;
+
     // API parallel to that for |MessageInTransit| itself (mostly getters for
     // header data).
     const void* main_buffer() const { return buffer_; }
@@ -233,6 +249,16 @@ class MOJO_SYSTEM_IMPL_EXPORT MessageInTransit {
     uint32_t unused;
   };
 
+  // The maximum possible size of a valid secondary buffer: for each handle,
+  // there'll be a handle table entry and its serialized data.
+  static const size_t kMaxSecondaryBufferSize;
+
+  // Validates the secondary buffer. Returns null on success, or a
+  // human-readable error message on error.
+  static const char* ValidateSecondaryBuffer(size_t num_handles,
+                                             const void* secondary_buffer,
+                                             size_t secondary_buffer_size);
+
   const Header* header() const {
     return static_cast<const Header*>(main_buffer_);
   }