device/fido: remove FidoCableDevice's friends.

There's a unittest that reaches into FidoCableDevice's internals, and
thus needs to be a friend class, in order to check that the sequence
numbers are being incremented. But the prior test already confirms that
the encryption and decrypt is working as expected by checking inputs and
outputs, and testing the behaviour is to be preferred to testing the
internals.

Therefore, delete that superfluous test and eliminate one of the
friends.

Another test needs to set the sequence counters. Better to add a testing
interface for that than to expose all private members. Thus another
friend can be dropped.

Then |FidoCableDevice::EncryptionData| can be made private.

Change-Id: I42ea49b14b4a4b4e0afa2a4e5490fa9075439fe8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1776796
Commit-Queue: Adam Langley <agl@chromium.org>
Reviewed-by: Martin Kreichgauer <martinkr@google.com>
Cr-Commit-Position: refs/heads/master@{#692301}

device/fido/cable/fido_cable_device.cc

@@ -39,78 +39,9 @@ base::Optional<std::vector<uint8_t>> ConstructEncryptionNonce(
   return constructed_nonce;
 }
 
-bool EncryptOutgoingMessage(
-    const base::Optional<FidoCableDevice::EncryptionData>& encryption_data,
-    std::vector<uint8_t>* message_to_encrypt) {
-  if (!encryption_data)
-    return false;
-
-  const auto nonce = ConstructEncryptionNonce(
-      encryption_data->nonce, true /* is_sender_client */,
-      encryption_data->write_sequence_num);
-  if (!nonce)
-    return false;
-
-  crypto::Aead aes_key(crypto::Aead::AES_256_GCM);
-  aes_key.Init(encryption_data->session_key);
-  DCHECK_EQ(nonce->size(), aes_key.NonceLength());
-
-  const uint8_t additional_data[1] = {
-      base::strict_cast<uint8_t>(FidoBleDeviceCommand::kMsg)};
-  std::vector<uint8_t> ciphertext =
-      aes_key.Seal(*message_to_encrypt, *nonce, additional_data);
-  message_to_encrypt->swap(ciphertext);
-  return true;
-}
-
-bool DecryptIncomingMessage(
-    const base::Optional<FidoCableDevice::EncryptionData>& encryption_data,
-    FidoBleFrame* incoming_frame) {
-  if (!encryption_data)
-    return false;
-
-  const auto nonce = ConstructEncryptionNonce(
-      encryption_data->nonce, false /* is_sender_client */,
-      encryption_data->read_sequence_num);
-  if (!nonce)
-    return false;
-
-  crypto::Aead aes_key(crypto::Aead::AES_256_GCM);
-  aes_key.Init(encryption_data->session_key);
-  DCHECK_EQ(nonce->size(), aes_key.NonceLength());
-
-  const uint8_t additional_data[1] = {
-      base::strict_cast<uint8_t>(incoming_frame->command())};
-  base::Optional<std::vector<uint8_t>> plaintext =
-      aes_key.Open(incoming_frame->data(), *nonce, additional_data);
-  if (!plaintext) {
-    FIDO_LOG(ERROR) << "Failed to decrypt caBLE message.";
-    return false;
-  }
-
-  incoming_frame->data().swap(*plaintext);
-  return true;
-}
-
 }  // namespace
 
-// FidoCableDevice::EncryptionData ----------------------------------------
-
-FidoCableDevice::EncryptionData::EncryptionData(
-    base::span<const uint8_t, 32> encryption_key,
-    base::span<const uint8_t, 8> nonce)
-    : session_key(fido_parsing_utils::Materialize(encryption_key)),
-      nonce(fido_parsing_utils::Materialize(nonce)) {}
-
-FidoCableDevice::EncryptionData::EncryptionData(EncryptionData&& data) =
-    default;
-
-FidoCableDevice::EncryptionData& FidoCableDevice::EncryptionData::operator=(
-    EncryptionData&& other) = default;
-
-FidoCableDevice::EncryptionData::~EncryptionData() = default;
-
-// FidoCableDevice::EncryptionData ----------------------------------------
+FidoCableDevice::EncryptionData::EncryptionData() = default;
 
 FidoCableDevice::FidoCableDevice(BluetoothAdapter* adapter, std::string address)
     : FidoBleDevice(adapter, std::move(address)) {}
