webauthn: only echo appid extension when requested.

This change aligns Chromium with the current state of the spec[1]: the
“appid” extension output will only be returned when the caller requested
the appid extension, and it'll be true iff the AppID value was actually
used when getting the assertion.

[1] https://w3c.github.io/webauthn/#sctn-appid-extension

BUG=853770

Change-Id: I6ac9470c71bfc0cc3b38f6e91c5e8f00b0586405
Reviewed-on: https://chromium-review.googlesource.com/1162823Reviewed-by: Balazs Engedy <engedy@chromium.org>
Reviewed-by: Jun Choi <hongjunchoi@chromium.org>
Reviewed-by: Greg Kerr <kerrnel@chromium.org>
Commit-Queue: Adam Langley <agl@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582231}

content/browser/webauth/authenticator_impl.cc

@@ -281,7 +281,7 @@ CreateMakeCredentialResponse(
 blink::mojom::GetAssertionAuthenticatorResponsePtr CreateGetAssertionResponse(
     const std::string& client_data_json,
     device::AuthenticatorGetAssertionResponse response_data,
-    bool echo_appid_extension) {
+    base::Optional<bool> echo_appid_extension) {
   auto response = blink::mojom::GetAssertionAuthenticatorResponse::New();
   auto common_info = blink::mojom::CommonCredentialInfo::New();
   common_info->client_data_json.assign(client_data_json.begin(),
@@ -292,7 +292,10 @@ blink::mojom::GetAssertionAuthenticatorResponsePtr CreateGetAssertionResponse(
   response->authenticator_data =
       response_data.auth_data().SerializeToByteArray();
   response->signature = response_data.signature();
-  response->echo_appid_extension = echo_appid_extension;
+  if (echo_appid_extension) {
+    response->echo_appid_extension = true;
+    response->appid_extension = *echo_appid_extension;
+  }
   response_data.user_entity()
       ? response->user_handle.emplace(response_data.user_entity()->user_id())
       : response->user_handle.emplace();
@@ -554,19 +557,14 @@ void AuthenticatorImpl::GetAssertion(
     return;
   }
 
-  base::Optional<std::array<uint8_t, crypto::kSHA256Length>>
-      alternative_application_parameter;
   if (options->appid) {
-    auto appid_hash = ProcessAppIdExtension(*options->appid, caller_origin);
-    if (!appid_hash) {
+    alternative_application_parameter_ =
+        ProcessAppIdExtension(*options->appid, caller_origin);
+    if (!alternative_application_parameter_) {
       std::move(callback).Run(blink::mojom::AuthenticatorStatus::INVALID_DOMAIN,
                               nullptr);
       return;
     }
-
-    alternative_application_parameter = std::move(appid_hash);
-    // TODO(agl): needs a test once a suitable, mock U2F device exists.
-    echo_appid_extension_ = true;
   }
 
   DCHECK(get_assertion_response_callback_.is_null());
@@ -589,9 +587,9 @@ void AuthenticatorImpl::GetAssertion(
 
   request_ = std::make_unique<device::GetAssertionRequestHandler>(
       connector_, protocols_,
-      CreateCtapGetAssertionRequest(
-          ConstructClientDataHash(client_data_json_), std::move(options),
-          std::move(alternative_application_parameter)),
+      CreateCtapGetAssertionRequest(ConstructClientDataHash(client_data_json_),
+                                    std::move(options),
+                                    alternative_application_parameter_),
       base::BindOnce(&AuthenticatorImpl::OnSignResponse,
                      weak_factory_.GetWeakPtr()),
       base::BindOnce(&AuthenticatorImpl::CreatePlatformAuthenticatorIfAvailable,
@@ -774,12 +772,17 @@ void AuthenticatorImpl::OnSignResponse(
       return;
     case device::FidoReturnCode::kSuccess:
       DCHECK(response_data.has_value());
+      base::Optional<bool> echo_appid_extension;
+      if (alternative_application_parameter_) {
+        echo_appid_extension = (response_data->GetRpIdHash() ==
+                                *alternative_application_parameter_);
+      }
       InvokeCallbackAndCleanup(
           std::move(get_assertion_response_callback_),
           blink::mojom::AuthenticatorStatus::SUCCESS,
           CreateGetAssertionResponse(std::move(client_data_json_),
                                      std::move(*response_data),
-                                     echo_appid_extension_));
+                                     echo_appid_extension));
       return;
   }
   NOTREACHED();
@@ -832,7 +835,7 @@ void AuthenticatorImpl::Cleanup() {
   make_credential_response_callback_.Reset();
   get_assertion_response_callback_.Reset();
   client_data_json_.clear();
-  echo_appid_extension_ = false;
+  alternative_application_parameter_.reset();
 }
 
 BrowserContext* AuthenticatorImpl::browser_context() const {

content/browser/webauth/authenticator_impl.h

@@ -16,6 +16,7 @@
 #include "base/optional.h"
 #include "content/common/content_export.h"
 #include "content/public/browser/web_contents_observer.h"
+#include "crypto/sha2.h"
 #include "device/fido/authenticator_get_assertion_response.h"
 #include "device/fido/authenticator_make_credential_response.h"
 #include "device/fido/fido_constants.h"
@@ -168,12 +169,11 @@ class CONTENT_EXPORT AuthenticatorImpl : public blink::mojom::Authenticator,
   blink::mojom::AttestationConveyancePreference attestation_preference_;
   std::string relying_party_id_;
   std::unique_ptr<base::OneShotTimer> timer_;
-
-  // Whether or not a GetAssertion call should return a PublicKeyCredential
-  // instance whose getClientExtensionResults() method yields a
-  // AuthenticationExtensions dictionary that contains the `appid: true`
-  // extension output.
-  bool echo_appid_extension_ = false;
+  // 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_;
 
   // Owns pipes to this Authenticator from |render_frame_host_|.
   mojo::Binding<blink::mojom::Authenticator> binding_;

content/browser/webauth/authenticator_impl_unittest.cc

@@ -714,26 +714,71 @@ TEST_F(AuthenticatorImplTest, AppIdExtensionValues) {
 // 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));
+  {
+    // First, test that the appid extension isn't echoed at all when not
+    // requested.
+    PublicKeyCredentialRequestOptionsPtr options =
+        GetTestPublicKeyCredentialRequestOptions();
+    device::test::ScopedVirtualFidoDevice virtual_device;
+    ASSERT_TRUE(virtual_device.mutable_state()->InjectRegistration(
+        options->allow_credentials[0]->id, kTestRelyingPartyId));
 
-  // Set the same URL as the appid parameter.
-  options->appid = kTestOrigin1;
+    TestGetAssertionCallback callback_receiver;
+    authenticator->GetAssertion(std::move(options),
+                                callback_receiver.callback());
+    callback_receiver.WaitForCallback();
+    ASSERT_EQ(AuthenticatorStatus::SUCCESS, callback_receiver.status());
 
-  authenticator->GetAssertion(std::move(options), callback_receiver.callback());
+    EXPECT_EQ(false, callback_receiver.value()->echo_appid_extension);
+  }
 
-  // Trigger timer.
-  callback_receiver.WaitForCallback();
-  EXPECT_EQ(AuthenticatorStatus::SUCCESS, callback_receiver.status());
+  {
+    // Second, test that the appid extension is echoed, but is false, when appid
+    // is requested but not used.
+    PublicKeyCredentialRequestOptionsPtr options =
+        GetTestPublicKeyCredentialRequestOptions();
+    device::test::ScopedVirtualFidoDevice virtual_device;
+    ASSERT_TRUE(virtual_device.mutable_state()->InjectRegistration(
+        options->allow_credentials[0]->id, kTestRelyingPartyId));
+
+    // This AppID won't be used because the RP ID will be tried (successfully)
+    // first.
+    options->appid = kTestOrigin1;
+
+    TestGetAssertionCallback callback_receiver;
+    authenticator->GetAssertion(std::move(options),
+                                callback_receiver.callback());
+    callback_receiver.WaitForCallback();
+    ASSERT_EQ(AuthenticatorStatus::SUCCESS, callback_receiver.status());
+
+    EXPECT_EQ(true, callback_receiver.value()->echo_appid_extension);
+    EXPECT_EQ(false, callback_receiver.value()->appid_extension);
+  }
+
+  {
+    // Lastly, when used, the appid extension result should be "true".
+    PublicKeyCredentialRequestOptionsPtr options =
+        GetTestPublicKeyCredentialRequestOptions();
+    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));
+
+    options->appid = kTestOrigin1;
+
+    TestGetAssertionCallback callback_receiver;
+    authenticator->GetAssertion(std::move(options),
+                                callback_receiver.callback());
+    callback_receiver.WaitForCallback();
+    ASSERT_EQ(AuthenticatorStatus::SUCCESS, callback_receiver.status());
+
+    EXPECT_EQ(true, callback_receiver.value()->echo_appid_extension);
+    EXPECT_EQ(true, callback_receiver.value()->appid_extension);
+  }
 }
 
 TEST_F(AuthenticatorImplTest, TestGetAssertionTimeout) {

third_party/blink/public/platform/modules/webauthn/authenticator.mojom

@@ -65,11 +65,10 @@ struct GetAssertionAuthenticatorResponse {
   array<uint8>? user_handle;
 
   // True if getClientExtensionResults() called on the returned
-  // PublicKeyCredential instance should yield an
-  // AuthenticationExtensionsClientOutputs dictionary that contains the `appid:
-  // true` extension output, which indicates to the relying party that the
-  // `appid` extension was acted upon.
+  // PublicKeyCredential instance should contain an `appid` extension output.
+  // If so, |appid_extension| contains the actual value.
   bool echo_appid_extension;
+  bool appid_extension;
 };
 
 // Information about the relying party. These fields take arbitrary input.

third_party/blink/renderer/modules/credentialmanager/credentials_container.cc

@@ -378,7 +378,9 @@ void OnGetAssertionComplete(
                                                authenticator_buffer,
                                                signature_buffer, user_handle);
     AuthenticationExtensionsClientOutputs extension_outputs;
-    extension_outputs.setAppid(credential->echo_appid_extension);
+    if (credential->echo_appid_extension) {
+      extension_outputs.setAppid(credential->appid_extension);
+    }
     resolver->Resolve(PublicKeyCredential::Create(credential->info->id, raw_id,
                                                   authenticator_response,
                                                   extension_outputs));