Update CastSocket connection flow to check for receiver credentials.

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

Index: chrome/browser/extensions/api/cast_channel/cast_socket.cc
===================================================================
--- chrome/browser/extensions/api/cast_channel/cast_socket.cc	(revision 230132)
+++ chrome/browser/extensions/api/cast_channel/cast_socket.cc	(working copy)
@@ -4,6 +4,10 @@
 
 #include "chrome/browser/extensions/api/cast_channel/cast_socket.h"
 
+#include <cert.h>
+#include <cryptohi.h>
+#include <pk11pub.h>
+#include <seccomon.h>
 #include <string.h>
 
 #include "base/bind.h"
@@ -13,6 +17,7 @@
 #include "base/sys_byteorder.h"
 #include "chrome/browser/extensions/api/cast_channel/cast_channel.pb.h"
 #include "chrome/browser/extensions/api/cast_channel/cast_message_util.h"
+#include "crypto/scoped_nss_types.h"

[Ryan Sleevi, 2013/10/22 20:23:22]

DESIGN: Strictly speaking, while I realize extensions are not used in Android,
depending on NSS directly like this is "discouraged" outside of NSS-specific or
ChromeOS specific files.

eg: cast_socket_nss.cc

Or by implementing the auth logic in a separate helper class, and having _nss /
_openssl defines.

[Munjal (Google), 2013/10/22 23:42:43]

Trying to understand this:
- You are saying that I should have another .h file that includes either _nss or
_openssl based on the platform? Can you point me to an example? I was just
following the NetworkingPrivate code you pointed me to and it seems to include
_nss files directly.

[Ryan Sleevi, 2013/10/23 19:14:04]

That's because the NetworkingPrivate code is/was only ChromeOS, IIRC. If not,
then we've got a bug there in terms of style.

You would have cast_auth_util.h, then have cast_auth_util_nss.cc /
cast_auth_util_openssl.cc

You can leave the OpenSSL not-implemented for now.

Both .cc would implement the same method from cast_auth_util.h, and you would
use the GYP file to trigger which dependenc.

[Munjal (Google), 2013/10/23 20:08:56]

Done.

 #include "net/base/address_list.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/net_errors.h"
@@ -44,8 +49,92 @@
 // kTcpKeepAliveDelaySecs.
 const int kTcpKeepAliveDelaySecs = 10;
 
