[Backport] Security bug 340895241 (2/2)

Partial manual cherry-pick of patch originally reviewed on
https://chromium-review.googlesource.com/c/chromium/src/+/5574105:
[FedCM] Also decode the image in IdpNetworkRequestManager

This will make it much easier to let Android use the downloaded
image instead of re-downloading it. It also reduces code
duplication between desktop and Android. I will make the
Android change in a later CL to reduce the size of this CL.

Bug: 340895241
Change-Id: I0aad63dd22a06af690473e788a25dc719305d94f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5574105
Commit-Queue: Christian Biesinger <[email protected]>
Reviewed-by: Yi Gu <[email protected]>
Reviewed-by: Nasko Oskov <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1307731}
Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/580290
Reviewed-by: Michal Klocek <[email protected]>

Author: cbiesinger

chromium/content/browser/webid/federated_auth_request_impl.cc

@@ -1597,7 +1597,7 @@ void FederatedAuthRequestImpl::FetchAccountPictures(
                      accounts, client_metadata));
   for (const auto& account : accounts) {
     if (account.picture.is_valid()) {
-      network_manager_->DownloadUncredentialedUrl(
+      network_manager_->DownloadAndDecodeImage(
           account.picture,
           base::BindOnce(&FederatedAuthRequestImpl::OnAccountPictureReceived,
                          weak_ptr_factory_.GetWeakPtr(), callback,
@@ -1613,12 +1613,8 @@ void FederatedAuthRequestImpl::FetchAccountPictures(
 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);
-  }
+    const gfx::Image& image) {
+  downloaded_images_[url] = image;
   cb.Run();
 }
 
@@ -1629,10 +1625,9 @@ void FederatedAuthRequestImpl::OnAllAccountPicturesReceived(
   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;
+      // We do not use std::move here in case multiple accounts use the same
+      // picture URL, and the underlying gfx::Image data is refcounted anyway.
+      account.decoded_picture = it->second;
     }
   }
   downloaded_images_.clear();

chromium/content/browser/webid/federated_auth_request_impl.h

@@ -29,6 +29,10 @@
 #include "third_party/blink/public/mojom/webid/federated_auth_request.mojom.h"
 #include "url/gurl.h"
 
+namespace gfx {
+class Image;
+}
+
 namespace content {
 
 class FederatedAuthUserInfoRequest;
@@ -247,9 +251,7 @@ class CONTENT_EXPORT FederatedAuthRequestImpl
       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);
+                                const gfx::Image& image);
   void OnAllAccountPicturesReceived(
       std::unique_ptr<IdentityProviderInfo> idp_info,
       IdpNetworkRequestManager::AccountList accounts,
@@ -373,7 +375,7 @@ class CONTENT_EXPORT FederatedAuthRequestImpl
   std::vector<IdentityProviderData> idp_data_for_display_;
 
   // The downloaded image data.
-  std::map<GURL, std::unique_ptr<std::string>> downloaded_images_;
+  std::map<GURL, gfx::Image> downloaded_images_;
 
   raw_ptr<FederatedIdentityApiPermissionContextDelegate>
       api_permission_delegate_ = nullptr;

chromium/content/browser/webid/idp_network_request_manager.cc

@@ -24,6 +24,7 @@
 #include "net/cookies/site_for_cookies.h"
 #include "net/http/http_request_headers.h"
 #include "net/http/http_status_code.h"
+#include "services/data_decoder/public/cpp/decode_image.h"
 #include "services/network/public/cpp/is_potentially_trustworthy.h"
 #include "services/network/public/cpp/resource_request.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
@@ -33,6 +34,7 @@
 #include "third_party/blink/public/common/manifest/manifest_icon_selector.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/gfx/color_utils.h"
+#include "ui/gfx/image/image.h"
 #include "url/origin.h"
 
 namespace content {
@@ -813,15 +815,17 @@ void IdpNetworkRequestManager::SendLogout(const GURL& logout_url,
               maxResponseSizeInKiB * 1024);
 }
 
-void IdpNetworkRequestManager::DownloadUncredentialedUrl(
-    const GURL& url,
-    DownloadCallback callback) {
+void IdpNetworkRequestManager::DownloadAndDecodeImage(const GURL& url,
+                                                      ImageCallback 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);
+  DownloadUrl(
+      std::move(resource_request),
+      /*url_encoded_post_data=*/absl::nullopt,
+      base::BindOnce(&IdpNetworkRequestManager::OnDownloadedImage,
+                     weak_ptr_factory_.GetWeakPtr(), std::move(callback)),
+      maxResponseSizeInKiB * 1024);
 }
 
 void IdpNetworkRequestManager::DownloadJsonAndParse(
@@ -903,6 +907,29 @@ void IdpNetworkRequestManager::FetchClientMetadata(
       maxResponseSizeInKiB * 1024);
 }
 
+void IdpNetworkRequestManager::OnDownloadedImage(
+    ImageCallback callback,
+    std::unique_ptr<std::string> response_body,
+    int response_code,
+    const std::string& mime_type) {
+  if (!response_body || response_code != net::HTTP_OK) {
+    std::move(callback).Run(gfx::Image());
+    return;
+  }
+
+  data_decoder::DecodeImageIsolated(
+      base::as_bytes(base::make_span(*response_body)),
+      data_decoder::mojom::ImageCodec::kDefault, /*shrink_to_fit=*/false,
+      data_decoder::kDefaultMaxSizeInBytes, gfx::Size(),
+      base::BindOnce(&IdpNetworkRequestManager::OnDecodedImage,
+                     weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+}
+
+void IdpNetworkRequestManager::OnDecodedImage(ImageCallback callback,
+                                              const SkBitmap& decoded_bitmap) {
+  std::move(callback).Run(gfx::Image::CreateFrom1xBitmap(decoded_bitmap));
+}
+
 std::unique_ptr<network::ResourceRequest>
 IdpNetworkRequestManager::CreateUncredentialedResourceRequest(
     const GURL& target_url,

chromium/content/browser/webid/idp_network_request_manager.h

@@ -21,6 +21,10 @@
 #include "url/gurl.h"
 #include "url/origin.h"
 
+namespace gfx {
+class Image;
+}
+
 namespace net {
 enum class ReferrerPolicy;
 }
@@ -170,6 +174,7 @@ class CONTENT_EXPORT IdpNetworkRequestManager {
   using TokenRequestCallback =
       base::OnceCallback<void(FetchStatus, TokenResult)>;
   using ContinueOnCallback = base::OnceCallback<void(FetchStatus, const GURL&)>;
+  using ImageCallback = base::OnceCallback<void(const gfx::Image&)>;
 
   static std::unique_ptr<IdpNetworkRequestManager> Create(
       RenderFrameHostImpl* host);
@@ -230,9 +235,8 @@ 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);
+  // Download and decode an image. The request is made uncredentialed.
+  virtual void DownloadAndDecodeImage(const GURL& url, ImageCallback callback);
 
  private:
   // Starts download request using `url_loader`. Calls `parse_json_callback`
@@ -255,6 +259,13 @@ class CONTENT_EXPORT IdpNetworkRequestManager {
                        DownloadCallback callback,
                        std::unique_ptr<std::string> response_body);
 
+  void OnDownloadedImage(ImageCallback callback,
+                         std::unique_ptr<std::string> response_body,
+                         int response_code,
+                         const std::string& mime_type);
+
+  void OnDecodedImage(ImageCallback callback, const SkBitmap& decoded_bitmap);
+
   std::unique_ptr<network::ResourceRequest> CreateUncredentialedResourceRequest(
       const GURL& target_url,
       bool send_origin,

chromium/content/public/browser/identity_request_account.h

@@ -11,6 +11,7 @@
 #include "content/common/content_export.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
 #include "third_party/skia/include/core/SkColor.h"
+#include "ui/gfx/image/image.h"
 #include "url/gurl.h"
 
 namespace content {
@@ -57,8 +58,8 @@ 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;
+  // This will be an empty image if fetching failed.
+  gfx::Image decoded_picture;
 
   std::vector<std::string> login_hints;
   std::vector<std::string> hosted_domains;