webauthn: check appids before invoking request proxy

Checking the App ID against the caller origin is a security decision and
should therefore occur before deciding whether the request needs to be
handed off to a proxy extension, just as with RP IDs.

Bug: 1231802
Change-Id: I3cf9a392b5cf01b6794d73a742207f6106c456ad
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3615538
Reviewed-by: Adam Langley <agl@chromium.org>
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Cr-Commit-Position: refs/heads/main@{#998475}

content/browser/webauth/authenticator_common.cc

@@ -693,6 +693,18 @@ void AuthenticatorCommon::MakeCredential(
   caller_origin_ = caller_origin;
   relying_party_id_ = options->relying_party.id;
 
+  absl::optional<std::string> appid_exclude;
+  if (options->appid_exclude) {
+    appid_exclude = ProcessAppIdExtension(
+        GetRenderFrameHost()->GetBrowserContext(), *options->appid_exclude,
+        caller_origin, options->remote_desktop_client_override);
+    if (!appid_exclude) {
+      CompleteMakeCredentialRequest(
+          blink::mojom::AuthenticatorStatus::INVALID_DOMAIN);
+      return;
+    }
+  }
+
   // If there is an active webAuthenticationProxy extension, let it handle the
   // request.
   WebAuthenticationRequestProxy* proxy = GetWebAuthnRequestProxyIfActive();
@@ -741,18 +753,6 @@ void AuthenticatorCommon::MakeCredential(
       return;
   }
 
-  absl::optional<std::string> appid_exclude;
-  if (options->appid_exclude) {
-    appid_exclude = ProcessAppIdExtension(
-        GetRenderFrameHost()->GetBrowserContext(), *options->appid_exclude,
-        caller_origin, options->remote_desktop_client_override);
-    if (!appid_exclude) {
-      CompleteMakeCredentialRequest(
-          blink::mojom::AuthenticatorStatus::INVALID_DOMAIN);
-      return;
-    }
-  }
-
   if (options->user.icon_url) {
     status = security_checker_->ValidateAPrioriAuthenticatedUrl(
         *options->user.icon_url);
@@ -1017,6 +1017,19 @@ void AuthenticatorCommon::GetAssertion(
 
   caller_origin_ = caller_origin;
   relying_party_id_ = options->relying_party_id;
+
+  if (options->appid) {
+    requested_extensions_.insert(RequestExtension::kAppID);
+    app_id_ = ProcessAppIdExtension(GetRenderFrameHost()->GetBrowserContext(),
+                                    *options->appid, caller_origin_,
+                                    options->remote_desktop_client_override);
+    if (!app_id_) {
+      CompleteGetAssertionRequest(
+          blink::mojom::AuthenticatorStatus::INVALID_DOMAIN);
+      return;
+    }
+  }
+
   WebAuthenticationRequestProxy* proxy = GetWebAuthnRequestProxyIfActive();
   if (proxy) {
     if (options->remote_desktop_client_override) {
@@ -1105,18 +1118,6 @@ void AuthenticatorCommon::GetAssertion(
     empty_allow_list_ = true;
   }
 
-  if (options->appid) {
-    requested_extensions_.insert(RequestExtension::kAppID);
-    app_id_ = ProcessAppIdExtension(GetRenderFrameHost()->GetBrowserContext(),
-                                    *options->appid, caller_origin_,
-                                    options->remote_desktop_client_override);
-    if (!app_id_) {
-      CompleteGetAssertionRequest(
-          blink::mojom::AuthenticatorStatus::INVALID_DOMAIN);
-      return;
-    }
-  }
-
   if (options->large_blob_read && options->large_blob_write) {
     CompleteGetAssertionRequest(
         blink::mojom::AuthenticatorStatus::CANNOT_READ_AND_WRITE_LARGE_BLOB);

content/browser/webauth/authenticator_impl_unittest.cc

@@ -1742,7 +1742,7 @@ class TestWebAuthenticationRequestProxy : public WebAuthenticationRequestProxy {
 
   Config& config() { return config_; }
 
-  const Observations& observations() { return observations_; }
+  Observations& observations() { return observations_; }
 
   bool IsActive() override { return config_.is_active; }
 
@@ -8881,6 +8881,64 @@ TEST_F(AuthenticatorImplWithRequestProxyTest, MakeCredentialOriginAndRpIds) {
   }
 }
 
+TEST_F(AuthenticatorImplWithRequestProxyTest, AppId) {
+  request_proxy().config().request_success = true;
+  request_proxy().config().make_credential_response =
+      MakeCredentialAuthenticatorResponse::New();
+  request_proxy().config().make_credential_response->info =
+      CommonCredentialInfo::New();
+
+  for (const auto& test_case : kValidAppIdCases) {
+    SCOPED_TRACE(std::string(test_case.origin) + " " +
+                 std::string(test_case.claimed_authority));
+
+    ASSERT_TRUE(test_client_.GetWebAuthenticationDelegate()
+                    ->MaybeGetRequestProxy(main_rfh()->GetBrowserContext())
+                    ->IsActive());
+
+    EXPECT_EQ(TryAuthenticationWithAppId(test_case.origin,
+                                         test_case.claimed_authority),
+              AuthenticatorStatus::SUCCESS);
+    EXPECT_EQ(request_proxy().observations().get_requests.size(), 1u);
+    request_proxy().observations().get_requests.clear();
+
+    EXPECT_EQ(TryRegistrationWithAppIdExclude(test_case.origin,
+                                              test_case.claimed_authority),
+              AuthenticatorStatus::SUCCESS);
+    EXPECT_EQ(request_proxy().observations().create_requests.size(), 1u);
+    request_proxy().observations().create_requests.clear();
+  }
+
+  // Test invalid cases that should be rejected. `kInvalidRelyingPartyTestCases`
+  // contains a mix of RP ID an App ID cases, but they should all be rejected.
+  for (const OriginClaimedAuthorityPair& test_case :
+       kInvalidRelyingPartyTestCases) {
+    SCOPED_TRACE(std::string(test_case.claimed_authority) + " " +
+                 std::string(test_case.origin));
+
+    if (strlen(test_case.claimed_authority) == 0) {
+      // In this case, no AppID is actually being tested.
+      continue;
+    }
+
+    ASSERT_TRUE(test_client_.GetWebAuthenticationDelegate()
+                    ->MaybeGetRequestProxy(main_rfh()->GetBrowserContext())
+                    ->IsActive());
+
+    AuthenticatorStatus test_status = TryAuthenticationWithAppId(
+        test_case.origin, test_case.claimed_authority);
+    EXPECT_TRUE(test_status == AuthenticatorStatus::INVALID_DOMAIN ||
+                test_status == test_case.expected_status);
+    EXPECT_EQ(request_proxy().observations().get_requests.size(), 0u);
+
+    test_status = TryRegistrationWithAppIdExclude(test_case.origin,
+                                                  test_case.claimed_authority);
+    EXPECT_TRUE(test_status == AuthenticatorStatus::INVALID_DOMAIN ||
+                test_status == test_case.expected_status);
+    EXPECT_EQ(request_proxy().observations().create_requests.size(), 0u);
+  }
+}
+
 TEST_F(AuthenticatorImplWithRequestProxyTest, MakeCredential_Timeout) {
   request_proxy().config().resolve_callbacks = false;
   request_proxy().config().request_success = true;