fido: add the unhashed AppID to CtapGetAssertionRequest

This changes CtapGetAssertionRequest to carry the unhashed AppID rather only
its hashed form, the "Alternative Application Parameter". The unhashed value
is passed to the Windows WebAuthn API for U2F sign requests.

Bug: 898718
Change-Id: I2d9e6b80463859adba9a786e0c478c9d63e4164b
Reviewed-on: https://chromium-review.googlesource.com/c/1354689
Commit-Queue: Martin Kreichgauer <martinkr@chromium.org>
Reviewed-by: Adam Langley <agl@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612349}

content/browser/webauth/authenticator_impl.cc

@@ -263,8 +263,7 @@ device::CtapMakeCredentialRequest CreateCtapMakeCredentialRequest(
 device::CtapGetAssertionRequest CreateCtapGetAssertionRequest(
     const std::string& client_data_json,
     const blink::mojom::PublicKeyCredentialRequestOptionsPtr& options,
-    base::Optional<base::span<const uint8_t, device::kRpIdHashLength>>
-        alternative_application_parameter,
+    base::Optional<std::string> app_id,
     bool is_incognito) {
   device::CtapGetAssertionRequest request_parameter(options->relying_party_id,
                                                     client_data_json);
@@ -278,9 +277,8 @@ device::CtapGetAssertionRequest CreateCtapGetAssertionRequest(
       mojo::ConvertTo<device::UserVerificationRequirement>(
           options->user_verification));
 
-  if (alternative_application_parameter) {
-    request_parameter.SetAlternativeApplicationParameter(
-        std::move(*alternative_application_parameter));
+  if (app_id) {
+    request_parameter.SetAppId(std::move(*app_id));
   }
 
   if (!options->cable_authentication_data.empty()) {
@@ -303,8 +301,9 @@ std::array<uint8_t, crypto::kSHA256Length> CreateApplicationParameter(
   return application_parameter;
 }
 
-base::Optional<std::array<uint8_t, crypto::kSHA256Length>>
-ProcessAppIdExtension(std::string appid, const url::Origin& caller_origin) {
+base::Optional<std::string> ProcessAppIdExtension(
+    std::string appid,
+    const url::Origin& caller_origin) {
   if (appid.empty()) {
     // See step two in the comments in |IsAppIdAllowedForOrigin|.
     appid = caller_origin.Serialize() + "/";
@@ -316,7 +315,7 @@ ProcessAppIdExtension(std::string appid, const url::Origin& caller_origin) {
     return base::nullopt;
   }
 
-  return CreateApplicationParameter(appid);
+  return appid;
 }
 
 // Parses the FIDO transport types extension from the DER-encoded, X.509
@@ -767,9 +766,8 @@ void AuthenticatorImpl::GetAssertion(
   }
 
   if (options->appid) {
-    alternative_application_parameter_ =
-        ProcessAppIdExtension(*options->appid, caller_origin_);
-    if (!alternative_application_parameter_) {
+    app_id_ = ProcessAppIdExtension(*options->appid, caller_origin_);
+    if (!app_id_) {
       std::move(callback).Run(blink::mojom::AuthenticatorStatus::INVALID_DOMAIN,
                               nullptr);
       return;
@@ -787,7 +785,7 @@ void AuthenticatorImpl::GetAssertion(
     connector_ = ServiceManagerConnection::GetForProcess()->GetConnector();
 
   auto ctap_request = CreateCtapGetAssertionRequest(
-      client_data_json_, std::move(options), alternative_application_parameter_,
+      client_data_json_, std::move(options), app_id_,
       browser_context()->IsOffTheRecord());
   auto opt_platform_authenticator_info =
       CreatePlatformAuthenticatorIfAvailableAndCheckIfCredentialExists(
@@ -1072,9 +1070,9 @@ void AuthenticatorImpl::OnSignResponse(
       }
 
       base::Optional<bool> echo_appid_extension;
-      if (alternative_application_parameter_) {
+      if (app_id_) {
         echo_appid_extension = (response_data->GetRpIdHash() ==
-                                *alternative_application_parameter_);
+                                CreateApplicationParameter(*app_id_));
       }
       InvokeCallbackAndCleanup(
           std::move(get_assertion_response_callback_),
@@ -1162,7 +1160,7 @@ void AuthenticatorImpl::Cleanup() {
   make_credential_response_callback_.Reset();
   get_assertion_response_callback_.Reset();
   client_data_json_.clear();
-  alternative_application_parameter_.reset();
+  app_id_.reset();
 }
 
 BrowserContext* AuthenticatorImpl::browser_context() const {

content/browser/webauth/authenticator_impl.h

@@ -108,7 +108,8 @@ class CONTENT_EXPORT AuthenticatorImpl : public blink::mojom::Authenticator,
   // requests coming from the CryptoToken U2F extension, modifies the object key
   // 'type' as required[2].
   // [1] https://w3c.github.io/webauthn/#dictdef-collectedclientdata
-  // [2] https://fidoalliance.org/specs/fido-u2f-v1.2-ps-20170411/fido-u2f-raw-message-formats-v1.2-ps-20170411.html#client-data
+  // [2]
+  // https://fidoalliance.org/specs/fido-u2f-v1.2-ps-20170411/fido-u2f-raw-message-formats-v1.2-ps-20170411.html#client-data
   static std::string SerializeCollectedClientDataToJson(
       const std::string& type,
       const std::string& origin,
@@ -186,11 +187,7 @@ class CONTENT_EXPORT AuthenticatorImpl : public blink::mojom::Authenticator,
   url::Origin caller_origin_;
   std::string relying_party_id_;
   std::unique_ptr<base::OneShotTimer> timer_;
-  // If the "appid" extension is in use then this is the SHA-256 hash of a U2F
-  // AppID. This is used to detect when an assertion request was successfully
-  // retried with this value.
-  base::Optional<std::array<uint8_t, crypto::kSHA256Length>>
-      alternative_application_parameter_;
+  base::Optional<std::string> app_id_;
   // awaiting_attestation_response_ is true if the embedder has been queried
   // about an attestsation decision and the response is still pending.
   bool awaiting_attestation_response_ = false;

device/fido/ctap_get_assertion_request.cc

@@ -120,20 +120,20 @@ CtapGetAssertionRequest& CtapGetAssertionRequest::SetCableExtension(
   return *this;
 }
 
-CtapGetAssertionRequest&
-CtapGetAssertionRequest::SetAlternativeApplicationParameter(
-    base::span<const uint8_t, kRpIdHashLength>
-        alternative_application_parameter) {
+CtapGetAssertionRequest& CtapGetAssertionRequest::SetAppId(std::string app_id) {
+  app_id_ = std::move(app_id);
   alternative_application_parameter_ =
-      fido_parsing_utils::Materialize(alternative_application_parameter);
+      std::array<uint8_t, crypto::kSHA256Length>();
+  crypto::SHA256HashString(*app_id_, alternative_application_parameter_->data(),
+                           alternative_application_parameter_->size());
   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_);
+         (app_id_ &&
+          response_rp_id_hash == *alternative_application_parameter());
 }
 
 }  // namespace device

device/fido/ctap_get_assertion_request.h

@@ -15,6 +15,7 @@
 #include "base/containers/span.h"
 #include "base/macros.h"
 #include "base/optional.h"
+#include "crypto/sha2.h"
 #include "device/fido/cable/cable_discovery_data.h"
 #include "device/fido/fido_constants.h"
 #include "device/fido/public_key_credential_descriptor.h"
@@ -49,9 +50,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) CtapGetAssertionRequest {
   CtapGetAssertionRequest& SetPinProtocol(uint8_t pin_protocol);
   CtapGetAssertionRequest& SetCableExtension(
       std::vector<CableDiscoveryData> cable_extension);
-  CtapGetAssertionRequest& SetAlternativeApplicationParameter(
-      base::span<const uint8_t, kRpIdHashLength>
-          alternative_application_parameter);
+  CtapGetAssertionRequest& SetAppId(std::string app_id);
 
   // Return true if the given RP ID hash from a response is valid for this
   // request.
@@ -87,6 +86,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) CtapGetAssertionRequest {
   alternative_application_parameter() const {
     return alternative_application_parameter_;
   }
+  const base::Optional<std::string>& app_id() const { return app_id_; }
 
   bool is_incognito_mode() const { return is_incognito_mode_; }
   void set_is_incognito_mode(bool is_incognito_mode) {
@@ -105,8 +105,10 @@ class COMPONENT_EXPORT(DEVICE_FIDO) CtapGetAssertionRequest {
   base::Optional<std::vector<uint8_t>> pin_auth_;
   base::Optional<uint8_t> pin_protocol_;
   base::Optional<std::vector<CableDiscoveryData>> cable_extension_;
-  base::Optional<std::array<uint8_t, kRpIdHashLength>>
+  base::Optional<std::string> app_id_;
+  base::Optional<std::array<uint8_t, crypto::kSHA256Length>>
       alternative_application_parameter_;
+
   bool is_incognito_mode_ = false;
 };
 

device/fido/fido_test_data.h

@@ -25,17 +25,18 @@ constexpr uint8_t kChallengeParameter[] = {
     0x44, 0x3,  0xa2, 0xe,  0x4e, 0x13, 0xe3, 0xd5, 0x3e, 0x50,
 };
 
+// SHA256(kRelyingPartyId)
 constexpr uint8_t kApplicationParameter[] = {
     0x11, 0x94, 0x22, 0x8D, 0xA8, 0xFD, 0xBD, 0xEE, 0xFD, 0x26, 0x1B,
     0xD7, 0xB6, 0x59, 0x5C, 0xFD, 0x70, 0xA5, 0x0D, 0x70, 0xC6, 0x40,
     0x7B, 0xCF, 0x01, 0x3D, 0xE9, 0x6D, 0x4E, 0xFB, 0x17, 0xDE,
 };
 
+// SHA256(kAppId)
 constexpr uint8_t kAlternativeApplicationParameter[] = {
-    0x01, 0x02, 0x03, 0x04, 0x01, 0x02, 0x03, 0x04, 0x01, 0x02, 0x03,
-    0x04, 0x01, 0x02, 0x03, 0x04, 0x01, 0x02, 0x03, 0x04, 0x01, 0x02,
-    0x03, 0x04, 0x01, 0x02, 0x03, 0x04, 0x01, 0x02, 0x03, 0x04,
-};
+    0x91, 0x14, 0xf2, 0xc9, 0xf2, 0x0b, 0x30, 0x7b, 0x49, 0xac, 0x96,
+    0x2c, 0xf7, 0x6e, 0xa2, 0x08, 0x3c, 0xa7, 0xa7, 0x8d, 0xe1, 0xcd,
+    0x4e, 0x82, 0x5e, 0xca, 0x3a, 0x98, 0x0f, 0x1a, 0x25, 0x6d};
 
 constexpr char kClientDataJson[] =
     R"({"challenge":"foobar","new_keys_may_be_added_here":"do not compare clientDataJSON against a template. See https://goo.gl/yabPex","origin":"https://google.com","type":"webauthn.create"})";
@@ -50,6 +51,7 @@ constexpr uint8_t kClientDataHash[] = {
 constexpr uint8_t kUserId[] = {0x10, 0x98, 0x23, 0x72, 0x35, 0x40, 0x98, 0x72};
 
 constexpr char kRelyingPartyId[] = "acme.com";
+constexpr char kAppId[] = "acme.com/";
 
 constexpr uint8_t kU2fRegisterCommandApduWithIndividualAttestation[] = {
     // CLA, INS, P1, P2 APDU instructions
@@ -299,9 +301,9 @@ constexpr uint8_t kU2fSignCommandApduWithAlternativeApplicationParameter[] = {
     0xae, 0xc8, 0x33, 0xac, 0xbf, 0xd2, 0x85, 0xa5, 0xdf, 0x44, 0x3, 0xa2, 0xe,
     0x4e, 0x13, 0xe3, 0xd5, 0x3e, 0x50,
     // Alternative application parameter
-    0x01, 0x02, 0x03, 0x04, 0x01, 0x02, 0x03, 0x04, 0x01, 0x02, 0x03, 0x04,
-    0x01, 0x02, 0x03, 0x04, 0x01, 0x02, 0x03, 0x04, 0x01, 0x02, 0x03, 0x04,
-    0x01, 0x02, 0x03, 0x04, 0x01, 0x02, 0x03, 0x04,
+    0x91, 0x14, 0xf2, 0xc9, 0xf2, 0x0b, 0x30, 0x7b, 0x49, 0xac, 0x96, 0x2c,
+    0xf7, 0x6e, 0xa2, 0x08, 0x3c, 0xa7, 0xa7, 0x8d, 0xe1, 0xcd, 0x4e, 0x82,
+    0x5e, 0xca, 0x3a, 0x98, 0x0f, 0x1a, 0x25, 0x6d,
     // Key handle length
     0x40,
     // Key handle
@@ -326,9 +328,9 @@ constexpr uint8_t
         0xfa, 0xae, 0xc8, 0x33, 0xac, 0xbf, 0xd2, 0x85, 0xa5, 0xdf, 0x44, 0x3,
         0xa2, 0xe, 0x4e, 0x13, 0xe3, 0xd5, 0x3e, 0x50,
         // Alternative application parameter
-        0x01, 0x02, 0x03, 0x04, 0x01, 0x02, 0x03, 0x04, 0x01, 0x02, 0x03, 0x04,
-        0x01, 0x02, 0x03, 0x04, 0x01, 0x02, 0x03, 0x04, 0x01, 0x02, 0x03, 0x04,
-        0x01, 0x02, 0x03, 0x04, 0x01, 0x02, 0x03, 0x04,
+        0x91, 0x14, 0xf2, 0xc9, 0xf2, 0x0b, 0x30, 0x7b, 0x49, 0xac, 0x96, 0x2c,
+        0xf7, 0x6e, 0xa2, 0x08, 0x3c, 0xa7, 0xa7, 0x8d, 0xe1, 0xcd, 0x4e, 0x82,
+        0x5e, 0xca, 0x3a, 0x98, 0x0f, 0x1a, 0x25, 0x6d,
         // Key handle length
         0x40,
         // Key handle

device/fido/get_assertion_task_unittest.cc

@@ -224,8 +224,7 @@ TEST_F(FidoGetAssertionTaskTest, TestSilentSignInWhenAppIdExtensionPresent) {
   allowed_list.push_back(PublicKeyCredentialDescriptor(
       CredentialType::kPublicKey,
       fido_parsing_utils::Materialize(test_data::kU2fSignKeyHandle)));
-  request.SetAlternativeApplicationParameter(
-      test_data::kAlternativeApplicationParameter);
+  request.SetAppId(test_data::kAppId);
   request.SetAllowList(std::move(allowed_list));
 
   auto device = MockFidoDevice::MakeCtap();
@@ -251,8 +250,7 @@ TEST_F(FidoGetAssertionTaskTest, TestU2fFallbackForAppIdExtension) {
   allowed_list.push_back(PublicKeyCredentialDescriptor(
       CredentialType::kPublicKey,
       fido_parsing_utils::Materialize(test_data::kU2fSignKeyHandle)));
-  request.SetAlternativeApplicationParameter(
-      test_data::kAlternativeApplicationParameter);
+  request.SetAppId(test_data::kAppId);
   request.SetAllowList(std::move(allowed_list));
 
   ::testing::InSequence s;
@@ -294,8 +292,7 @@ TEST_F(FidoGetAssertionTaskTest, TestAvoidSilentSignInForCtapOnlyDevice) {
       CredentialType::kPublicKey,
       fido_parsing_utils::Materialize(test_data::kU2fSignKeyHandle)));
 
-  request.SetAlternativeApplicationParameter(
-      test_data::kAlternativeApplicationParameter);
+  request.SetAppId(test_data::kAppId);
   request.SetAllowList(std::move(allowed_list));
 
   auto device = MockFidoDevice::MakeCtap(ReadCTAPGetInfoResponse(

device/fido/u2f_sign_operation_unittest.cc

@@ -347,8 +347,7 @@ TEST_F(U2fSignOperationTest, SignWithCorruptedResponse) {
 TEST_F(U2fSignOperationTest, AlternativeApplicationParameter) {
   auto request = CreateSignRequest(
       {fido_parsing_utils::Materialize(test_data::kU2fSignKeyHandle)});
-  request.SetAlternativeApplicationParameter(
-      test_data::kAlternativeApplicationParameter);
+  request.SetAppId(test_data::kAppId);
 
   auto device = std::make_unique<MockFidoDevice>();
   EXPECT_CALL(*device, GetId()).WillRepeatedly(::testing::Return("device"));
@@ -387,8 +386,7 @@ TEST_F(U2fSignOperationTest, AlternativeApplicationParameter) {
 TEST_F(U2fSignOperationTest, AlternativeApplicationParameterRejection) {
   auto request = CreateSignRequest(
       {fido_parsing_utils::Materialize(test_data::kU2fSignKeyHandle)});
-  request.SetAlternativeApplicationParameter(
-      test_data::kAlternativeApplicationParameter);
+  request.SetAppId(test_data::kAppId);
 
   auto device = std::make_unique<MockFidoDevice>();
   EXPECT_CALL(*device, GetId()).WillRepeatedly(::testing::Return("device"));

device/fido/win/authenticator.cc

@@ -261,13 +261,10 @@ void WinWebAuthnApiAuthenticator::GetAssertion(CtapGetAssertionRequest request,
   get_assertion_data_ = GetAssertionData{};
   get_assertion_data_->rp_id16 = base::UTF8ToUTF16(request.rp_id());
   base::Optional<base::string16> opt_app_id16 = base::nullopt;
-  // TODO(martinkr): alternative_application_parameter() is already hashed,
-  // so this doesn't work. We need to make it store the full AppID.
-  if (request.alternative_application_parameter()) {
+  if (request.app_id()) {
     get_assertion_data_->opt_app_id16 = base::UTF8ToUTF16(base::StringPiece(
-        reinterpret_cast<const char*>(
-            request.alternative_application_parameter()->data()),
-        request.alternative_application_parameter()->size()));
+        reinterpret_cast<const char*>(request.app_id()->data()),
+        request.app_id()->size()));
   }
 
   get_assertion_data_->request_client_data = request.client_data_json();