+// Public key of the certificate with which the peer cert should be signed.
+static const unsigned char kCAPublicKeyDER[] = {
+  0x30, 0x82, 0x03, 0x87, 0x30, 0x82, 0x02, 0x6f, 0xa0, 0x03, 0x02, 0x01,
+  0x02, 0x02, 0x01, 0x01, 0x30, 0x0d, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86,
+  0xf7, 0x0d, 0x01, 0x01, 0x05, 0x05, 0x00, 0x30, 0x7c, 0x31, 0x0b, 0x30,
+  0x09, 0x06, 0x03, 0x55, 0x04, 0x06, 0x13, 0x02, 0x55, 0x53, 0x31, 0x13,
+  0x30, 0x11, 0x06, 0x03, 0x55, 0x04, 0x08, 0x0c, 0x0a, 0x43, 0x61, 0x6c,
+  0x69, 0x66, 0x6f, 0x72, 0x6e, 0x69, 0x61, 0x31, 0x16, 0x30, 0x14, 0x06,
+  0x03, 0x55, 0x04, 0x07, 0x0c, 0x0d, 0x4d, 0x6f, 0x75, 0x6e, 0x74, 0x61,
+  0x69, 0x6e, 0x20, 0x56, 0x69, 0x65, 0x77, 0x31, 0x13, 0x30, 0x11, 0x06,
+  0x03, 0x55, 0x04, 0x0a, 0x0c, 0x0a, 0x47, 0x6f, 0x6f, 0x67, 0x6c, 0x65,
+  0x20, 0x49, 0x6e, 0x63, 0x31, 0x12, 0x30, 0x10, 0x06, 0x03, 0x55, 0x04,
+  0x0b, 0x0c, 0x09, 0x47, 0x6f, 0x6f, 0x67, 0x6c, 0x65, 0x20, 0x54, 0x56,
+  0x31, 0x17, 0x30, 0x15, 0x06, 0x03, 0x55, 0x04, 0x03, 0x0c, 0x0e, 0x45,
+  0x75, 0x72, 0x65, 0x6b, 0x61, 0x20, 0x52, 0x6f, 0x6f, 0x74, 0x20, 0x43,
+  0x41, 0x30, 0x1e, 0x17, 0x0d, 0x31, 0x32, 0x31, 0x32, 0x31, 0x39, 0x30,
+  0x30, 0x34, 0x37, 0x31, 0x32, 0x5a, 0x17, 0x0d, 0x33, 0x32, 0x31, 0x32,
+  0x31, 0x34, 0x30, 0x30, 0x34, 0x37, 0x31, 0x32, 0x5a, 0x30, 0x7d, 0x31,
+  0x0b, 0x30, 0x09, 0x06, 0x03, 0x55, 0x04, 0x06, 0x13, 0x02, 0x55, 0x53,
+  0x31, 0x13, 0x30, 0x11, 0x06, 0x03, 0x55, 0x04, 0x08, 0x0c, 0x0a, 0x43,
+  0x61, 0x6c, 0x69, 0x66, 0x6f, 0x72, 0x6e, 0x69, 0x61, 0x31, 0x16, 0x30,
+  0x14, 0x06, 0x03, 0x55, 0x04, 0x07, 0x0c, 0x0d, 0x4d, 0x6f, 0x75, 0x6e,
+  0x74, 0x61, 0x69, 0x6e, 0x20, 0x56, 0x69, 0x65, 0x77, 0x31, 0x13, 0x30,
+  0x11, 0x06, 0x03, 0x55, 0x04, 0x0a, 0x0c, 0x0a, 0x47, 0x6f, 0x6f, 0x67,
+  0x6c, 0x65, 0x20, 0x49, 0x6e, 0x63, 0x31, 0x12, 0x30, 0x10, 0x06, 0x03,
+  0x55, 0x04, 0x0b, 0x0c, 0x09, 0x47, 0x6f, 0x6f, 0x67, 0x6c, 0x65, 0x20,
+  0x54, 0x56, 0x31, 0x18, 0x30, 0x16, 0x06, 0x03, 0x55, 0x04, 0x03, 0x0c,
+  0x0f, 0x45, 0x75, 0x72, 0x65, 0x6b, 0x61, 0x20, 0x47, 0x65, 0x6e, 0x31,
+  0x20, 0x49, 0x43, 0x41, 0x30, 0x82, 0x01, 0x22, 0x30, 0x0d, 0x06, 0x09,
+  0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x01, 0x05, 0x00, 0x03,
+  0x82, 0x01, 0x0f, 0x00, 0x30, 0x82, 0x01, 0x0a, 0x02, 0x82, 0x01, 0x01,
+  0x00, 0xbc, 0x22, 0x80, 0xbd, 0x80, 0xf6, 0x3a, 0x21, 0x00, 0x3b, 0xae,
+  0x76, 0x5e, 0x35, 0x7f, 0x3d, 0xc3, 0x64, 0x5c, 0x55, 0x94, 0x86, 0x34,
+  0x2f, 0x05, 0x87, 0x28, 0xcd, 0xf7, 0x69, 0x8c, 0x17, 0xb3, 0x50, 0xa7,
+  0xb8, 0x82, 0xfa, 0xdf, 0xc7, 0x43, 0x2d, 0xd6, 0x7e, 0xab, 0xa0, 0x6f,
+  0xb7, 0x13, 0x72, 0x80, 0xa4, 0x47, 0x15, 0xc1, 0x20, 0x99, 0x50, 0xcd,
+  0xec, 0x14, 0x62, 0x09, 0x5b, 0xa4, 0x98, 0xcd, 0xd2, 0x41, 0xb6, 0x36,
+  0x4e, 0xff, 0xe8, 0x2e, 0x32, 0x30, 0x4a, 0x81, 0xa8, 0x42, 0xa3, 0x6c,
+  0x9b, 0x33, 0x6e, 0xca, 0xb2, 0xf5, 0x53, 0x66, 0xe0, 0x27, 0x53, 0x86,
+  0x1a, 0x85, 0x1e, 0xa7, 0x39, 0x3f, 0x4a, 0x77, 0x8e, 0xfb, 0x54, 0x66,
+  0x66, 0xfb, 0x58, 0x54, 0xc0, 0x5e, 0x39, 0xc7, 0xf5, 0x50, 0x06, 0x0b,
+  0xe0, 0x8a, 0xd4, 0xce, 0xe1, 0x6a, 0x55, 0x1f, 0x8b, 0x17, 0x00, 0xe6,
+  0x69, 0xa3, 0x27, 0xe6, 0x08, 0x25, 0x69, 0x3c, 0x12, 0x9d, 0x8d, 0x05,
+  0x2c, 0xd6, 0x2e, 0xa2, 0x31, 0xde, 0xb4, 0x52, 0x50, 0xd6, 0x20, 0x49,
+  0xde, 0x71, 0xa0, 0xf9, 0xad, 0x20, 0x40, 0x12, 0xf1, 0xdd, 0x25, 0xeb,
+  0xd5, 0xe6, 0xb8, 0x36, 0xf4, 0xd6, 0x8f, 0x7f, 0xca, 0x43, 0xdc, 0xd7,
+  0x10, 0x5b, 0xe6, 0x3f, 0x51, 0x8a, 0x85, 0xb3, 0xf3, 0xff, 0xf6, 0x03,
+  0x2d, 0xcb, 0x23, 0x4f, 0x9c, 0xad, 0x18, 0xe7, 0x93, 0x05, 0x8c, 0xac,
+  0x52, 0x9a, 0xf7, 0x4c, 0xe9, 0x99, 0x7a, 0xbe, 0x6e, 0x7e, 0x4d, 0x0a,
+  0xe3, 0xc6, 0x1c, 0xa9, 0x93, 0xfa, 0x3a, 0xa5, 0x91, 0x5d, 0x1c, 0xbd,
+  0x66, 0xeb, 0xcc, 0x60, 0xdc, 0x86, 0x74, 0xca, 0xcf, 0xf8, 0x92, 0x1c,
+  0x98, 0x7d, 0x57, 0xfa, 0x61, 0x47, 0x9e, 0xab, 0x80, 0xb7, 0xe4, 0x48,
+  0x80, 0x2a, 0x92, 0xc5, 0x1b, 0x02, 0x03, 0x01, 0x00, 0x01, 0xa3, 0x13,
+  0x30, 0x11, 0x30, 0x0f, 0x06, 0x03, 0x55, 0x1d, 0x13, 0x04, 0x08, 0x30,
+  0x06, 0x01, 0x01, 0xff, 0x02, 0x01, 0x01, 0x30, 0x0d, 0x06, 0x09, 0x2a,
+  0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x05, 0x05, 0x00, 0x03, 0x82,
+  0x01, 0x01, 0x00, 0x8b, 0xd4, 0xa1, 0xb1, 0xcf, 0x5d, 0xcd, 0x7b, 0x6c,
+  0x48, 0x4a, 0x41, 0x1f, 0x53, 0x2f, 0x18, 0x2d, 0x32, 0x45, 0xff, 0x9e,
+  0xab, 0xd3, 0x73, 0x3e, 0x1f, 0x22, 0xd7, 0xea, 0xfa, 0x01, 0xe6, 0x73,
+  0x03, 0x0f, 0x2b, 0xc6, 0x25, 0xbb, 0xa5, 0xee, 0xc5, 0xf5, 0x45, 0xcb,
+  0x24, 0x12, 0x2a, 0xad, 0xc2, 0x5d, 0x05, 0xf4, 0x7a, 0xf5, 0xc2, 0x9b,
+  0x10, 0x16, 0x5a, 0xd1, 0x0a, 0x73, 0xc5, 0x16, 0x39, 0xa0, 0x10, 0xca,
+  0xd1, 0x68, 0x85, 0x9e, 0xfb, 0x9e, 0x26, 0x83, 0x8e, 0x58, 0xf3, 0x77,
+  0xa0, 0x4e, 0xe5, 0xdb, 0x97, 0xbe, 0x2d, 0x00, 0x5f, 0xf5, 0x94, 0xdb,
+  0xb1, 0x9d, 0x65, 0x6b, 0xfd, 0xf0, 0xd1, 0x04, 0x51, 0xdf, 0xcc, 0x92,
+  0xa6, 0x99, 0x2d, 0x71, 0xf5, 0x4d, 0xd5, 0x23, 0xfe, 0x33, 0x1c, 0xa9,
+  0xb4, 0xab, 0xc5, 0xbf, 0x1a, 0xb8, 0xd1, 0x80, 0xef, 0x89, 0xc9, 0xe2,
+  0x1f, 0x9c, 0x4c, 0x48, 0x3b, 0xa2, 0xfa, 0x02, 0x0a, 0xdc, 0x84, 0x01,
+  0x8a, 0x87, 0x02, 0xfb, 0x59, 0xee, 0xa7, 0x4c, 0x04, 0x7d, 0x74, 0x99,
+  0x87, 0x6a, 0x25, 0x44, 0xad, 0x16, 0xaa, 0xec, 0x4e, 0x35, 0x1b, 0x7c,
+  0x7b, 0x84, 0xc9, 0xb1, 0x3f, 0xe1, 0x82, 0x70, 0xe5, 0x0d, 0xe7, 0xd9,
+  0x6d, 0xfa, 0x95, 0xb6, 0xc5, 0xe4, 0x1e, 0xe8, 0x11, 0x9b, 0xd8, 0xb2,
+  0xf3, 0xa4, 0xfd, 0x13, 0xf3, 0x83, 0x4f, 0xf7, 0x07, 0x14, 0x20, 0xbb,
+  0x22, 0xa5, 0xa6, 0x8f, 0xd6, 0xb5, 0xdb, 0xa9, 0x74, 0x78, 0xe2, 0x93,
+  0x0d, 0xe5, 0x23, 0x2f, 0x05, 0x17, 0xe0, 0xb2, 0x97, 0x67, 0x34, 0x4d,
+  0x0f, 0x9c, 0x76, 0x43, 0x7b, 0xa6, 0x21, 0x4a, 0x56, 0x05, 0xf6, 0x2a,
+  0x7c, 0xf2, 0x7f, 0x12, 0x94, 0x82, 0x26, 0x29, 0x07, 0xf0, 0x0b, 0x6c,
+  0x6c, 0x79, 0x14, 0xb0, 0x74, 0xd5, 0x6c };
+
 }  // namespace
 
