[Fido][BLE] Fix Bugs in FidoBleTransaction

This change fixes a number of bugs in FidoBleTransaction and adds
corresponding tests. In particular, it fixes a race condition, where
the completion callback was run before the last write was acknowlegded.
In addition, it improves the robustness with regard to malformed input.

Bug: 880053
Change-Id: I81f6d5c02d01b9d7ea1a184a7236cd74d52f356d
Reviewed-on: https://chromium-review.googlesource.com/1224383
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592398}

device/BUILD.gn

@@ -73,6 +73,7 @@ test("device_unittests") {
     "fido/ble/fido_ble_connection_unittest.cc",
     "fido/ble/fido_ble_device_unittest.cc",
     "fido/ble/fido_ble_frames_unittest.cc",
+    "fido/ble/fido_ble_transaction_unittest.cc",
     "fido/ble_adapter_power_manager_unittest.cc",
     "fido/cable/fido_cable_device_unittest.cc",
     "fido/cable/fido_cable_discovery_unittest.cc",

device/fido/ble/fido_ble_frames.cc

@@ -6,6 +6,7 @@
 
 #include <algorithm>
 #include <limits>
+#include <tuple>
 
 #include "base/logging.h"
 #include "base/numerics/safe_conversions.h"
@@ -20,6 +21,9 @@ FidoBleFrame::FidoBleFrame(FidoBleDeviceCommand command,
                            std::vector<uint8_t> data)
     : command_(command), data_(std::move(data)) {}
 
+FidoBleFrame::FidoBleFrame(const FidoBleFrame&) = default;
+FidoBleFrame& FidoBleFrame::operator=(const FidoBleFrame&) = default;
+
 FidoBleFrame::FidoBleFrame(FidoBleFrame&&) = default;
 FidoBleFrame& FidoBleFrame::operator=(FidoBleFrame&&) = default;
 
@@ -82,6 +86,11 @@ FidoBleFrame::ToFragments(size_t max_fragment_size) const {
   return {initial_fragment, std::move(other_fragments)};
 }
 
+bool operator==(const FidoBleFrame& lhs, const FidoBleFrame& rhs) {
+  return std::forward_as_tuple(lhs.command(), lhs.data()) ==
+         std::forward_as_tuple(rhs.command(), rhs.data());
+}
+
 FidoBleFrameFragment::FidoBleFrameFragment() = default;
 
 FidoBleFrameFragment::FidoBleFrameFragment(const FidoBleFrameFragment& frame) =

device/fido/ble/fido_ble_frames.h

@@ -59,6 +59,9 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoBleFrame {
   FidoBleFrame();
   FidoBleFrame(FidoBleDeviceCommand command, std::vector<uint8_t> data);
 
+  FidoBleFrame(const FidoBleFrame&);
+  FidoBleFrame& operator=(const FidoBleFrame&);
+
   FidoBleFrame(FidoBleFrame&&);
   FidoBleFrame& operator=(FidoBleFrame&&);
 
@@ -86,10 +89,11 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoBleFrame {
  private:
   FidoBleDeviceCommand command_ = FidoBleDeviceCommand::kMsg;
   std::vector<uint8_t> data_;
-
-  DISALLOW_COPY_AND_ASSIGN(FidoBleFrame);
 };
 
+COMPONENT_EXPORT(DEVICE_FIDO)
+bool operator==(const FidoBleFrame& lhs, const FidoBleFrame& rhs);
+
 // A single frame sent over BLE may be split over multiple writes and
 // notifications because the technology was not designed for large messages.
 // This class represents a single fragment. Not to be used directly.

device/fido/ble/fido_ble_transaction.cc

@@ -6,6 +6,7 @@
 
 #include <utility>
 
+#include "base/threading/thread_task_runner_handle.h"
 #include "device/fido/ble/fido_ble_connection.h"
 #include "device/fido/fido_constants.h"
 
@@ -23,6 +24,13 @@ FidoBleTransaction::~FidoBleTransaction() = default;
 
 void FidoBleTransaction::WriteRequestFrame(FidoBleFrame request_frame,
                                            FrameCallback callback) {
+  if (control_point_length_ < 3u) {
+    VLOG(2) << "Control Point Length is too short: " << control_point_length_;
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::BindOnce(std::move(callback), base::nullopt));
+    return;
+  }
+
   DCHECK(!request_frame_ && callback_.is_null());
   request_frame_ = std::move(request_frame);
   callback_ = std::move(callback);
@@ -37,6 +45,8 @@ void FidoBleTransaction::WriteRequestFragment(
     const FidoBleFrameFragment& fragment) {
   buffer_.clear();
   fragment.Serialize(&buffer_);
+  DCHECK(!has_pending_request_fragment_write_);
+  has_pending_request_fragment_write_ = true;
   // A weak pointer is required, since this call might time out. If that
   // happens, the current FidoBleTransaction could be destroyed.
   connection_->WriteControlPoint(
@@ -48,22 +58,31 @@ void FidoBleTransaction::WriteRequestFragment(
 }
 
 void FidoBleTransaction::OnRequestFragmentWritten(bool success) {
+  DCHECK(has_pending_request_fragment_write_);
+  has_pending_request_fragment_write_ = false;
   StopTimeout();
   if (!success) {
     OnError(base::nullopt);
     return;
   }
 
-  if (request_cont_fragments_.empty()) {
-    // The transaction wrote the full request frame. A response should follow
-    // soon after.
-    StartTimeout();
+  if (!request_cont_fragments_.empty()) {
+    auto next_request_fragment = std::move(request_cont_fragments_.front());
+    request_cont_fragments_.pop();
+    WriteRequestFragment(next_request_fragment);
+    return;
+  }
+
+  // The transaction wrote the full request frame. It is possible that the full
+  // response frame was already received, at which point we process it and run
+  // the completim callback.
+  if (response_frame_assembler_ && response_frame_assembler_->IsDone()) {
+    ProcessResponseFrame();
     return;
   }
 
-  auto next_request_fragment = std::move(request_cont_fragments_.front());
-  request_cont_fragments_.pop();
-  WriteRequestFragment(next_request_fragment);
+  // Otherwise, a response should follow soon after.
+  StartTimeout();
 }
 
 void FidoBleTransaction::OnResponseFragment(std::vector<uint8_t> data) {
@@ -71,7 +90,7 @@ void FidoBleTransaction::OnResponseFragment(std::vector<uint8_t> data) {
   if (!response_frame_assembler_) {
     FidoBleFrameInitializationFragment fragment;
     if (!FidoBleFrameInitializationFragment::Parse(data, &fragment)) {
-      DLOG(ERROR) << "Malformed Frame Initialization Fragment";
+      LOG(ERROR) << "Malformed Frame Initialization Fragment";
       OnError(base::nullopt);
       return;
     }
@@ -79,13 +98,12 @@ void FidoBleTransaction::OnResponseFragment(std::vector<uint8_t> data) {
     response_frame_assembler_.emplace(fragment);
   } else {
     FidoBleFrameContinuationFragment fragment;
-    if (!FidoBleFrameContinuationFragment::Parse(data, &fragment)) {
-      DLOG(ERROR) << "Malformed Frame Continuation Fragment";
+    if (!FidoBleFrameContinuationFragment::Parse(data, &fragment) ||
+        !response_frame_assembler_->AddFragment(fragment)) {
+      LOG(ERROR) << "Malformed Frame Continuation Fragment";
       OnError(base::nullopt);
       return;
     }
-
-    response_frame_assembler_->AddFragment(fragment);
   }
 
   if (!response_frame_assembler_->IsDone()) {
@@ -94,12 +112,18 @@ void FidoBleTransaction::OnResponseFragment(std::vector<uint8_t> data) {
     return;
   }
 
-  FidoBleFrame frame = std::move(*response_frame_assembler_->GetFrame());
-  response_frame_assembler_.reset();
-  ProcessResponseFrame(std::move(frame));
+  // It is possible to receive the last response fragment before the write of
+  // the last request fragment has been acknowledged. If this is the case, do
+  // not run the completion callback yet.
+  if (!has_pending_request_fragment_write_)
+    ProcessResponseFrame();
 }
 
-void FidoBleTransaction::ProcessResponseFrame(FidoBleFrame response_frame) {
+void FidoBleTransaction::ProcessResponseFrame() {
+  DCHECK(response_frame_assembler_ && response_frame_assembler_->IsDone());
+  auto response_frame = std::move(*response_frame_assembler_->GetFrame());
+  response_frame_assembler_.reset();
+
   DCHECK(request_frame_.has_value());
   if (response_frame.command() == request_frame_->command()) {
     request_frame_.reset();
@@ -108,19 +132,35 @@ void FidoBleTransaction::ProcessResponseFrame(FidoBleFrame response_frame) {
   }
 
   if (response_frame.command() == FidoBleDeviceCommand::kKeepAlive) {
-    DVLOG(2) << "CMD_KEEPALIVE: "
-             << static_cast<uint8_t>(response_frame.GetKeepaliveCode());
+    if (!response_frame.IsValid()) {
+      LOG(ERROR) << "Got invald KeepAlive Command.";
+      OnError(base::nullopt);
+      return;
+    }
+
+    VLOG(2) << "CMD_KEEPALIVE: "
+            << static_cast<int>(response_frame.GetKeepaliveCode());
     // Expect another reponse frame soon.
     StartTimeout();
     return;
   }
 
-  DCHECK_EQ(response_frame.command(), FidoBleDeviceCommand::kError);
-  DLOG(ERROR) << "CMD_ERROR: "
-              << static_cast<uint8_t>(response_frame.GetErrorCode());
-  OnError(response_frame.IsValid()
-              ? base::make_optional(std::move(response_frame))
-              : base::nullopt);
+  if (response_frame.command() == FidoBleDeviceCommand::kError) {
+    if (!response_frame.IsValid()) {
+      LOG(ERROR) << "Got invald Error Command.";
+      OnError(base::nullopt);
+      return;
+    }
+
+    LOG(ERROR) << "CMD_ERROR: "
+               << static_cast<int>(response_frame.GetErrorCode());
+    OnError(std::move(response_frame));
+    return;
+  }
+
+  LOG(ERROR) << "Got unexpected Command: "
+             << static_cast<int>(response_frame.command());
+  OnError(base::nullopt);
 }
 
 void FidoBleTransaction::StartTimeout() {
@@ -137,7 +177,9 @@ void FidoBleTransaction::OnError(base::Optional<FidoBleFrame> response_frame) {
   request_frame_.reset();
   request_cont_fragments_ = base::queue<FidoBleFrameContinuationFragment>();
   response_frame_assembler_.reset();
-  std::move(callback_).Run(std::move(response_frame));
+  // |callback_| might have been run due to a previous error.
+  if (callback_)
+    std::move(callback_).Run(std::move(response_frame));
 }
 
 }  // namespace device

device/fido/ble/fido_ble_transaction.h

@@ -8,6 +8,7 @@
 #include <memory>
 #include <vector>
 
+#include "base/component_export.h"
 #include "base/containers/queue.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
@@ -22,7 +23,7 @@ class FidoBleConnection;
 // This class encapsulates logic related to a single U2F BLE request and
 // response. FidoBleTransaction is owned by FidoBleDevice, which is the only
 // class that should make use of this class.
-class FidoBleTransaction {
+class COMPONENT_EXPORT(DEVICE_FIDO) FidoBleTransaction {
  public:
   using FrameCallback = base::OnceCallback<void(base::Optional<FidoBleFrame>)>;
 
@@ -36,7 +37,7 @@ class FidoBleTransaction {
  private:
   void WriteRequestFragment(const FidoBleFrameFragment& fragment);
   void OnRequestFragmentWritten(bool success);
-  void ProcessResponseFrame(FidoBleFrame response_frame);
+  void ProcessResponseFrame();
 
   void StartTimeout();
   void StopTimeout();
@@ -54,6 +55,8 @@ class FidoBleTransaction {
   std::vector<uint8_t> buffer_;
   base::OneShotTimer timer_;
 
+  bool has_pending_request_fragment_write_ = false;
+
   base::WeakPtrFactory<FidoBleTransaction> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(FidoBleTransaction);