Implementation of SecureP2PSocket

remoting/protocol/secure_p2p_socket.cc

Index: remoting/protocol/secure_p2p_socket.cc
diff --git a/remoting/protocol/secure_p2p_socket.cc b/remoting/protocol/secure_p2p_socket.cc
new file mode 100644
index 0000000000000000000000000000000000000000..870bcd56fa42300b09901cc44b3ab430a13ae189
--- /dev/null
+++ b/remoting/protocol/secure_p2p_socket.cc
@@ -0,0 +1,310 @@
+// Copyright (c) 2011 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 "remoting/protocol/secure_p2p_socket.h"
+
+#include "base/logging.h"
+#include "base/rand_util.h"
+#include "crypto/symmetric_key.h"
+#include "net/base/io_buffer.h"
+#include "net/base/net_errors.h"
+
+using net::CompletionCallback;
+using net::IOBuffer;

[Sergey Ulanov, 2011/06/22 23:51:35]

not needed because you include io_buffer.h.

[Alpha Left Google, 2011/06/23 21:53:01]

This using is just so that I don't need net:: when I use IOBuffer.

[Sergey Ulanov, 2011/06/23 23:12:45]

Oh, yes, I misread it, sorry.

+
+namespace remoting {
+namespace protocol {
+
+namespace {
+const char kMaskSalt[16] = {0xDB, 0x68, 0xB5, 0xFD, 0x17, 0x0E, 0x15, 0x77,
+                            0x56, 0xAF, 0x7A, 0x3A, 0x1A, 0x57, 0x75, 0x02};
+const char kHashSalt[16] = {0x4E, 0x2F, 0x96, 0xAB, 0x0A, 0x39, 0x92, 0xA2,
+                            0x56, 0x94, 0x91, 0xF5, 0x7E, 0x58, 0x2E, 0xFA};
+const char kFrameType[4] = {0x0, 0x0, 0x0, 0x1};
+const size_t kKeySize = 16;
+const int kHeaderSize = 44;
+const int kSeqNumberSize = 8;
+const int kFrameTypeSize = sizeof(kFrameType);

[Sergey Ulanov, 2011/06/22 23:51:35]

nit: put this next to kFrameType.

[Alpha Left Google, 2011/06/23 21:53:01]

Done.

+const int kHashPosition = 0;
+const int kNoncePosition = kKeySize;
+const int kRawMessagePosition = kNoncePosition + kKeySize;
+const int kSeqNumberPosition = kRawMessagePosition;
+const int kFrameTypePosition = kSeqNumberPosition + kSeqNumberSize;
+const int kMessagePosition = kFrameTypePosition + kFrameTypeSize;
+const int kReadBufferSize = 65536;
+const std::string kMaskSaltStr(kMaskSalt, kKeySize);
+const std::string kHashSaltStr(kHashSalt, kKeySize);
+
+inline void Set8(void* memory, size_t offset, uint8 v) {

[Sergey Ulanov, 2011/06/22 23:51:35]

Do you really need Set8 and Get8? You just case |memory| to to uint8* in SetBE64
and GetBE64 and replace Set8 and Get8 with direct array access. It also might be
easier to optimize for compiler this way.

[Alpha Left Google, 2011/06/23 21:53:01]

Done.

+  static_cast<uint8*>(memory)[offset] = v;
+}
+
+inline uint8 Get8(const void* memory, size_t offset) {
+  return static_cast<const uint8*>(memory)[offset];
+}
+
+inline void SetBE64(void* memory, uint64 v) {
+  Set8(memory, 0, static_cast<uint8>(v >> 56));
+  Set8(memory, 1, static_cast<uint8>(v >> 48));
+  Set8(memory, 2, static_cast<uint8>(v >> 40));
+  Set8(memory, 3, static_cast<uint8>(v >> 32));
+  Set8(memory, 4, static_cast<uint8>(v >> 24));
+  Set8(memory, 5, static_cast<uint8>(v >> 16));
+  Set8(memory, 6, static_cast<uint8>(v >>  8));
+  Set8(memory, 7, static_cast<uint8>(v >>  0));
+}
+
+inline uint64 GetBE64(const void* memory) {
+  return (static_cast<uint64>(Get8(memory, 0)) << 56)
+      | (static_cast<uint64>(Get8(memory, 1)) << 48)

[Sergey Ulanov, 2011/06/22 23:51:35]

nit: don't wrap operators, '|' should be at the end of the previous line.

[Alpha Left Google, 2011/06/23 21:53:01]

Done.

+      | (static_cast<uint64>(Get8(memory, 2)) << 40)
+      | (static_cast<uint64>(Get8(memory, 3)) << 32)
+      | (static_cast<uint64>(Get8(memory, 4)) << 24)
+      | (static_cast<uint64>(Get8(memory, 5)) << 16)
+      | (static_cast<uint64>(Get8(memory, 6)) <<  8)
+      | (static_cast<uint64>(Get8(memory, 7)) <<  0);
+}
+
+}  // namespace
+
+////////////////////////////////////////////////////////////////////////////
+// SecureP2PSocket Implementation.
+SecureP2PSocket::SecureP2PSocket(Socket* socket, const std::string& ice_key)
+    : socket_(socket),
+      write_seq_(0),
+      read_seq_(0),
+      user_read_callback_(NULL),
+      user_read_buf_len_(0),
+      user_write_callback_(NULL),
+      user_write_buf_len_(0),
+      ALLOW_THIS_IN_INITIALIZER_LIST(
+          read_callback_(NewCallback(this, &SecureP2PSocket::ReadDone))),
+      read_buf_(new net::IOBufferWithSize(kReadBufferSize)),
+      ALLOW_THIS_IN_INITIALIZER_LIST(
+          write_callback_(NewCallback(this, &SecureP2PSocket::WriteDone))),
+      msg_hasher_(crypto::HMAC::SHA1) {
+  // Make sure the key is valid.
+  CHECK(ice_key.size() == kKeySize);
+
+  // Create the mask key from ice key.
+  crypto::HMAC mask_hasher(crypto::HMAC::SHA1);
+  bool ret = mask_hasher.Init(
+      reinterpret_cast<const unsigned char*>(ice_key.data()), kKeySize);
+  DCHECK(ret) << "Initialize HMAC-SHA1 for mask failed.";
+  scoped_array<uint8> mask_digest(new uint8[mask_hasher.DigestLength()]);
+  mask_hasher.Sign(kMaskSaltStr, mask_digest.get(),
+                   mask_hasher.DigestLength());
+  mask_key_.reset(crypto::SymmetricKey::Import(
+      crypto::SymmetricKey::AES,
+      std::string(mask_digest.get(), mask_digest.get() + kKeySize)));
+  DCHECK(mask_key_.get()) << "Import symmetric key failed.";
+
+  // Initialize the encryptor with mask key.
+  encryptor_.Init(mask_key_.get(), crypto::Encryptor::CTR, "");
+
+  // Create the hash key from ice key.
+  crypto::HMAC hash_hasher(crypto::HMAC::SHA1);
+  ret = hash_hasher.Init(
+      reinterpret_cast<const unsigned char*>(ice_key.data()), kKeySize);
+  DCHECK(ret) << "Initialize HMAC-SHA1 for hash failed.";
+  scoped_array<uint8> hash_key(new uint8[hash_hasher.DigestLength()]);
+  hash_hasher.Sign(kHashSaltStr, hash_key.get(), hash_hasher.DigestLength());
+
+  // Create a hasher for message.
+  ret = msg_hasher_.Init(hash_key.get(), kKeySize);
+  DCHECK(ret) << "Initialize HMAC-SHA1 for message failed.";
+}
+
+int SecureP2PSocket::Read(IOBuffer* buf, int buf_len,
+                          CompletionCallback* callback) {
+  DCHECK(!user_read_buf_);
+  DCHECK(!user_read_buf_len_);
+  DCHECK(!user_read_callback_);
+
+  user_read_buf_ = buf;
+  user_read_buf_len_ = buf_len;
+  user_read_callback_ = callback;
+  return ReadInternal();
+}
+
+int SecureP2PSocket::Write(IOBuffer* buf, int buf_len,
+                           CompletionCallback* callback) {
+  // See the spec for the steps taken in this method:
+  // http://www.whatwg.org/specs/web-apps/current-work/complete/video-conferencing-and-peer-to-peer-communication.html#peer-to-peer-connections
+  // 4. Increment sequence number by one.
+  ++write_seq_;
+
+  const int kEncryptedBufferSize = kHeaderSize + buf_len;

[Sergey Ulanov, 2011/06/22 23:51:35]

This should not be a const.

[Alpha Left Google, 2011/06/23 21:53:01]

This is just to make sure I don't modify it later.

[Sergey Ulanov, 2011/06/23 23:12:45]

call it encrypted_buffer_size?

+  scoped_refptr<net::IOBuffer> encrypted_buf =
+      new net::IOBuffer(kEncryptedBufferSize);
+
+  // 6. Concatenate to form the raw message.
+  const int kRawMessageSize = kSeqNumberSize + kFrameTypeSize + buf_len;

[Sergey Ulanov, 2011/06/22 23:51:35]

why is this a const? it depends on buf_len

[Alpha Left Google, 2011/06/23 21:53:01]

Just so that I never modify it.

[Sergey Ulanov, 2011/06/23 23:12:45]

raw_message_size?

+  std::string raw_message;
+  raw_message.resize(kRawMessageSize);
+  char* raw_message_buf = const_cast<char*>(raw_message.data());
+  SetBE64(raw_message_buf, write_seq_);
+  memcpy(raw_message_buf + kSeqNumberSize, kFrameType,
+         kFrameTypeSize);
+  memcpy(raw_message_buf + kSeqNumberSize + kFrameTypeSize,
+         buf->data(), buf_len);
+
+  // 7. Decrypt the message.

[Sergey Ulanov, 2011/06/22 23:51:35]

s/Decrypt/Encrypt/

[Alpha Left Google, 2011/06/23 21:53:01]

Done.

+  std::string nonce = base::RandBytesAsString(kKeySize);
+  CHECK(encryptor_.UpdateCounter(nonce));
+  std::string encrypted_message;
+  CHECK(encryptor_.Encrypt(raw_message, &encrypted_message));
+  memcpy(encrypted_buf->data() + kRawMessagePosition,
+         encrypted_message.data(), encrypted_message.size());
+
+  // 8. Concatenate nonce and encrypted message to form masked message.
+  memcpy(encrypted_buf->data() + kNoncePosition, nonce.data(), kKeySize);
+
+  // 10. Create hash from masked message with nonce.
+  scoped_array<uint8> msg_digest(new uint8[msg_hasher_.DigestLength()]);
+  msg_hasher_.Sign(
+      base::StringPiece(encrypted_buf->data() + kNoncePosition,
+                        kRawMessageSize + kKeySize),
+      msg_digest.get(), msg_hasher_.DigestLength());
+  memcpy(encrypted_buf->data() + kHashPosition, msg_digest.get(), kKeySize);
+
+  // Write to the socket.
+  int ret = socket_->Write(encrypted_buf, kEncryptedBufferSize,
+                           write_callback_.get());
+  if (ret == net::ERR_IO_PENDING) {
+    DCHECK(callback);
+    user_write_callback_ = callback;
+    user_write_buf_len_ = buf_len;
+    return ret;
+  } else if (ret < 0) {
+    return ret;
+  }
+  DCHECK_EQ(buf_len + kHeaderSize, ret);
+  return buf_len;
+}
+
+bool SecureP2PSocket::SetReceiveBufferSize(int32 size) {
+  return true;
+}
+
+bool SecureP2PSocket::SetSendBufferSize(int32 size) {
+  return true;
+}
+
+int SecureP2PSocket::ReadInternal() {
+  int ret = socket_->Read(read_buf_, kReadBufferSize, read_callback_.get());
+  if (ret == net::ERR_IO_PENDING || ret < 0)
+    return ret;
+
+  ret = DecryptBuffer(ret);
+  if (ret == net::ERR_INVALID_RESPONSE)
+    return ReadInternal();

[Sergey Ulanov, 2011/06/22 23:51:35]

It looks wrong that ReadInternal() is recursive. Replace with a while loop?

[Alpha Left Google, 2011/06/23 21:53:01]

I changed this code to simply return ret so that upper level should handle it.

[Sergey Ulanov, 2011/06/23 23:12:45]

I think it's better to handle it here. In most cases (at least for remoting
code) reader stops reading after the first error, so an attacker can just send
invalid packet to terminate a session. Protection from invalid packets is
provided by the encryption layer, so it should handle it, not the upper layer.

+
+  user_read_buf_ = NULL;
+  user_read_buf_len_ = 0;
+  user_read_callback_ = NULL;
+  return ret;
+}
+
+void SecureP2PSocket::ReadDone(int err) {
+  net::CompletionCallback* callback = user_read_callback_;
+  user_read_callback_ = NULL;
+
+  if (err < 0) {
+    user_read_buf_len_ = 0;
+    user_read_buf_ = NULL;
+    callback->Run(err);
+    return;
+  }
+
+  int ret = DecryptBuffer(err);
+  if (ret == net::ERR_INVALID_RESPONSE)
+    ret = ReadInternal();
+  if (ret == net::ERR_IO_PENDING)
+    return;
+
+  user_read_buf_ = NULL;
+  user_read_buf_len_ = 0;
+  callback->Run(ret);
+}
+
+void SecureP2PSocket::WriteDone(int err) {
+  net::CompletionCallback* callback = user_write_callback_;
+  int buf_len = user_write_buf_len_;
+
+  user_write_callback_ = NULL;
+  user_write_buf_len_ = 0;
+
+  if (err >= 0) {
+    DCHECK_EQ(buf_len + kHeaderSize, err);
+    callback->Run(buf_len);
+    return;
+  }
+  callback->Run(err);
+}
+
+int SecureP2PSocket::DecryptBuffer(int size) {
+  if (size > user_read_buf_len_)
+    return net::ERR_MSG_TOO_BIG;

[Sergey Ulanov, 2011/06/22 23:51:35]

UDP sockets just truncate packets in case read buffer is not big enough. I think
it's acceptable to do it here too. Usually Read() doesn't return
ERR_MSG_TOO_BIG.

[Alpha Left Google, 2011/06/23 21:53:01]

Done.

+  if (size < kRawMessagePosition)
+    return net::ERR_INVALID_RESPONSE;
+
+  // See the spec for the steps taken in this method:
+  // http://www.whatwg.org/specs/web-apps/current-work/complete/video-conferencing-and-peer-to-peer-communication.html#peer-to-peer-connections
+  // 5. Compute hash of the message.
+  scoped_array<uint8> msg_digest(new uint8[msg_hasher_.DigestLength()]);
+  msg_hasher_.Sign(
+      base::StringPiece(read_buf_->data() + kNoncePosition,
+                        size - kNoncePosition),
+      msg_digest.get(), msg_hasher_.DigestLength());
+
+  // 6. Compare the hash values.
+  int ret = memcmp(read_buf_->data(), msg_digest.get(), kKeySize);
+  if (ret)
+    return net::ERR_INVALID_RESPONSE;
+
+  // 7. Decrypt the message.
+  std::string nonce = std::string(
+      read_buf_->data() + kNoncePosition, kKeySize);
+  CHECK(encryptor_.UpdateCounter(nonce));
+  const int kRawMessageSize = size - kRawMessagePosition;
+
+  // TODO(hclam): Change Encryptor API to trim this memcpy.
+  std::string encrypted_message(read_buf_->data() + kRawMessagePosition,
+                                kRawMessageSize);
+  std::string raw_message;
+  CHECK(encryptor_.Decrypt(encrypted_message, &raw_message));
+
+  if (kRawMessageSize < kSeqNumberSize)
+    return net::ERR_INVALID_RESPONSE;
+
+  // 12. Read the sequence number.
+  uint64 seq_number = GetBE64(raw_message.data());
+
+  // 13. Reject if this packet is too old.
+  // TODO(hclam): Determine if we should follow the spec. The spec says drop
+  // the packet and don't do reordering but we can do better.
+  if (false && seq_number < read_seq_)

[Sergey Ulanov, 2011/06/22 23:51:35]

why "if(false)" ? Maybe just remove this code and add comments somewhere saying
that the behavior of this code doesn't completely confirm to the spec.

[Alpha Left Google, 2011/06/23 21:53:01]

Done.

+    return net::ERR_INVALID_RESPONSE;
+
+  // 14. Save the most recent sequence number.
+  read_seq_ = seq_number;
+
+  // 15. Parse the frame type.
+  if (kRawMessageSize < kSeqNumberSize + kFrameTypeSize)
+    return net::ERR_INVALID_RESPONSE;
+  ret = memcmp(raw_message.data() + kSeqNumberSize, kFrameType,
+               kFrameTypeSize);
+  if (ret)
+    return net::ERR_INVALID_RESPONSE;
+
+  // 16. Read the message.
+  const int kMessageSize = kRawMessageSize - kSeqNumberSize - kFrameTypeSize;
+  memcpy(user_read_buf_->data(),
+         raw_message.data() + kSeqNumberSize + kFrameTypeSize, kMessageSize);
+  return kMessageSize;
+}
+
+}  // namespace protocol
+}  // namespace remoting