+typedef scoped_ptr_malloc<
+    CERTCertificate,
+    crypto::NSSDestroyer<CERTCertificate, CERT_DestroyCertificate> >
+ScopedCERTCertificate;
+
 namespace extensions {
 
 static base::LazyInstance<
@@ -65,7 +154,8 @@
 const uint32 kMaxMessageSize = 65536;
 
 CastSocket::CastSocket(const std::string& owner_extension_id,
-                       const GURL& url, CastSocket::Delegate* delegate,
+                       const GURL& url,
+                       CastSocket::Delegate* delegate,
                        net::NetLog* net_log) :
     ApiResource(owner_extension_id),
     channel_id_(0),
@@ -146,12 +236,37 @@
       connection.Pass(), host_and_port, ssl_config, context);
 }
 
+bool CastSocket::ExtractPeerCert(std::string* cert) {
+  CHECK(peer_cert_.empty());
+  net::SSLInfo ssl_info;
+  if (!socket_->GetSSLInfo(&ssl_info) || !ssl_info.cert.get())
+    return false;
+  bool result = net::X509Certificate::GetDEREncoded(
+     ssl_info.cert->os_cert_handle(), cert);
+  if (result)
+    DVLOG(1) << "Successfully extracted peer certificate: " << *cert;
+  return result;
+}
+
+void CastSocket::SendAuthChallenge() {
+  CastMessage challenge_message;
+  CreateAuthChallengeMessage(&challenge_message);
+  SendMessageInternal(challenge_message,
+                      base::Bind(&CastSocket::OnChallengeEvent, AsWeakPtr()));

[Ryan Sleevi, 2013/10/22 20:23:22]

It's unfortunate that you have to use WeakPtr here. Is there no way to avoid
that - eg: by having defined lifetimes?

[Munjal (Google), 2013/10/22 23:42:43]

Right now, all code in this file uses WeakPtr. I will leave it as is and
investigate if there is a way to eliminate it.

+}
+
 void CastSocket::OnConnectComplete(int result) {
   int rv = DoConnectLoop(result);
   if (rv != net::ERR_IO_PENDING)
     DoConnectCallback(rv);
 }
 
