Fix error handling for message parse errors that occur during connection setup.

chrome/browser/extensions/api/cast_channel/cast_socket.cc

 // Copyright 2013 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 "chrome/browser/extensions/api/cast_channel/cast_socket.h"
 
 #include <stdlib.h>
 #include <string.h>
 
 #include "base/bind.h"
@@ skipping 140 matching lines @@
     return false;
   bool result = net::X509Certificate::GetDEREncoded(
      ssl_info.cert->os_cert_handle(), cert);
   if (result)
     VLOG_WITH_CONNECTION(1) << "Successfully extracted peer certificate: "
                             << *cert;
   return result;
 }
 
 bool CastSocket::VerifyChallengeReply() {
-  return AuthenticateChallengeReply(*challenge_reply_.get(), peer_cert_);
+  return AuthenticateChallengeReply(*challenge_reply_, peer_cert_);
 }
 
 void CastSocket::Connect(const net::CompletionCallback& callback) {
   DCHECK(CalledOnValidThread());
   VLOG_WITH_CONNECTION(1) << "Connect readyState = " << ready_state_;
   if (ready_state_ != READY_STATE_NONE) {
     callback.Run(net::ERR_CONNECTION_FAILED);
     return;
   }
   ready_state_ = READY_STATE_CONNECTING;
@@ skipping 352 matching lines @@
       case READ_STATE_READ:
         rv = DoRead();
         break;
       case READ_STATE_READ_COMPLETE:
         rv = DoReadComplete(rv);
         break;
       case READ_STATE_DO_CALLBACK:
         rv = DoReadCallback();
         break;
       case READ_STATE_ERROR:
+        // DoReadError leaves the read_state_ as READ_STATE_NONE,
+        // which is an exit criteria for the finite state machine.

[Wez, 2014/07/22 22:33:10]

This comment is really stipulating an invariant of DoReadError()'s behaviour,
that read_state_ == READ_STATE_NONE after it has executed - so why not
DCHECK_EQ(...) for that instead of the comment, to document the requirement?

[Kevin M, 2014/07/23 18:57:33]

:) Good idea. Done.

         rv = DoReadError(rv);
         break;
       default:
         NOTREACHED() << "BUG in read flow. Unknown state: " << state;
         break;
     }
   } while (rv != net::ERR_IO_PENDING && read_state_ != READ_STATE_NONE);
 
