[Backport] Security bug 340895241 (1/2)

Partial manual cherry-pick of patch originally reviewed on
https://chromium-review.googlesource.com/c/chromium/src/+/5564909:
[FedCM] Download all profile pictures

This ensures that we download the pictures even if the account later
gets filtered out.

This CL only does desktop because Android needs JNI changes for
ImageDecoder first.

We can't do the decoding in content/ because ImageDecoderImpl lives
in chrome/.

Bug: 340895241
Change-Id: Ie973c8ab57cc0810bb9b4d988904738e427421f2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5564909
Reviewed-by: Yi Gu <[email protected]>
Reviewed-by: Dave Tapuska <[email protected]>
Commit-Queue: Christian Biesinger <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1306939}
Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/580289
Reviewed-by: Allan Sandfeld Jensen <[email protected]>

Author: cbiesinger

chromium/content/browser/webid/federated_auth_request_impl.cc

@@ -4,6 +4,7 @@
 
 #include "content/browser/webid/federated_auth_request_impl.h"
 
+#include "base/barrier_closure.h"
 #include "base/command_line.h"
 #include "base/containers/contains.h"
 #include "base/containers/cxx20_erase.h"
@@ -1102,7 +1103,7 @@ void FederatedAuthRequestImpl::OnClientMetadataResponseReceived(
     IdpNetworkRequestManager::ClientMetadata client_metadata) {
   // TODO(yigu): Clean up the client metadata related errors for metrics and
   // console logs.
-  OnFetchDataForIdpSucceeded(std::move(idp_info), accounts, client_metadata);
+  FetchAccountPictures(std::move(idp_info), accounts, client_metadata);
 }
 
 bool HasScope(const std::vector<std::string>& scope, std::string name) {
@@ -1578,13 +1579,66 @@ void FederatedAuthRequestImpl::OnAccountsResponseReceived(
                 weak_ptr_factory_.GetWeakPtr(), std::move(idp_info),
                 std::move(accounts)));
       } else {
-        OnFetchDataForIdpSucceeded(std::move(idp_info), accounts,
-                                   IdpNetworkRequestManager::ClientMetadata());
+        FetchAccountPictures(std::move(idp_info), accounts,
+                             IdpNetworkRequestManager::ClientMetadata());
       }
     }
   }
 }
 
+void FederatedAuthRequestImpl::FetchAccountPictures(
+    std::unique_ptr<IdentityProviderInfo> idp_info,
+    const IdpNetworkRequestManager::AccountList& accounts,
+    const IdpNetworkRequestManager::ClientMetadata& client_metadata) {
+  auto callback = BarrierClosure(
+      accounts.size(),
+      base::BindOnce(&FederatedAuthRequestImpl::OnAllAccountPicturesReceived,
+                     weak_ptr_factory_.GetWeakPtr(), std::move(idp_info),
+                     accounts, client_metadata));
+  for (const auto& account : accounts) {
+    if (account.picture.is_valid()) {
+      network_manager_->DownloadUncredentialedUrl(
+          account.picture,
+          base::BindOnce(&FederatedAuthRequestImpl::OnAccountPictureReceived,
+                         weak_ptr_factory_.GetWeakPtr(), callback,
+                         account.picture));
+    } else {
+      // We have to still call the callback to make sure the barrier
+      // callback gets the right number of calls.
+      callback.Run();
+    }
+  }
+}
+
+void FederatedAuthRequestImpl::OnAccountPictureReceived(
+    base::RepeatingClosure cb,
+    GURL url,
+    std::unique_ptr<std::string> response_body,
+    int response_code,
+    const std::string& mime_type) {
+  if (response_body) {
+    downloaded_images_[url] = std::move(response_body);
+  }
+  cb.Run();
+}
+
+void FederatedAuthRequestImpl::OnAllAccountPicturesReceived(
+    std::unique_ptr<IdentityProviderInfo> idp_info,
+    IdpNetworkRequestManager::AccountList accounts,
+    const IdpNetworkRequestManager::ClientMetadata& client_metadata) {
+  for (auto& account : accounts) {
+    auto it = downloaded_images_.find(account.picture);
+    if (it != downloaded_images_.end()) {
+      // We do not use std::move here in case multiple accounts use the
+      // same picture URL.
+      DCHECK(it->second);
+      account.picture_data = *it->second;
+    }
+  }
+  downloaded_images_.clear();
+  OnFetchDataForIdpSucceeded(std::move(idp_info), accounts, client_metadata);
+}
+
 void FederatedAuthRequestImpl::ComputeLoginStateAndReorderAccounts(
     const IdentityProviderConfigPtr& idp,
     IdpNetworkRequestManager::AccountList& accounts) {

chromium/content/browser/webid/federated_auth_request_impl.h

@@ -239,6 +239,21 @@ class CONTENT_EXPORT FederatedAuthRequestImpl
       std::unique_ptr<IdentityProviderInfo> idp_info,
       IdpNetworkRequestManager::FetchStatus status,
       IdpNetworkRequestManager::AccountList accounts);