+void CastSocket::OnChallengeEvent(int result) {
+  int rv = DoConnectLoop(result);
+  if (rv != net::ERR_IO_PENDING)
+    DoConnectCallback(rv);
+}
+
 void CastSocket::Connect(const net::CompletionCallback& callback) {
   DCHECK(CalledOnValidThread());
   int result = net::ERR_CONNECTION_FAILED;
@@ -205,6 +320,16 @@
       case CONN_STATE_SSL_CONNECT_COMPLETE:
         rv = DoSslConnectComplete(rv);
         break;
+      case CONN_STATE_AUTH_CHALLENGE_SEND:
+        rv = DoAuthChallengeSend();
+        break;
+      case CONN_STATE_AUTH_CHALLENGE_SEND_COMPLETE:
+        rv = DoAuthChallengeSendComplete(rv);
+        break;
+      case CONN_STATE_AUTH_CHALLENGE_REPLY_COMPLETE:
+        rv = DoAuthChallengeReplyComplete(rv);
+        break;
+
       default:
         NOTREACHED() << "BUG in CastSocket state machine code";
         break;
@@ -243,10 +368,132 @@
              peer_cert_.empty() &&
              ExtractPeerCert(&peer_cert_)) {
     next_state_ = CONN_STATE_TCP_CONNECT;
+  } else if (result == net::OK && is_secure_) {
+    next_state_ = CONN_STATE_AUTH_CHALLENGE_SEND;
   }
   return result;
 }
 
