webauthn: don't send hmac-secret when not supported.

Previously we would send the hmac-secret extension to authenticators
that didn't advertise support for it. That's fine because it'll be
ignored but, in the future when we have more support for hmac-secret, it
gets expensive.

This change adds the basic infrastructure for that and stops sending the
extension unless the authenticator advertises support.

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

content/browser/webauth/authenticator_impl_unittest.cc

@@ -2939,55 +2939,59 @@ TEST_F(AuthenticatorImplTest, ExtensionHMACSecret) {
   NavigateAndCommit(GURL(kTestOrigin1));
 
   for (const bool include_extension : {false, true}) {
-    SCOPED_TRACE(include_extension);
+    for (const bool authenticator_support : {false, true}) {
+      SCOPED_TRACE(include_extension);
+      SCOPED_TRACE(authenticator_support);
 
-    virtual_device_factory_->SetSupportedProtocol(
-        device::ProtocolVersion::kCtap2);
+      device::VirtualCtap2Device::Config config;
+      config.hmac_secret_support = authenticator_support;
+      virtual_device_factory_->SetCtap2Config(config);
 
-    mojo::Remote<blink::mojom::Authenticator> authenticator =
-        ConnectToAuthenticator();
-    PublicKeyCredentialCreationOptionsPtr options =
-        GetTestPublicKeyCredentialCreationOptions();
-    options->hmac_create_secret = include_extension;
-    TestMakeCredentialCallback callback_receiver;
-    authenticator->MakeCredential(std::move(options),
-                                  callback_receiver.callback());
-    callback_receiver.WaitForCallback();
-    EXPECT_EQ(AuthenticatorStatus::SUCCESS, callback_receiver.status());
+      mojo::Remote<blink::mojom::Authenticator> authenticator =
+          ConnectToAuthenticator();
+      PublicKeyCredentialCreationOptionsPtr options =
+          GetTestPublicKeyCredentialCreationOptions();
+      options->hmac_create_secret = include_extension;
+      TestMakeCredentialCallback callback_receiver;
+      authenticator->MakeCredential(std::move(options),
+                                    callback_receiver.callback());
+      callback_receiver.WaitForCallback();
+      EXPECT_EQ(AuthenticatorStatus::SUCCESS, callback_receiver.status());
 
-    base::Optional<Value> attestation_value =
-        Reader::Read(callback_receiver.value()->attestation_object);
-    ASSERT_TRUE(attestation_value);
-    ASSERT_TRUE(attestation_value->is_map());
-    const auto& attestation = attestation_value->GetMap();
+      base::Optional<Value> attestation_value =
+          Reader::Read(callback_receiver.value()->attestation_object);
+      ASSERT_TRUE(attestation_value);
+      ASSERT_TRUE(attestation_value->is_map());
+      const auto& attestation = attestation_value->GetMap();
 
-    const auto auth_data_it = attestation.find(Value(device::kAuthDataKey));
-    ASSERT_TRUE(auth_data_it != attestation.end());
-    ASSERT_TRUE(auth_data_it->second.is_bytestring());
-    const std::vector<uint8_t>& auth_data =
-        auth_data_it->second.GetBytestring();
-    base::Optional<device::AuthenticatorData> parsed_auth_data =
-        device::AuthenticatorData::DecodeAuthenticatorData(auth_data);
-
-    // The virtual CTAP2 device always echos the hmac-secret extension on
-    // registrations. Therefore, if |hmac_secret| was set above it should be
-    // serialised in the CBOR and correctly passed all the way back around to
-    // the reply's authenticator data.
-    bool has_hmac_secret = false;
-    const auto& extensions = parsed_auth_data->extensions();
-    if (extensions) {
-      CHECK(extensions->is_map());
-      const cbor::Value::MapValue& extensions_map = extensions->GetMap();
-      const auto hmac_secret_it =
-          extensions_map.find(cbor::Value(device::kExtensionHmacSecret));
-      if (hmac_secret_it != extensions_map.end()) {
-        ASSERT_TRUE(hmac_secret_it->second.is_bool());
-        EXPECT_TRUE(hmac_secret_it->second.GetBool());
-        has_hmac_secret = true;
+      const auto auth_data_it = attestation.find(Value(device::kAuthDataKey));
+      ASSERT_TRUE(auth_data_it != attestation.end());
+      ASSERT_TRUE(auth_data_it->second.is_bytestring());
+      const std::vector<uint8_t>& auth_data =
+          auth_data_it->second.GetBytestring();
+      base::Optional<device::AuthenticatorData> parsed_auth_data =
+          device::AuthenticatorData::DecodeAuthenticatorData(auth_data);
+
+      // The virtual CTAP2 device always echos the hmac-secret extension on
+      // registrations. Therefore, if |hmac_secret| was set above it should be
+      // serialised in the CBOR and correctly passed all the way back around to
+      // the reply's authenticator data.
+      bool has_hmac_secret = false;
+      const auto& extensions = parsed_auth_data->extensions();
+      if (extensions) {
+        CHECK(extensions->is_map());
+        const cbor::Value::MapValue& extensions_map = extensions->GetMap();
+        const auto hmac_secret_it =
+            extensions_map.find(cbor::Value(device::kExtensionHmacSecret));
+        if (hmac_secret_it != extensions_map.end()) {
+          ASSERT_TRUE(hmac_secret_it->second.is_bool());
+          EXPECT_TRUE(hmac_secret_it->second.GetBool());
+          has_hmac_secret = true;
+        }
       }
-    }
 
-    EXPECT_EQ(include_extension, has_hmac_secret);
+      EXPECT_EQ(include_extension && authenticator_support, has_hmac_secret);
+    }
   }
 }
 

device/fido/fido_authenticator.cc

@@ -141,4 +141,8 @@ bool FidoAuthenticator::SupportsCredProtectExtension() const {
   return Options() && Options()->supports_cred_protect;
 }
 
+bool FidoAuthenticator::SupportsHMACSecretExtension() const {
+  return false;
+}
+
 }  // namespace device

