webauthn: fix appid extension.

It broke because of the test that the response RP ID hash was actually
the hash of the RP ID. (Which isn't true when using an AppID hash.)

Obviously we were missing test coverage here, so add that now that it's
easy to do with VirtualFidoDevice. (Confirmed that the test failed
before the rest of these changes and passes now.)

Change-Id: Ibd3b39339500e24f00c4f6c12ab4405ae0596107
Reviewed-on: https://chromium-review.googlesource.com/1159522Reviewed-by: Jun Choi <hongjunchoi@chromium.org>
Commit-Queue: Adam Langley <agl@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580197}

content/browser/webauth/authenticator_impl_unittest.cc

@@ -682,7 +682,7 @@ constexpr OriginClaimedAuthorityPair kValidAppIdCases[] = {
 };
 
 // Verify behavior for various combinations of origins and RP IDs.
-TEST_F(AuthenticatorImplTest, AppIdExtension) {
+TEST_F(AuthenticatorImplTest, AppIdExtensionValues) {
   TestServiceManagerContext smc;
   device::test::ScopedVirtualFidoDevice virtual_device;
 
@@ -711,6 +711,31 @@ TEST_F(AuthenticatorImplTest, AppIdExtension) {
   }
 }
 
+// Verify that a credential registered with U2F can be used via webauthn.
+TEST_F(AuthenticatorImplTest, AppIdExtension) {
+  SimulateNavigation(GURL(kTestOrigin1));
+  PublicKeyCredentialRequestOptionsPtr options =
+      GetTestPublicKeyCredentialRequestOptions();
+  TestGetAssertionCallback callback_receiver;
+  auto task_runner = base::MakeRefCounted<base::TestMockTimeTaskRunner>(
+      base::Time::Now(), base::TimeTicks::Now());
+  auto authenticator = ConstructAuthenticatorWithTimer(task_runner);
+  device::test::ScopedVirtualFidoDevice virtual_device;
+
+  // Inject a registration for the URL (which is a U2F AppID).
+  ASSERT_TRUE(virtual_device.mutable_state()->InjectRegistration(
+      options->allow_credentials[0]->id, kTestOrigin1));
+
+  // Set the same URL as the appid parameter.
+  options->appid = kTestOrigin1;
+
+  authenticator->GetAssertion(std::move(options), callback_receiver.callback());
+
+  // Trigger timer.
+  callback_receiver.WaitForCallback();
+  EXPECT_EQ(AuthenticatorStatus::SUCCESS, callback_receiver.status());
+}
+
 TEST_F(AuthenticatorImplTest, TestGetAssertionTimeout) {
   SimulateNavigation(GURL(kTestOrigin1));
   PublicKeyCredentialRequestOptionsPtr options =

device/fido/ctap_get_assertion_request.cc

@@ -160,6 +160,13 @@ CtapGetAssertionRequest::SetAlternativeApplicationParameter(
   return *this;
 }
 
+bool CtapGetAssertionRequest::CheckResponseRpIdHash(
+    const std::array<uint8_t, kRpIdHashLength>& response_rp_id_hash) {
+  return response_rp_id_hash == fido_parsing_utils::CreateSHA256Hash(rp_id_) ||
+         (alternative_application_parameter_ &&
+          response_rp_id_hash == *alternative_application_parameter_);
+}
+
 base::Optional<CtapGetAssertionRequest> ParseCtapGetAssertionRequest(
     base::span<const uint8_t> request_bytes) {
   const auto& cbor_request = cbor::CBORReader::Read(request_bytes);

device/fido/ctap_get_assertion_request.h

@@ -52,6 +52,11 @@ class COMPONENT_EXPORT(DEVICE_FIDO) CtapGetAssertionRequest {
       base::span<const uint8_t, kRpIdHashLength>
           alternative_application_parameter);
 
+  // Return true if the given RP ID hash from a response is valid for this
+  // request.
+  bool CheckResponseRpIdHash(
+      const std::array<uint8_t, kRpIdHashLength>& response_rp_id_hash);
+
   const std::string& rp_id() const { return rp_id_; }
   const std::array<uint8_t, kClientDataHashLength>& client_data_hash() const {
     return client_data_hash_;

device/fido/get_assertion_request_handler.cc

@@ -178,7 +178,7 @@ void GetAssertionRequestHandler::HandleResponse(
     return;
   }
 
-  if (!response || !response->CheckRpIdHash(request_.rp_id()) ||
+  if (!response || !request_.CheckResponseRpIdHash(response->GetRpIdHash()) ||
       !CheckResponseCredentialIdMatchesRequestAllowList(*authenticator,
                                                         request_, *response) ||
       !CheckRequirementsOnResponseUserEntity(request_, *response)) {

device/fido/make_credential_request_handler.cc

@@ -9,6 +9,7 @@
 #include "base/bind.h"
 #include "device/fido/authenticator_make_credential_response.h"
 #include "device/fido/fido_authenticator.h"
+#include "device/fido/fido_parsing_utils.h"
 #include "device/fido/make_credential_task.h"
 #include "services/service_manager/public/cpp/connector.h"
 
@@ -112,7 +113,10 @@ void MakeCredentialRequestHandler::HandleResponse(
     return;
   }
 
-  if (!response || !response->CheckRpIdHash(request_parameter_.rp().rp_id())) {
+  const auto rp_id_hash =
+      fido_parsing_utils::CreateSHA256Hash(request_parameter_.rp().rp_id());
+
+  if (!response || response->GetRpIdHash() != rp_id_hash) {
     OnAuthenticatorResponse(
         authenticator, CtapDeviceResponseCode::kCtap2ErrOther, base::nullopt);
     return;

device/fido/response_data.cc

@@ -32,9 +32,4 @@ std::string ResponseData::GetId() const {
   return id;
 }
 
-bool ResponseData::CheckRpIdHash(const std::string& rp_id) const {
-  return base::make_span(GetRpIdHash()) ==
-         base::make_span(fido_parsing_utils::CreateSHA256Hash(rp_id));
-}
-
 }  // namespace device

device/fido/response_data.h

@@ -28,11 +28,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) ResponseData {
 
   std::string GetId() const;
 
-  // Checks that the SHA256 hash of the relying party id obtained from the
-  // request parameter matches the application parameter returned from the
-  // authenticator.
-  bool CheckRpIdHash(const std::string& rp_id) const;
-
   const std::vector<uint8_t>& raw_credential_id() const {
     return raw_credential_id_;
   }