-  // Read loop is done - If the result is ERR_FAILED then close with error.
-  if (rv == net::ERR_FAILED)
-    CloseWithError(error_state_);
+  if (rv == net::ERR_FAILED) {
+    // Handle errors that might have occurred.

[Wez, 2014/07/22 22:33:09]

nit: This comment is superfluous.

[Kevin M, 2014/07/23 18:57:33]

Done.

+    if (ready_state_ == READY_STATE_CONNECTING) {
+      // Connection not yet established.
+      // Clean up without triggering the message event delegate (error
+      // will be sent on the connect cb instead.)

[Wez, 2014/07/22 22:33:09]

nit: Suggest:
"Read errors during the connection handshake should notify the caller via the
connect callback, rather than the message event delegate."
and update the else clause comment similarly.

[Kevin M, 2014/07/23 18:57:33]

Done.

+      PostTaskToStartConnectLoop(net::ERR_FAILED);
+    } else {
+      // Connection is already established.
+      // Close and send error status via the message event delegate.
+      CloseWithError(error_state_);
+    }
+  }
 }
 
 int CastSocket::DoRead() {
   read_state_ = READ_STATE_READ_COMPLETE;
   // Figure out whether to read header or body, and the remaining bytes.
   uint32 num_bytes_to_read = 0;
   if (header_read_buffer_->RemainingCapacity() > 0) {
     current_read_buffer_ = header_read_buffer_;
     num_bytes_to_read = header_read_buffer_->RemainingCapacity();
-    DCHECK_LE(num_bytes_to_read, MessageHeader::header_size());
+    CHECK_LE(num_bytes_to_read, MessageHeader::header_size());
   } else {
     DCHECK_GT(current_message_size_, 0U);
     num_bytes_to_read = current_message_size_ - body_read_buffer_->offset();
     current_read_buffer_ = body_read_buffer_;
-    DCHECK_LE(num_bytes_to_read, MessageHeader::max_message_size());
+    CHECK_LE(num_bytes_to_read, MessageHeader::max_message_size());

[Ryan Sleevi, 2014/07/22 22:52:43]

BUG? It seems to me that the attacker has full control over the message size in
a valid header, in which case, they can construct a message header that
indicates a larger max_message_size

If that is the case, you shouldn't CHECK(). That is, you should never CHECK()
over 'remote' data, as that's a browser DOS.

[Wez, 2014/07/23 05:22:00]

Exactly; see my suggestion re the NOTREACHED() + failure result pattern.

[Kevin M, 2014/07/23 18:57:33]

Done.

[Kevin M, 2014/07/23 18:57:34]

Acknowledged.

   }
-  DCHECK_GT(num_bytes_to_read, 0U);
+  CHECK_GT(num_bytes_to_read, 0U);
 
   // Read up to num_bytes_to_read into |current_read_buffer_|.
   return socket_->Read(
       current_read_buffer_.get(),
       num_bytes_to_read,
       base::Bind(&CastSocket::DoReadLoop, AsWeakPtr()));
 }
 
 int CastSocket::DoReadComplete(int result) {
   VLOG_WITH_CONNECTION(2) << "DoReadComplete result = " << result
                           << " header offset = "
                           << header_read_buffer_->offset()
                           << " body offset = " << body_read_buffer_->offset();
   if (result <= 0) {  // 0 means EOF: the peer closed the socket
     VLOG_WITH_CONNECTION(1) << "Read error, peer closed the socket";
     error_state_ = CHANNEL_ERROR_SOCKET_ERROR;
     read_state_ = READ_STATE_ERROR;
     return result == 0 ? net::ERR_FAILED : result;
   }
 
   // Some data was read.  Move the offset in the current buffer forward.
-  DCHECK_LE(current_read_buffer_->offset() + result,
-            current_read_buffer_->capacity());
+  CHECK_LE(current_read_buffer_->offset() + result,
+           current_read_buffer_->capacity());
   current_read_buffer_->set_offset(current_read_buffer_->offset() + result);
   read_state_ = READ_STATE_READ;
 
   if (current_read_buffer_.get() == header_read_buffer_.get() &&
       current_read_buffer_->RemainingCapacity() == 0) {
     // A full header is read, process the contents.
     if (!ProcessHeader()) {
       error_state_ = cast_channel::CHANNEL_ERROR_INVALID_MESSAGE;
       read_state_ = READ_STATE_ERROR;
     }
   } else if (current_read_buffer_.get() == body_read_buffer_.get() &&
              static_cast<uint32>(current_read_buffer_->offset()) ==
              current_message_size_) {
     // Full body is read, process the contents.
     if (ProcessBody()) {
       read_state_ = READ_STATE_DO_CALLBACK;
     } else {
       error_state_ = cast_channel::CHANNEL_ERROR_INVALID_MESSAGE;
       read_state_ = READ_STATE_ERROR;
     }
   }
 
   return net::OK;
 }
 
 int CastSocket::DoReadCallback() {
   read_state_ = READ_STATE_READ;
-  const CastMessage& message = *(current_message_.get());
-  if (IsAuthMessage(message)) {
-    // An auth message is received, check that connect flow is running.
-    if (ready_state_ == READY_STATE_CONNECTING) {
+  const CastMessage& message = *current_message_;
+  if (ready_state_ == READY_STATE_CONNECTING) {
+    if (IsAuthMessage(message)) {
       challenge_reply_.reset(new CastMessage(message));
       PostTaskToStartConnectLoop(net::OK);
+      return net::OK;
     } else {
+      // Expected an auth message, got something else instead. Handle as error.
       read_state_ = READ_STATE_ERROR;
+      return net::ERR_INVALID_RESPONSE;
     }
-  } else if (delegate_) {
-    MessageInfo message_info;
-    if (CastMessageToMessageInfo(message, &message_info))
-      delegate_->OnMessage(this, message_info);
-    else
-      read_state_ = READ_STATE_ERROR;
   }
+
+  MessageInfo message_info;
+  if (!CastMessageToMessageInfo(message, &message_info)) {
+    read_state_ = READ_STATE_ERROR;
+    return net::ERR_INVALID_RESPONSE;

[Wez, 2014/07/22 22:33:09]

Is it a problem that we're no longer clearing |current_message_| in case of
error?

[Kevin M, 2014/07/23 18:57:33]

Re-added that behavior.

+  }
+  delegate_->OnMessage(this, message_info);

[Ryan Sleevi, 2014/07/22 22:52:43]

Is the delegate_ allowed to delete this?

If so, line 643 is inherently dangerous (really, we shouldn't be calling into
the Delegate here, or in DoReadLoop, but waiting until the pc is in the
'outermost' layer of CastSocket)

[Kevin M, 2014/07/23 18:57:33]

No, the delegate is not allowed to delete this during OnMessage events.

   current_message_->Clear();
   return net::OK;
 }
 
 int CastSocket::DoReadError(int result) {
+  VLOG_WITH_CONNECTION(1) << "DoReadError: " << result;

[Ryan Sleevi, 2014/07/22 22:52:43]

Spammy. VLOG(2) if you have to. But really, you shouldn't.

Long term, wire up the NetLog to Cast to aid in debugging. We discourage console
spam for this.

[Kevin M, 2014/07/23 18:57:33]

Done.

   DCHECK_LE(result, 0);
-  // If inside connection flow, then get back to connect loop.
-  if (ready_state_ == READY_STATE_CONNECTING) {
-    PostTaskToStartConnectLoop(result);
-    // does not try to report error also.
-    return net::OK;
-  }
   return net::ERR_FAILED;
 }
 
 bool CastSocket::ProcessHeader() {
-  DCHECK_EQ(static_cast<uint32>(header_read_buffer_->offset()),
-            MessageHeader::header_size());
+  CHECK_EQ(static_cast<uint32>(header_read_buffer_->offset()),
+           MessageHeader::header_size());
   MessageHeader header;
   MessageHeader::ReadFromIOBuffer(header_read_buffer_.get(), &header);
   if (header.message_size > MessageHeader::max_message_size())
     return false;
 
   VLOG_WITH_CONNECTION(2) << "Parsed header { message_size: "
                           << header.message_size << " }";
   current_message_size_ = header.message_size;
   return true;
 }
 
 bool CastSocket::ProcessBody() {
-  DCHECK_EQ(static_cast<uint32>(body_read_buffer_->offset()),
-            current_message_size_);
+  CHECK_EQ(static_cast<uint32>(body_read_buffer_->offset()),
+           current_message_size_);

[Ryan Sleevi, 2014/07/22 22:52:43]

SECURITY BUG: Crash on hostile input.

[Kevin M, 2014/07/23 18:57:33]

Done.

   if (!current_message_->ParseFromArray(
       body_read_buffer_->StartOfBuffer(), current_message_size_)) {
     return false;
   }
   current_message_size_ = 0;
   header_read_buffer_->set_offset(0);
   body_read_buffer_->set_offset(0);
   current_read_buffer_ = header_read_buffer_;
   return true;
 }
 
 // static
 bool CastSocket::Serialize(const CastMessage& message_proto,
                            std::string* message_data) {
   DCHECK(message_data);
   message_proto.SerializeToString(message_data);
   size_t message_size = message_data->size();
   if (message_size > MessageHeader::max_message_size()) {
     message_data->clear();
     return false;
   }
   CastSocket::MessageHeader header;
   header.SetMessageSize(message_size);
   header.PrependToString(message_data);
   return true;
-};
+}
 
 void CastSocket::CloseWithError(ChannelError error) {
   DCHECK(CalledOnValidThread());
   socket_.reset(NULL);
   ready_state_ = READY_STATE_CLOSED;
   error_state_ = error;
   if (delegate_)
     delegate_->OnError(this, error);
 }
 
 std::string CastSocket::CastUrl() const {
   return ((channel_auth_ == CHANNEL_AUTH_TYPE_SSL_VERIFIED) ?
           "casts://" : "cast://") + ip_endpoint_.ToString();
 }
 
 bool CastSocket::CalledOnValidThread() const {
   return thread_checker_.CalledOnValidThread();
 }
 
 CastSocket::MessageHeader::MessageHeader() : message_size(0) { }
 
 void CastSocket::MessageHeader::SetMessageSize(size_t size) {
-  DCHECK(size < static_cast<size_t>(kuint32max));
-  DCHECK(size > 0);
+  CHECK(size < static_cast<size_t>(kuint32max));
+  CHECK(size > 0);

[Ryan Sleevi, 2014/07/22 22:52:43]

CHECK_LT
CHECK_GT

SECURITY_BUG? Crash on hostile input?

[Kevin M, 2014/07/23 18:57:33]

Now returning a bool, which eventually yields an error if false is returned.

   message_size = static_cast<size_t>(size);
 }
 
 // TODO(mfoltz): Investigate replacing header serialization with base::Pickle,
 // if bit-for-bit compatible.
 void CastSocket::MessageHeader::PrependToString(std::string* str) {
   MessageHeader output = *this;
   output.message_size = base::HostToNet32(message_size);
   size_t header_size = base::checked_cast<size_t,uint32>(
       MessageHeader::header_size());
@@ skipping 31 matching lines @@
   return true;
 }
 
 CastSocket::WriteRequest::~WriteRequest() { }
 
 }  // namespace cast_channel
 }  // namespace api
 }  // namespace extensions
 
 #undef VLOG_WITH_CONNECTION