@@ -123,7 +54,8 @@ FidoCableDevice::~FidoCableDevice() = default;
 FidoDevice::CancelToken FidoCableDevice::DeviceTransact(
     std::vector<uint8_t> command,
     DeviceCallback callback) {
-  if (!EncryptOutgoingMessage(encryption_data_, &command)) {
+  if (!encryption_data_ ||
+      !EncryptOutgoingMessage(*encryption_data_, &command)) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::BindOnce(std::move(callback), base::nullopt));
     state_ = State::kDeviceError;
@@ -145,7 +77,8 @@ void FidoCableDevice::OnResponseFrame(FrameCallback callback,
   state_ = frame ? State::kReady : State::kDeviceError;
 
   if (frame && frame->command() != FidoBleDeviceCommand::kControl) {
-    if (!DecryptIncomingMessage(encryption_data_, &frame.value())) {
+    if (!encryption_data_ ||
+        !DecryptIncomingMessage(*encryption_data_, &frame.value())) {
       state_ = State::kDeviceError;
       frame = base::nullopt;
     }
@@ -177,11 +110,68 @@ void FidoCableDevice::SetEncryptionData(
     base::span<const uint8_t, 8> nonce) {
   // Encryption data must be set at most once during Cable handshake protocol.
   DCHECK(!encryption_data_);
-  encryption_data_.emplace(session_key, nonce);
+  encryption_data_.emplace();
+  encryption_data_->session_key = fido_parsing_utils::Materialize(session_key);
+  encryption_data_->nonce = fido_parsing_utils::Materialize(nonce);
 }
 
 FidoTransportProtocol FidoCableDevice::DeviceTransport() const {
   return FidoTransportProtocol::kCloudAssistedBluetoothLowEnergy;
 }
 
+void FidoCableDevice::SetSequenceNumbersForTesting(uint32_t read_seq,
+                                                   uint32_t write_seq) {
+  encryption_data_->write_sequence_num = write_seq;
+  encryption_data_->read_sequence_num = read_seq;
+}
+
+// static
+bool FidoCableDevice::EncryptOutgoingMessage(
+    const EncryptionData& encryption_data,
+    std::vector<uint8_t>* message_to_encrypt) {
+  const auto nonce = ConstructEncryptionNonce(
+      encryption_data.nonce, true /* is_sender_client */,
+      encryption_data.write_sequence_num);
+  if (!nonce)
+    return false;
+
+  crypto::Aead aes_key(crypto::Aead::AES_256_GCM);
+  aes_key.Init(encryption_data.session_key);
+  DCHECK_EQ(nonce->size(), aes_key.NonceLength());
+
+  const uint8_t additional_data[1] = {
+      base::strict_cast<uint8_t>(FidoBleDeviceCommand::kMsg)};
+  std::vector<uint8_t> ciphertext =
+      aes_key.Seal(*message_to_encrypt, *nonce, additional_data);
+  message_to_encrypt->swap(ciphertext);
+  return true;
+}
+
+// static
+bool FidoCableDevice::DecryptIncomingMessage(
+    const EncryptionData& encryption_data,
+    FidoBleFrame* incoming_frame) {
+  const auto nonce = ConstructEncryptionNonce(
+      encryption_data.nonce, false /* is_sender_client */,
+      encryption_data.read_sequence_num);
+  if (!nonce)
+    return false;
+
+  crypto::Aead aes_key(crypto::Aead::AES_256_GCM);
+  aes_key.Init(encryption_data.session_key);
+  DCHECK_EQ(nonce->size(), aes_key.NonceLength());
+
+  const uint8_t additional_data[1] = {
+      base::strict_cast<uint8_t>(incoming_frame->command())};
+  base::Optional<std::vector<uint8_t>> plaintext =
+      aes_key.Open(incoming_frame->data(), *nonce, additional_data);
+  if (!plaintext) {
+    FIDO_LOG(ERROR) << "Failed to decrypt caBLE message.";
+    return false;
+  }
+
+  incoming_frame->data().swap(*plaintext);
+  return true;
+}
+
 }  // namespace device

device/fido/cable/fido_cable_device.h

@@ -26,23 +26,6 @@ class FidoBleFrame;
 
 class COMPONENT_EXPORT(DEVICE_FIDO) FidoCableDevice : public FidoBleDevice {
  public:
-  // Encapsulates state FidoCableDevice maintains to encrypt and decrypt
-  // data within FidoBleFrame.
-  struct COMPONENT_EXPORT(DEVICE_FIDO) EncryptionData {
-    EncryptionData(base::span<const uint8_t, 32> session_key,
-                   base::span<const uint8_t, 8> nonce);
-    EncryptionData(EncryptionData&& data);
-    EncryptionData& operator=(EncryptionData&& other);
-    ~EncryptionData();
-
-    std::array<uint8_t, 32> session_key;
-    std::array<uint8_t, 8> nonce;
-    uint32_t write_sequence_num = 0;
-    uint32_t read_sequence_num = 0;
-
-    DISALLOW_COPY_AND_ASSIGN(EncryptionData);
-  };
-
   using FrameCallback = FidoBleTransaction::FrameCallback;
 
   FidoCableDevice(BluetoothAdapter* adapter, std::string address);
@@ -64,11 +47,27 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoCableDevice : public FidoBleDevice {
                          base::span<const uint8_t, 8> nonce);
   FidoTransportProtocol DeviceTransport() const override;
 
+  // SetCountersForTesting allows tests to set the message counters. Non-test
+  // code must not call this function.
+  void SetSequenceNumbersForTesting(uint32_t read_counter,
+                                    uint32_t write_counter);
+
  private:
-  FRIEND_TEST_ALL_PREFIXES(FidoCableDeviceTest,
-                           TestCableDeviceSendMultipleRequests);
-  FRIEND_TEST_ALL_PREFIXES(FidoCableDeviceTest,
-                           TestCableDeviceErrorOnMaxCounter);
+  // Encapsulates state FidoCableDevice maintains to encrypt and decrypt
+  // data within FidoBleFrame.
+  struct EncryptionData {
+    EncryptionData();
+
+    std::array<uint8_t, 32> session_key;
+    std::array<uint8_t, 8> nonce;
+    uint32_t write_sequence_num = 0;
+    uint32_t read_sequence_num = 0;
+  };
+
+  static bool EncryptOutgoingMessage(const EncryptionData& encryption_data,
+                                     std::vector<uint8_t>* message_to_encrypt);
+  static bool DecryptIncomingMessage(const EncryptionData& encryption_data,
+                                     FidoBleFrame* incoming_frame);
 
   base::Optional<EncryptionData> encryption_data_;
   base::WeakPtrFactory<FidoCableDevice> weak_factory_{this};

device/fido/cable/fido_cable_device_unittest.cc

@@ -174,34 +174,6 @@ TEST_F(FidoCableDeviceTest, TestCaBleDeviceSendData) {
   ConnectWithLength(kControlPointLength);
 
   EXPECT_CALL(*connection(), WriteControlPointPtr(_, _))
-      .WillOnce(Invoke([this](const auto& data, auto* cb) {
-        base::SequencedTaskRunnerHandle::Get()->PostTask(
-            FROM_HERE, base::BindOnce(std::move(*cb), true));
-
-        const auto authenticator_reply = authenticator()->ReplyWithSameMessage(
-            base::make_span(data).subspan(kCTAPFramingLength));
-        base::SequencedTaskRunnerHandle::Get()->PostTask(
-            FROM_HERE, base::BindOnce(connection()->read_callback(),
-                                      ConstructSerializedOutgoingFragment(
-                                          authenticator_reply)));
-      }));
-
-  TestDeviceCallbackReceiver callback_receiver;
-  device()->DeviceTransact(fido_parsing_utils::Materialize(kTestData),
-                           callback_receiver.callback());
-
-  callback_receiver.WaitForCallback();
-  const auto& value = callback_receiver.value();
-  ASSERT_TRUE(value);
-  EXPECT_THAT(*value, ::testing::ElementsAreArray(kTestData));
-}
-
-// Test that FidoCableDevice properly updates counters when sending/receiving
-// multiple requests.
-TEST_F(FidoCableDeviceTest, TestCableDeviceSendMultipleRequests) {
-  ConnectWithLength(kControlPointLength);
-  EXPECT_CALL(*connection(), WriteControlPointPtr(_, _))
-      .Times(2)
       .WillRepeatedly(Invoke([this](const auto& data, auto* cb) {
         base::SequencedTaskRunnerHandle::Get()->PostTask(
             FROM_HERE, base::BindOnce(std::move(*cb), true));
@@ -214,30 +186,18 @@ TEST_F(FidoCableDeviceTest, TestCableDeviceSendMultipleRequests) {
                                           authenticator_reply)));
       }));
 
-  ASSERT_TRUE(device()->encryption_data_);
-  EXPECT_EQ(0u, device()->encryption_data_->write_sequence_num);
-  EXPECT_EQ(0u, device()->encryption_data_->read_sequence_num);
+  for (size_t i = 0; i < 3; i++) {
+    SCOPED_TRACE(i);
 
-  TestDeviceCallbackReceiver callback_receiver1;
-  device()->DeviceTransact(fido_parsing_utils::Materialize(kTestData),
-                           callback_receiver1.callback());
-  callback_receiver1.WaitForCallback();
-  const auto& value1 = callback_receiver1.value();
-  ASSERT_TRUE(value1);
-  EXPECT_THAT(*value1, ::testing::ElementsAreArray(kTestData));
-  EXPECT_EQ(1u, device()->encryption_data_->write_sequence_num);
-  EXPECT_EQ(1u, device()->encryption_data_->read_sequence_num);
-
-  constexpr uint8_t kTestData2[] = {'T', 'E', 'S', 'T', '2'};
-  TestDeviceCallbackReceiver callback_receiver2;
-  device()->DeviceTransact(fido_parsing_utils::Materialize(kTestData2),
-                           callback_receiver2.callback());
-  callback_receiver2.WaitForCallback();
-  const auto& value2 = callback_receiver2.value();
-  ASSERT_TRUE(value2);
-  EXPECT_THAT(*value2, ::testing::ElementsAreArray(kTestData2));
-  EXPECT_EQ(2u, device()->encryption_data_->write_sequence_num);
-  EXPECT_EQ(2u, device()->encryption_data_->read_sequence_num);
+    TestDeviceCallbackReceiver callback_receiver;
+    device()->DeviceTransact(fido_parsing_utils::Materialize(kTestData),
+                             callback_receiver.callback());
+
+    callback_receiver.WaitForCallback();
+    const auto& value = callback_receiver.value();
+    ASSERT_TRUE(value);
+    EXPECT_THAT(*value, ::testing::ElementsAreArray(kTestData));
+  }
 }
 
 TEST_F(FidoCableDeviceTest, TestCableDeviceFailOnIncorrectSessionKey) {
@@ -322,8 +282,7 @@ TEST_F(FidoCableDeviceTest, TestCableDeviceErrorOnMaxCounter) {
       }));
 
   TestDeviceCallbackReceiver callback_receiver;
-  ASSERT_TRUE(device()->encryption_data_);
-  device()->encryption_data_->read_sequence_num = kInvalidCounter;
+  device()->SetSequenceNumbersForTesting(kInvalidCounter, 0);
   device()->DeviceTransact(fido_parsing_utils::Materialize(kTestData),
                            callback_receiver.callback());