device/fido/fido_authenticator.h

@@ -211,6 +211,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoAuthenticator {
   virtual base::string16 GetDisplayName() const = 0;
   virtual ProtocolVersion SupportedProtocol() const;
   virtual bool SupportsCredProtectExtension() const;
+  virtual bool SupportsHMACSecretExtension() const;
   virtual const base::Optional<AuthenticatorSupportedOptions>& Options()
       const = 0;
   virtual base::Optional<FidoTransportProtocol> AuthenticatorTransport()

device/fido/fido_device_authenticator.cc

@@ -719,6 +719,13 @@ ProtocolVersion FidoDeviceAuthenticator::SupportedProtocol() const {
   return device_->supported_protocol();
 }
 
+bool FidoDeviceAuthenticator::SupportsHMACSecretExtension() const {
+  const base::Optional<AuthenticatorGetInfoResponse>& get_info_response =
+      device_->device_info();
+  return get_info_response && get_info_response->extensions &&
+         base::Contains(*get_info_response->extensions, kExtensionHmacSecret);
+}
+
 const base::Optional<AuthenticatorSupportedOptions>&
 FidoDeviceAuthenticator::Options() const {
   return options_;

device/fido/fido_device_authenticator.h

@@ -97,6 +97,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDeviceAuthenticator
   std::string GetId() const override;
   base::string16 GetDisplayName() const override;
   ProtocolVersion SupportedProtocol() const override;
+  bool SupportsHMACSecretExtension() const override;
   const base::Optional<AuthenticatorSupportedOptions>& Options() const override;
   base::Optional<FidoTransportProtocol> AuthenticatorTransport() const override;
   bool IsInPairingMode() const override;

device/fido/make_credential_request_handler.cc

@@ -944,6 +944,10 @@ void MakeCredentialRequestHandler::SpecializeRequestForAuthenticator(
       authenticator->Options()->supports_android_client_data_ext) {
     request->android_client_data_ext = *options_.android_client_data_ext;
   }
+
+  if (request->hmac_secret && !authenticator->SupportsHMACSecretExtension()) {
+    request->hmac_secret = false;
+  }
 }
 
 }  // namespace device

device/fido/virtual_ctap2_device.cc

@@ -532,14 +532,22 @@ VirtualCtap2Device::VirtualCtap2Device(scoped_refptr<State> state,
     device_info_->options = std::move(options);
   }
 
+  std::vector<std::string> extensions;
+
   if (config.cred_protect_support) {
-    device_info_->extensions.emplace(
-        {std::string(device::kExtensionCredProtect)});
+    extensions.emplace_back(device::kExtensionCredProtect);
+  }
+
+  if (config.hmac_secret_support) {
+    extensions.emplace_back(device::kExtensionHmacSecret);
   }
 
   if (config.support_android_client_data_extension) {
-    device_info_->extensions.emplace(
-        {std::string(device::kExtensionAndroidClientData)});
+    extensions.emplace_back(device::kExtensionAndroidClientData);
+  }
+
+  if (!extensions.empty()) {
+    device_info_->extensions.emplace(std::move(extensions));
   }
 
   if (config.max_credential_count_in_list > 0) {
@@ -893,6 +901,13 @@ base::Optional<CtapDeviceResponseCode> VirtualCtap2Device::OnMakeCredential(
   base::Optional<cbor::Value> extensions;
   cbor::Value::MapValue extensions_map;
   if (request.hmac_secret) {
+    if (!config_.hmac_secret_support) {
+      // Should not have been sent. Authenticators will normally ignore unknown
+      // extensions but Chromium should not make this mistake.
+      DLOG(ERROR)
+          << "Rejecting makeCredential due to unexpected hmac_secret extension";
+      return base::nullopt;
+    }
     extensions_map.emplace(cbor::Value(kExtensionHmacSecret),
                            cbor::Value(true));
   }

device/fido/virtual_ctap2_device.h

@@ -52,6 +52,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) VirtualCtap2Device
     uint8_t bio_enrollment_capacity = 10;
     uint8_t bio_enrollment_samples_required = 4;
     bool cred_protect_support = false;
+    bool hmac_secret_support = false;
 
     // force_cred_protect, if set and if |cred_protect_support| is true, is a
     // credProtect level that will be forced for all registrations. This

device/fido/win/authenticator.cc

@@ -188,6 +188,10 @@ bool WinWebAuthnApiAuthenticator::SupportsCredProtectExtension() const {
   return win_api_->Version() >= WEBAUTHN_API_VERSION_2;
 }
 
+bool WinWebAuthnApiAuthenticator::SupportsHMACSecretExtension() const {
+  return true;
+}
+
 const base::Optional<AuthenticatorSupportedOptions>&
 WinWebAuthnApiAuthenticator::Options() const {
   // The request can potentially be fulfilled by any device that Windows

device/fido/win/authenticator.h

@@ -63,6 +63,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) WinWebAuthnApiAuthenticator
   // SupportsCredProtectExtension returns whether the native API supports the
   // credProtect CTAP extension.
   bool SupportsCredProtectExtension() const override;
+  bool SupportsHMACSecretExtension() const override;
   const base::Optional<AuthenticatorSupportedOptions>& Options() const override;
   base::Optional<FidoTransportProtocol> AuthenticatorTransport() const override;
   bool IsWinNativeApiAuthenticator() const override;