+int CastSocket::DoAuthChallengeSend() {
+  next_state_ = CONN_STATE_AUTH_CHALLENGE_SEND_COMPLETE;
+  SendAuthChallenge();
+  return net::ERR_IO_PENDING;
+}
+
+int CastSocket::DoAuthChallengeSendComplete(int result) {
+  next_state_ = CONN_STATE_AUTH_CHALLENGE_REPLY_COMPLETE;

[Ryan Sleevi, 2013/10/22 20:23:22]

Shouldn't you update this only after the error check - eg: line 387?

You're forcing an extra pass through the loop unnecessarily, it seems.

[Munjal (Google), 2013/10/22 23:42:43]

Done.

+  if (result != net::OK)
+    return result;
+
+  ReadData();

[Ryan Sleevi, 2013/10/22 20:23:22]

BUG: This may synchronously complete (line 635), which in turn go through
ProcessHeader(), ProcessBody(), and ParseMessageFromBody(), which may force a
re-entrance into the loop when OnChallengeEvent() is called (from line 713)

[Munjal (Google), 2013/10/22 23:42:43]

Done.

+  return net::ERR_IO_PENDING;
+}
+
+int CastSocket::DoAuthChallengeReplyComplete(int result) {
+  if (result != net::OK)
+    return result;
+  if (VerifyChallengeReply())
+    return net::OK;
+  else
+    return net::ERR_FAILED;
+}
+
+bool CastSocket::VerifyChallengeReply() {
+  CHECK(!peer_cert_.empty());
+  CHECK(challenge_reply_.get());

[Ryan Sleevi, 2013/10/22 20:23:22]

SECURITY BUG: Seems like an attacker could induce a remote crash by sending an
invalid reply that failed to parse.

It doesn't seem like this should be a DCHECK, but something you (gracefully)
handle.

[Munjal (Google), 2013/10/22 23:42:43]

Done.

+
+  DVLOG(1) << "Challenge reply: " << MessageProtoToString(*challenge_reply_);
+
+  if (challenge_reply_->payload_type() != CastMessage_PayloadType_BINARY) {
+    LOG(ERROR) << "Wrong payload type in challenge reply";

[Ryan Sleevi, 2013/10/22 20:23:22]

DVLOG?

There's fairly extensive use of LOG() here, which incurs size in all shipping
binaries. Really think carefully about how often users will need to log this -
and whether it's worth that cost to the hundreds of millions of Chrome users, or
if it's just meant as a debugging aid for Chrome development.

[Munjal (Google), 2013/10/22 23:42:43]

Done.

+    return false;
+  }
+  if (!challenge_reply_->has_payload_binary()) {
+    LOG(ERROR) << "Payload type is binary but payload_binary field not set";
+    return false;
+  }
+  DeviceAuthMessage auth_message;
+  if (!auth_message.ParseFromString(challenge_reply_->payload_binary())) {
+    LOG(ERROR) << "Cannot parse binary payload into DeviceAuthMessage";
+    return false;
+  }
+  DVLOG(1) << "Auth message: " << AuthMessageToString(auth_message);
+  if (auth_message.has_error()) {
+    LOG(ERROR) << "Auth message has error: " <<
+        auth_message.error().error_type();
+    return false;
+  }
+  if (!auth_message.has_response()) {
+    LOG(ERROR) << "Auth message has no response field";
+    return false;
+  }
+
+  const AuthResponse& response = auth_message.response();
+  return VerifyCredentials(response.client_auth_certificate(),
+                           response.signature(),
+                           peer_cert_);
+}
+
+bool CastSocket::VerifyCredentials(const std::string& certificate,
+                                   const std::string& signature,
+                                   const std::string& data) {
+  // Make a copy of the incoming cert string since we are going to
+  // directly use the underlying char array.
+  std::string cert_data(certificate);
+  SECItem der_cert;
+  der_cert.type = siDERCertBuffer;
+  der_cert.data = reinterpret_cast<unsigned char*>(const_cast<char*>(
+      cert_data.c_str()));
+  der_cert.len = cert_data.length();
+
+  // Parse into a certificate structure.
+  ScopedCERTCertificate cert(CERT_NewTempCertificate(
+      CERT_GetDefaultCertDB(), &der_cert, NULL, PR_FALSE, PR_TRUE));

[Ryan Sleevi, 2013/10/22 20:23:22]

You should make sure that crypto::EnsureNSSInit() is called at some point before
using NSS functions.

[Munjal (Google), 2013/10/22 23:42:43]

Done.

+  if (!cert.get()) {
+     LOG(ERROR) << "Failed to parse certificate.";
+     return false;
+  }
+
+  // Check that the certificate is signed by trusted CA.
+  SECItem trusted_ca_key_der_item;
+  trusted_ca_key_der_item.type = siDERCertBuffer;
+  trusted_ca_key_der_item.data = const_cast<unsigned char*>(kCAPublicKeyDER);
+  trusted_ca_key_der_item.len = sizeof(kCAPublicKeyDER);
+  crypto::ScopedSECKEYPublicKey ca_public_key(
+      SECKEY_ImportDERPublicKey(&trusted_ca_key_der_item, CKK_RSA));
+  SECStatus verified = CERT_VerifySignedDataWithPublicKey(
+      &cert->signatureWrap, ca_public_key.get(), NULL);
+  if (verified != SECSuccess) {
+    LOG(ERROR)<< "Cert not signed by trusted CA";
+    return false;
+  }
+
+  // Verify that the |signature| matches |data|.
+  crypto::ScopedSECKEYPublicKey public_key(CERT_ExtractPublicKey(cert.get()));
+  if (!public_key.get()) {
+    LOG(ERROR) << "Unable to extract public key from certificate.";
+    return false;
+  }
+  SECItem signature_item;
+  signature_item.type = siBuffer;
+  signature_item.data = reinterpret_cast<unsigned char*>(
+      const_cast<char*>(signature.c_str()));
+  signature_item.len = static_cast<unsigned int>(signature.size());
+  verified = VFY_VerifyDataDirect(
+      reinterpret_cast<unsigned char*>(const_cast<char*>(data.c_str())),
+      data.size(),
+      public_key.get(),
+      &signature_item,
+      SEC_OID_PKCS1_RSA_ENCRYPTION,
+      SEC_OID_SHA1, NULL, NULL);
+
+  if (verified != SECSuccess) {
+    LOG(ERROR) << "Signed blobs did not match.";
+    return false;
+  }
+  return true;
+}
+
 void CastSocket::DoConnectCallback(int result) {
   ready_state_ = (result == net::OK) ? READY_STATE_OPEN : READY_STATE_CLOSED;
   error_state_ = (result == net::OK) ?
@@ -276,16 +523,24 @@
     callback.Run(result);
     return;
   }