+  // Fetches the account pictures for |accounts| and calls
+  // OnFetchDataForIdpSucceeded when done.
+  void FetchAccountPictures(
+      std::unique_ptr<IdentityProviderInfo> idp_info,
+      const IdpNetworkRequestManager::AccountList& accounts,
+      const IdpNetworkRequestManager::ClientMetadata& client_metadata);
+  void OnAccountPictureReceived(base::RepeatingClosure cb,
+                                GURL url,
+                                std::unique_ptr<std::string> response_body,
+                                int response_code,
+                                const std::string& mime_type);
+  void OnAllAccountPicturesReceived(
+      std::unique_ptr<IdentityProviderInfo> idp_info,
+      IdpNetworkRequestManager::AccountList accounts,
+      const IdpNetworkRequestManager::ClientMetadata& client_metadata);
   void OnAccountSelected(const GURL& idp_config_url,
                          const std::string& account_id,
                          bool is_sign_in);
@@ -357,6 +372,9 @@ class CONTENT_EXPORT FederatedAuthRequestImpl
   base::flat_map<GURL, std::unique_ptr<IdentityProviderInfo>> idp_infos_;
   std::vector<IdentityProviderData> idp_data_for_display_;
 
+  // The downloaded image data.
+  std::map<GURL, std::unique_ptr<std::string>> downloaded_images_;
+
   raw_ptr<FederatedIdentityApiPermissionContextDelegate>
       api_permission_delegate_ = nullptr;
   raw_ptr<FederatedIdentityAutoReauthnPermissionContextDelegate>

chromium/content/browser/webid/idp_network_request_manager.cc

@@ -813,6 +813,17 @@ void IdpNetworkRequestManager::SendLogout(const GURL& logout_url,
               maxResponseSizeInKiB * 1024);
 }
 
+void IdpNetworkRequestManager::DownloadUncredentialedUrl(
+    const GURL& url,
+    DownloadCallback callback) {
+  std::unique_ptr<network::ResourceRequest> resource_request =
+      CreateUncredentialedResourceRequest(url, /*send_origin=*/false);
+
+  DownloadUrl(std::move(resource_request),
+              /*url_encoded_post_data=*/std::nullopt, std::move(callback),
+              maxResponseSizeInKiB * 1024);
+}
+
 void IdpNetworkRequestManager::DownloadJsonAndParse(
     std::unique_ptr<network::ResourceRequest> resource_request,
     absl::optional<std::string> url_encoded_post_data,

chromium/content/browser/webid/idp_network_request_manager.h

@@ -230,6 +230,10 @@ class CONTENT_EXPORT IdpNetworkRequestManager {
   // Send logout request to a single target.
   virtual void SendLogout(const GURL& logout_url, LogoutCallback);
 
+  // Download a URL. The request is made uncredentialed.
+  virtual void DownloadUncredentialedUrl(const GURL& url,
+                                         DownloadCallback callback);
+
  private:
   // Starts download request using `url_loader`. Calls `parse_json_callback`
   // when the download result has been parsed.

chromium/content/public/browser/identity_request_account.h

@@ -57,6 +57,9 @@ struct CONTENT_EXPORT IdentityRequestAccount {
   std::string name;
   std::string given_name;
   GURL picture;
+  // This will be an empty string if fetching failed.
+  std::string picture_data;
+
   std::vector<std::string> login_hints;
   std::vector<std::string> hosted_domains;