[Backport] CVE-2020-6493: Use after free in WebAuthentication

Manual backport of patch originally reviewed on
https://chromium-review.googlesource.com/c/chromium/src/+/2203776:
fido: improve guards against adding authenticators with identical IDs

Make FidoRequestHandler::AuthenticatorAdded() return early when an
FidoAuthenticator is added whose ID matches that of a previously added
authenticator. The request handler  previously did not add the
duplicate authenticator into its |active_authenticators_| map, but then
attempted to dispatch its request to it (or rather to an invalid
reference).

Also better guard against authenticators being removed during
initialization by making the (asynchronously run)
InitializeAuthenticatorAndDispatchRequest() method look up the
AuthenticatorState for the authenticator to be initialized by its ID
rather than passing around AuthenticatorState pointers that may have
been freed by the time the method runs because the authenticator went
away.

Lastly, derive VirtualFidoDevice IDs randomly. It previously used its
instance pointer address for "randomness" which, aside from being weird,
could lead to re-use of IDs. (FidoAuthenticator ID reuse in itself
_should_ not be a problem, but certainly could lead to bugs if the rest
of the code is less than careful about it.)

Bug: 1082105
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Reviewed-by: Christopher Thompson <cthomp@chromium.org>
Reviewed-by: Adam Langley <agl@chromium.org>
Cr-Commit-Position: refs/heads/master@{#770190}
Change-Id: Ie4e3fd39c3360bf0131cdd6dd33b2be4dbb225a8
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>

4 files changed, 34 insertions, 15 deletions

diff --git a/chromium/device/fido/fido_request_handler_base.cc b/chromium/device/fido/fido_request_handler_base.cc
index e6216746eb9..9908a241336 100644
--- a/chromium/device/fido/fido_request_handler_base.cc
+++ b/chromium/device/fido/fido_request_handler_base.cc
@@ -13,6 +13,7 @@
 #include "base/strings/string_piece.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "build/build_config.h"
+#include "components/device_event_log/device_event_log.h"
 #include "device/fido/ble_adapter_manager.h"
 #include "device/fido/fido_authenticator.h"
 #include "device/fido/fido_discovery_factory.h"
@@ -154,11 +155,7 @@ FidoRequestHandlerBase::~FidoRequestHandlerBase() {
 
 void FidoRequestHandlerBase::StartAuthenticatorRequest(
     const std::string& authenticator_id) {
-  auto authenticator_it = active_authenticators_.find(authenticator_id);
-  if (authenticator_it == active_authenticators_.end())
-    return;
-
-  InitializeAuthenticatorAndDispatchRequest(authenticator_it->second);
+  InitializeAuthenticatorAndDispatchRequest(authenticator_id);
 }
 
 void FidoRequestHandlerBase::CancelActiveAuthenticators(
@@ -293,9 +290,18 @@ void FidoRequestHandlerBase::DiscoveryStarted(
 void FidoRequestHandlerBase::AuthenticatorAdded(
     FidoDiscoveryBase* discovery,
     FidoAuthenticator* authenticator) {
-  DCHECK(authenticator &&
-         !base::Contains(active_authenticators(), authenticator->GetId()));
-  active_authenticators_.emplace(authenticator->GetId(), authenticator);
+  DCHECK(!authenticator->GetId().empty());
+  std::pair<AuthenticatorMap::iterator, bool> ret;
+  ret = active_authenticators_.insert(
+      std::pair<std::string, FidoAuthenticator*>(
+          authenticator->GetId(),
+          authenticator));
+  if (!ret.second) {
+    NOTREACHED();
+    FIDO_LOG(ERROR) << "Authenticator with duplicate ID "
+                    << authenticator->GetId();
+    return;
+  }
 
   // If |observer_| exists, dispatching request to |authenticator| is
   // delegated to |observer_|. Else, dispatch request to |authenticator|
@@ -317,7 +323,7 @@ void FidoRequestHandlerBase::AuthenticatorAdded(
         FROM_HERE,
         base::BindOnce(
             &FidoRequestHandlerBase::InitializeAuthenticatorAndDispatchRequest,
-            GetWeakPtr(), authenticator));
+            GetWeakPtr(), authenticator->GetId()));
   } else {
     VLOG(2) << "Embedder controls the dispatch.";
   }
@@ -346,7 +352,12 @@ void FidoRequestHandlerBase::NotifyObserverTransportAvailability() {
 }
 
 void FidoRequestHandlerBase::InitializeAuthenticatorAndDispatchRequest(
-    FidoAuthenticator* authenticator) {
+    const std::string& authenticator_id) {
+  auto authenticator_it = active_authenticators_.find(authenticator_id);
+  if (authenticator_it == active_authenticators_.end()) {
+    return;
+  }
+  FidoAuthenticator* authenticator = authenticator_it->second.get();
   authenticator->InitializeAuthenticator(
       base::BindOnce(&FidoRequestHandlerBase::DispatchRequest,
                      weak_factory_.GetWeakPtr(), authenticator));
diff --git a/chromium/device/fido/fido_request_handler_base.h b/chromium/device/fido/fido_request_handler_base.h
index 936ece7c9d6..c6373999f7d 100644
--- a/chromium/device/fido/fido_request_handler_base.h
+++ b/chromium/device/fido/fido_request_handler_base.h
@@ -270,7 +270,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
   // DispatchRequest(). InitializeAuthenticator() sends a GetInfo command
   // to FidoDeviceAuthenticator instances in order to determine their protocol
   // versions before a request can be dispatched.
-  void InitializeAuthenticatorAndDispatchRequest(FidoAuthenticator*);
+  void InitializeAuthenticatorAndDispatchRequest(const std::string& authenticator_id);
   void ConstructBleAdapterPowerManager();
 
   AuthenticatorMap active_authenticators_;
diff --git a/chromium/device/fido/virtual_fido_device.cc b/chromium/device/fido/virtual_fido_device.cc
index 67becb475ae..202d5c5d757 100644
--- a/chromium/device/fido/virtual_fido_device.cc
+++ b/chromium/device/fido/virtual_fido_device.cc
@@ -8,6 +8,7 @@
 #include <tuple>
 #include <utility>
 
+#include "base/rand_util.h"
 #include "crypto/ec_signature_creator.h"
 #include "device/fido/fido_parsing_utils.h"
 #include "third_party/boringssl/src/include/openssl/ec_key.h"
@@ -135,10 +136,10 @@ bool VirtualFidoDevice::State::InjectResidentKey(
                                     /*icon_url=*/base::nullopt));
 }
 
-VirtualFidoDevice::VirtualFidoDevice() = default;
-
 // VirtualFidoDevice ----------------------------------------------------------
 
+VirtualFidoDevice::VirtualFidoDevice() = default;
+
 VirtualFidoDevice::VirtualFidoDevice(scoped_refptr<State> state)
     : state_(std::move(state)) {}
 
@@ -251,12 +252,16 @@ void VirtualFidoDevice::TryWink(base::OnceClosure cb) {
 }
 
 std::string VirtualFidoDevice::GetId() const {
-  // Use our heap address to get a unique-ish number. (0xffe1 is a prime).
-  return "VirtualFidoDevice-" + std::to_string((size_t)this % 0xffe1);
+  return id_;
 }
 
 FidoTransportProtocol VirtualFidoDevice::DeviceTransport() const {
   return state_->transport;
 }
 
+// static
+std::string VirtualFidoDevice::MakeVirtualFidoDeviceId() {
+  return "VirtualFidoDevice-" + base::RandBytesAsString(32);
+}
+
 }  // namespace device
diff --git a/chromium/device/fido/virtual_fido_device.h b/chromium/device/fido/virtual_fido_device.h
index 563f6129ef6..ca95144eafb 100644
--- a/chromium/device/fido/virtual_fido_device.h
+++ b/chromium/device/fido/virtual_fido_device.h
@@ -231,6 +231,9 @@ class COMPONENT_EXPORT(DEVICE_FIDO) VirtualFidoDevice : public FidoDevice {
   FidoTransportProtocol DeviceTransport() const override;
 
  private:
+  static std::string MakeVirtualFidoDeviceId();
+
+  const std::string id_ = MakeVirtualFidoDeviceId();
   scoped_refptr<State> state_ = base::MakeRefCounted<State>();
 
   DISALLOW_COPY_AND_ASSIGN(VirtualFidoDevice);