-  WriteRequest write_request(callback);
   CastMessage message_proto;
-  if (!MessageInfoToCastMessage(message, &message_proto) ||
-      !write_request.SetContent(message_proto)) {
+  if (!MessageInfoToCastMessage(message, &message_proto)) {
     CloseWithError(cast_channel::CHANNEL_ERROR_INVALID_MESSAGE);
     // TODO(mfoltz): Do a better job of signaling cast_channel errors to the
     // caller.
     callback.Run(net::OK);
     return;
   }
+  SendMessageInternal(message_proto, callback);
+}
+
+void CastSocket::SendMessageInternal(const CastMessage& message_proto,
+                                     const net::CompletionCallback& callback) {
+  WriteRequest write_request(callback);
+  if (!write_request.SetContent(message_proto)) {
+    CloseWithError(cast_channel::CHANNEL_ERROR_INVALID_MESSAGE);
+    callback.Run(net::OK);

[Ryan Sleevi, 2013/10/22 20:23:22]

BUG: You run a callback but then continue executing the body.

Seems like you should be returning here immediately following the abort.

[Munjal (Google), 2013/10/22 23:42:43]

Done.

+  }
   write_queue_.push(write_request);
   WriteData();
 }
@@ -297,10 +552,6 @@
     return;
 
   WriteRequest& request = write_queue_.front();
