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

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

Index: chrome/browser/extensions/api/cast_channel/cast_socket.cc
diff --git a/chrome/browser/extensions/api/cast_channel/cast_socket.cc b/chrome/browser/extensions/api/cast_channel/cast_socket.cc
index 6130d92e18755dc825880177b785f7536baf5afc..11e40a6109276848c779046ef2bfdb6f7532aaf1 100644
--- a/chrome/browser/extensions/api/cast_channel/cast_socket.cc
+++ b/chrome/browser/extensions/api/cast_channel/cast_socket.cc
@@ -158,7 +158,7 @@ bool CastSocket::ExtractPeerCert(std::string* cert) {
 }
 
 bool CastSocket::VerifyChallengeReply() {
-  return AuthenticateChallengeReply(*challenge_reply_.get(), peer_cert_);
+  return AuthenticateChallengeReply(*challenge_reply_, peer_cert_);
 }
 
 void CastSocket::Connect(const net::CompletionCallback& callback) {
@@ -531,6 +531,8 @@ void CastSocket::DoReadLoop(int result) {
         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:
@@ -539,9 +541,19 @@ void CastSocket::DoReadLoop(int result) {
     }
   } 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() {
@@ -551,14 +563,14 @@ int CastSocket::DoRead() {
   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(
@@ -580,8 +592,8 @@ int CastSocket::DoReadComplete(int 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;
 
@@ -609,40 +621,38 @@ int CastSocket::DoReadComplete(int result) {
 
 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())
@@ -655,8 +665,8 @@ bool CastSocket::ProcessHeader() {
 }
 
 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;
@@ -682,7 +692,7 @@ bool CastSocket::Serialize(const CastMessage& message_proto,
   header.SetMessageSize(message_size);
   header.PrependToString(message_data);
   return true;
-};
+}
 
 void CastSocket::CloseWithError(ChannelError error) {
   DCHECK(CalledOnValidThread());
@@ -705,8 +715,8 @@ bool CastSocket::CalledOnValidThread() const {
 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);
 }