-  if (ready_state_ != READY_STATE_OPEN) {
-    request.callback.Run(net::ERR_FAILED);
-    return;
-  }
 
   DVLOG(1) << "WriteData byte_count = " << request.io_buffer->size() <<
     " bytes_written " << request.io_buffer->BytesConsumed();
@@ -311,8 +562,6 @@
       request.io_buffer->BytesRemaining(),
       base::Bind(&CastSocket::OnWriteData, AsWeakPtr()));
 
-  DVLOG(1) << "WriteData result = " << result;
-
   if (result != net::ERR_IO_PENDING)
     OnWriteData(result);
 }
@@ -360,9 +609,8 @@
 
 void CastSocket::ReadData() {
   DCHECK(CalledOnValidThread());
-  if (!socket_.get() || ready_state_ != READY_STATE_OPEN) {
+  if (!socket_.get())
     return;
-  }
   DCHECK(!read_callback_pending_);
   read_callback_pending_ = true;
   // Figure out if we are reading the header or body, and the remaining bytes.
@@ -393,9 +641,9 @@
 
 void CastSocket::OnReadData(int result) {
   DCHECK(CalledOnValidThread());
-  DVLOG(1) << "OnReadData result = " << result <<
-    " header offset = " << header_read_buffer_->offset() <<
-    " body offset = " << body_read_buffer_->offset();
+  DVLOG(1) << "OnReadData result = " << result
+           << " header offset = " << header_read_buffer_->offset()
+           << " body offset = " << body_read_buffer_->offset();
   read_callback_pending_ = false;
   if (result <= 0) {
     CloseWithError(CHANNEL_ERROR_SOCKET_ERROR);
@@ -459,7 +707,11 @@
       current_message_size_))
     return false;
   DVLOG(1) << "Parsed message " << MessageProtoToString(message_proto);
-  if (delegate_) {
+  // If the message is an auth message then we handle it internally.
+  if (IsAuthMessage(message_proto)) {
+    challenge_reply_.reset(new CastMessage(message_proto));
+    OnChallengeEvent(net::OK);
+  } else if (delegate_) {
     MessageInfo message;
     if (!CastMessageToMessageInfo(message_proto, &message))
       return false;