Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: backport security fixes from Chromium release M90-3 #29251

Merged
merged 5 commits into from
May 20, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions patches/chromium/.patches
Original file line number Diff line number Diff line change
Expand Up @@ -121,3 +121,11 @@ add_crashkeys_to_identify_where_target_is_assigned_to_a_stale_value.patch
add_weak_pointer_to_rwhier_framesinkidownermap_and_rwhier_targetmap.patch
add_null_pointer_check_in_renderwidgethostinputeventrouter.patch
cherry-pick-8089dbfc616f.patch
replace_std_vector_with_base_observerlist_to_support_container.patch
guard_webcontents_downloadimage_against_malformed_renderer.patch
fileapi_terminate_filereaderloader_before_dispatching_onabort_event.patch
notifications_crash_if_improper_action_icons_sent_from_renderer.patch
reland_views_handle_deletion_when_toggling_fullscreen.patch
media_feeds_disable_media_feeds_and_related_features.patch
remove_tabs_and_line_breaks_from_the_middle_of_app_names_when.patch
autofill_fixed_refill_of_changed_form.patch
41 changes: 41 additions & 0 deletions patches/chromium/autofill_fixed_refill_of_changed_form.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Andrey Belenko <andrey.belenko@gmail.com>
Date: Wed, 19 May 2021 17:20:52 +0200
Subject: Fixed refill of changed form.

(cherry picked from commit 533bb3adcfe3499f90e2646fc60312f303b963ac)

Bug: 1203667
Change-Id: I2693a024531775e0e60cc330107d77d10558f466
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2867655
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2874611

diff --git a/components/autofill/core/browser/autofill_manager.cc b/components/autofill/core/browser/autofill_manager.cc
index 866d4b98a839d73777edafc681af7d809cbfc56b..a49386223c864395768970a44840fa1f462fad24 100644
--- a/components/autofill/core/browser/autofill_manager.cc
+++ b/components/autofill/core/browser/autofill_manager.cc
@@ -1669,7 +1669,10 @@ void AutofillManager::FillOrPreviewDataModelForm(
form_structure->RationalizePhoneNumbersInSection(autofill_field->section);

FormData result = form;
- DCHECK_EQ(form_structure->field_count(), form.fields.size());
+ // TODO(crbug/1203667#c9): Skip if the form has changed in the meantime, which
+ // may happen with refills.
+ if (form_structure->field_count() != form.fields.size())
+ return;

if (action == AutofillDriver::FORM_DATA_ACTION_FILL && !is_refill) {
SetFillingContext(
@@ -1710,8 +1713,10 @@ void AutofillManager::FillOrPreviewDataModelForm(
continue;
}

- // The field order should be the same in |form_structure| and |result|.
- DCHECK(form_structure->field(i)->SameFieldAs(result.fields[i]));
+ // TODO(crbug/1203667#c9): Skip if the form has changed in the meantime,
+ // which may happen with refills.
+ if (!form_structure->field(i)->SameFieldAs(result.fields[i]))
+ continue;

AutofillField* cached_field = form_structure->field(i);
FieldTypeGroup field_group_type = cached_field->Type().group();
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Andrey Belenko <andrey.belenko@gmail.com>
Date: Wed, 19 May 2021 12:24:19 +0200
Subject: FileAPI: Terminate FileReaderLoader before dispatching onabort event.

Otherwise FileReader could end up in an inconsistent state where a load
is still in progress while the state was set to done.

(cherry picked from commit a74c980df61dd7367ad1b11e6a735be82d2696f0)

Bug: 1201073
Change-Id: Ib2c833537e1badc57d125568d5d35f53f12582a8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2860442
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2871355

diff --git a/third_party/blink/renderer/core/fileapi/file_reader.cc b/third_party/blink/renderer/core/fileapi/file_reader.cc
index 2bbc4db7923c659eda6d63230998ca3414f397d3..013f451efe3eb0bfb9232f6dc7e0b675a6b2a55a 100644
--- a/third_party/blink/renderer/core/fileapi/file_reader.cc
+++ b/third_party/blink/renderer/core/fileapi/file_reader.cc
@@ -335,7 +335,10 @@ void FileReader::abort() {
loading_state_ = kLoadingStateAborted;

DCHECK_NE(kDone, state_);
- state_ = kDone;
+ // Synchronously cancel the loader before dispatching events. This way we make
+ // sure the FileReader internal state stays consistent even if another load
+ // is started from one of the event handlers, or right after abort returns.
+ Terminate();

base::AutoReset<bool> firing_events(&still_firing_events_, true);

@@ -347,15 +350,12 @@ void FileReader::abort() {
ThrottlingController::RemoveReader(GetExecutionContext(), this);

FireEvent(event_type_names::kAbort);
+ // TODO(https://crbug.com/1204139): Only fire loadend event if no new load was
+ // started from the abort event handler.
FireEvent(event_type_names::kLoadend);

// All possible events have fired and we're done, no more pending activity.
ThrottlingController::FinishReader(GetExecutionContext(), this, final_step);
-
- // Also synchronously cancel the loader, as script might initiate a new load
- // right after this method returns, in which case an async termination would
- // terminate the wrong loader.
- Terminate();
}

void FileReader::result(StringOrArrayBuffer& result_attribute) const {
@@ -428,6 +428,8 @@ void FileReader::DidFinishLoading() {
ThrottlingController::RemoveReader(GetExecutionContext(), this);

FireEvent(event_type_names::kLoad);
+ // TODO(https://crbug.com/1204139): Only fire loadend event if no new load was
+ // started from the abort event handler
FireEvent(event_type_names::kLoadend);

// All possible events have fired and we're done, no more pending activity.
Original file line number Diff line number Diff line change
@@ -0,0 +1,226 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Andrey Belenko <andrey.belenko@gmail.com>
Date: Wed, 19 May 2021 10:40:04 +0200
Subject: Guard WebContents::DownloadImage() against malformed renderer
response

Callers expect that ImageDownloadCallback gets invoked with two vectors
having the same number of elements (one containing the bitmaps and the
other one the corresponding sizes).

However, these vectors are populated directly from the Mojo response,
so there needs to be some browser-process sanitization to protect
against buggy or compromised renderers.

In this patch, WebContentsImpl::OnDidDownloadImage() mimics a 400 error
if the response is malformed, similarly to how it's done in other edge
cases (renderer process dead upon download). Because this scenario is
a violation of the Mojo API contract, the browser process also issues
a bad message log (newly-introduced WCI_INVALID_DOWNLOAD_IMAGE_RESULT)
and shuts down the renderer process.

(cherry picked from commit 034ba14e44f08e8ca84b42350f3238f847e08e5f)

Change-Id: Ic0843e10efc26809fabd8f1bbe506ba1703d1486
Fixed: 1201446
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2871796

diff --git a/components/favicon/core/favicon_handler.cc b/components/favicon/core/favicon_handler.cc
index 0d53f634fb760b23ca5af660ea527ad699af8170..3aa805d0f5f4961badc516a22051e28b3303024a 100644
--- a/components/favicon/core/favicon_handler.cc
+++ b/components/favicon/core/favicon_handler.cc
@@ -507,6 +507,8 @@ void FaviconHandler::OnDidDownloadFavicon(
const GURL& image_url,
const std::vector<SkBitmap>& bitmaps,
const std::vector<gfx::Size>& original_bitmap_sizes) {
+ DCHECK_EQ(bitmaps.size(), original_bitmap_sizes.size());
+
// Mark download as finished.
image_download_request_.Cancel();

diff --git a/components/favicon/core/favicon_handler.h b/components/favicon/core/favicon_handler.h
index 98706de7a7bcdeee857acd5a9113c45ecf82dca7..574d88ac91a6322644cbe5fda9e964807830c20e 100644
--- a/components/favicon/core/favicon_handler.h
+++ b/components/favicon/core/favicon_handler.h
@@ -238,7 +238,9 @@ class FaviconHandler {
void ScheduleImageDownload(const GURL& image_url,
favicon_base::IconType icon_type);

- // Triggered when a download of an image has finished.
+ // Triggered when a download of an image has finished. |bitmaps| and
+ // |original_bitmap_sizes| must contain the same number of elements (i.e. same
+ // vector size).
void OnDidDownloadFavicon(
favicon_base::IconType icon_type,
int id,
diff --git a/components/favicon/ios/web_favicon_driver.mm b/components/favicon/ios/web_favicon_driver.mm
index 67eed96b73f37b37485a6f52fd2113ff84aca30d..ce9a3390ebe878f556e2dc6130a85ca58d16cb65 100644
--- a/components/favicon/ios/web_favicon_driver.mm
+++ b/components/favicon/ios/web_favicon_driver.mm
@@ -75,6 +75,7 @@
for (const auto& frame : frames) {
sizes.push_back(gfx::Size(frame.width(), frame.height()));
}
+ DCHECK_EQ(frames.size(), sizes.size());
}
std::move(local_callback)
.Run(local_download_id, metadata.http_response_code, local_url,
diff --git a/components/favicon_base/select_favicon_frames.cc b/components/favicon_base/select_favicon_frames.cc
index 6d482593eb288ef85838883d845838876e567624..8dacf5c20bd765e9a84ab32cea1f5e2a8b083fb2 100644
--- a/components/favicon_base/select_favicon_frames.cc
+++ b/components/favicon_base/select_favicon_frames.cc
@@ -216,6 +216,7 @@ gfx::ImageSkia CreateFaviconImageSkia(
const std::vector<gfx::Size>& original_sizes,
int desired_size_in_dip,
float* score) {
+ DCHECK_EQ(bitmaps.size(), original_sizes.size());

const std::vector<float>& favicon_scales = favicon_base::GetFaviconScales();
std::vector<int> desired_sizes;
diff --git a/components/favicon_base/select_favicon_frames.h b/components/favicon_base/select_favicon_frames.h
index 573a38c79cddf839622488589b71b4d19fbdfba6..eab1e54466763e5a6a6069a29e877da054972fa8 100644
--- a/components/favicon_base/select_favicon_frames.h
+++ b/components/favicon_base/select_favicon_frames.h
@@ -38,6 +38,8 @@ extern const float kSelectFaviconFramesInvalidScore;
// it inspired by this method.
// If an unsupported scale (not in the favicon_base::GetFaviconScales())
// is requested, the ImageSkia will automatically scales using lancoz3.
+// |original_sizes| represents the pixel sizes of the favicon bitmaps in
+// |bitmaps|, which also means both vectors must have the same size.
gfx::ImageSkia CreateFaviconImageSkia(
const std::vector<SkBitmap>& bitmaps,
const std::vector<gfx::Size>& original_sizes,
diff --git a/content/browser/bad_message.h b/content/browser/bad_message.h
index e094f707393d4542ecaaa40fea6339badcfaa2bd..e198d661c711dc648a4e4c2249e0a60f69c406da 100644
--- a/content/browser/bad_message.h
+++ b/content/browser/bad_message.h
@@ -266,6 +266,9 @@ enum BadMessageReason {
CSDH_BAD_OWNER = 238,
SYNC_COMPOSITOR_NO_LOCAL_SURFACE_ID = 239,
WCI_INVALID_FULLSCREEN_OPTIONS = 240,
+ PAYMENTS_WITHOUT_PERMISSION = 241,
+ WEB_BUNDLE_INVALID_NAVIGATION_URL = 242,
+ WCI_INVALID_DOWNLOAD_IMAGE_RESULT = 243,

// Please add new elements here. The naming convention is abbreviated class
// name (e.g. RenderFrameHost becomes RFH) plus a unique description of the
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index af8196ef8a368b6e91703a8b91db58b94417a182..258c95ff609f0b074d74d1f54f5b9e842e33ed7d 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -168,6 +168,7 @@
#include "third_party/blink/public/common/web_preferences/web_preferences.h"
#include "third_party/blink/public/mojom/frame/frame.mojom.h"
#include "third_party/blink/public/mojom/frame/fullscreen.mojom.h"
+#include "third_party/blink/public/mojom/image_downloader/image_downloader.mojom.h"
#include "third_party/blink/public/mojom/loader/pause_subresource_loading_handle.mojom.h"
#include "third_party/blink/public/mojom/mediastream/media_stream.mojom-shared.h"
#include "third_party/skia/include/core/SkBitmap.h"
@@ -4914,18 +4915,18 @@ int WebContentsImpl::DownloadImageInFrame(
// respond with a 400 HTTP error code to indicate that something went wrong.
GetUIThreadTaskRunner({})->PostTask(
FROM_HERE,
- base::BindOnce(&WebContentsImpl::OnDidDownloadImage,
- weak_factory_.GetWeakPtr(), std::move(callback),
- download_id, url, 400, std::vector<SkBitmap>(),
- std::vector<gfx::Size>()));
+ base::BindOnce(
+ &WebContentsImpl::OnDidDownloadImage, weak_factory_.GetWeakPtr(),
+ initiator_frame->GetWeakPtr(), std::move(callback), download_id,
+ url, 400, std::vector<SkBitmap>(), std::vector<gfx::Size>()));
return download_id;
}

mojo_image_downloader->DownloadImage(
url, is_favicon, preferred_size, max_bitmap_size, bypass_cache,
base::BindOnce(&WebContentsImpl::OnDidDownloadImage,
- weak_factory_.GetWeakPtr(), std::move(callback),
- download_id, url));
+ weak_factory_.GetWeakPtr(), initiator_frame->GetWeakPtr(),
+ std::move(callback), download_id, url));
return download_id;
}

@@ -7905,6 +7906,7 @@ bool WebContentsImpl::CompletedFirstVisuallyNonEmptyPaint() {
}

void WebContentsImpl::OnDidDownloadImage(
+ base::WeakPtr<RenderFrameHostImpl> rfh,
ImageDownloadCallback callback,
int id,
const GURL& image_url,
@@ -7914,6 +7916,21 @@ void WebContentsImpl::OnDidDownloadImage(
OPTIONAL_TRACE_EVENT1("content", "WebContentsImpl::OnDidDownloadImage",
"image_url",
base::trace_event::ValueToString(image_url));
+
+ // Guard against buggy or compromised renderers that could violate the API
+ // contract that |images| and |original_image_sizes| must have the same
+ // length.
+ if (images.size() != original_image_sizes.size()) {
+ if (rfh) {
+ ReceivedBadMessage(rfh->GetProcess(),
+ bad_message::WCI_INVALID_DOWNLOAD_IMAGE_RESULT);
+ }
+ // Respond with a 400 to indicate that something went wrong.
+ std::move(callback).Run(id, 400, image_url, std::vector<SkBitmap>(),
+ std::vector<gfx::Size>());
+ return;
+ }
+
std::move(callback).Run(id, http_status_code, image_url, images,
original_image_sizes);
}
diff --git a/content/browser/web_contents/web_contents_impl.h b/content/browser/web_contents/web_contents_impl.h
index 357ba07b83577392a902203cd7ee50e0ba3d27dd..9208a11b52c559de08dc0ff66fd37865c692bfe7 100644
--- a/content/browser/web_contents/web_contents_impl.h
+++ b/content/browser/web_contents/web_contents_impl.h
@@ -1502,7 +1502,8 @@ class CONTENT_EXPORT WebContentsImpl : public WebContents,
std::set<RenderWidgetHostView*>& result);

// Called with the result of a DownloadImage() request.
- void OnDidDownloadImage(ImageDownloadCallback callback,
+ void OnDidDownloadImage(base::WeakPtr<RenderFrameHostImpl> rfh,
+ ImageDownloadCallback callback,
int id,
const GURL& image_url,
int32_t http_status_code,
diff --git a/content/public/browser/web_contents.h b/content/public/browser/web_contents.h
index c558236eb621ae6a67468afaa1b8431b9857a35f..3a333f0149a31c7c11a324bfdf06e356ff9d5081 100644
--- a/content/public/browser/web_contents.h
+++ b/content/public/browser/web_contents.h
@@ -955,8 +955,9 @@ class WebContents : public PageNavigator,
// |bitmaps| will be empty on download failure.
// |sizes| are the sizes in pixels of the bitmaps before they were resized due
// to the max bitmap size passed to DownloadImage(). Each entry in the bitmaps
- // vector corresponds to an entry in the sizes vector. If a bitmap was
- // resized, there should be a single returned bitmap.
+ // vector corresponds to an entry in the sizes vector (both vector sizes are
+ // guaranteed to be equal). If a bitmap was resized, there should be a single
+ // returned bitmap.
using ImageDownloadCallback =
base::OnceCallback<void(int id,
int http_status_code,
diff --git a/third_party/blink/renderer/modules/image_downloader/image_downloader_impl.cc b/third_party/blink/renderer/modules/image_downloader/image_downloader_impl.cc
index c81a25b0d24b9c4a8e96a38e868fa2795aba0da0..b32ebc06ba2eb68f8406a913dc0da75a0c2b183e 100644
--- a/third_party/blink/renderer/modules/image_downloader/image_downloader_impl.cc
+++ b/third_party/blink/renderer/modules/image_downloader/image_downloader_impl.cc
@@ -79,7 +79,8 @@ SkBitmap ResizeImage(const SkBitmap& image, uint32_t max_image_size) {
// size |max_image_size|. Returns the result if it is not empty. Otherwise,
// find the smallest image in the array and resize it proportionally to fit
// in a box of size |max_image_size|.
-// Sets |original_image_sizes| to the sizes of |images| before resizing.
+// Sets |original_image_sizes| to the sizes of |images| before resizing. Both
+// output vectors are guaranteed to have the same size.
void FilterAndResizeImagesForMaximalSize(
const WTF::Vector<SkBitmap>& unfiltered,
uint32_t max_image_size,
@@ -202,6 +203,8 @@ void ImageDownloaderImpl::DidDownloadImage(
FilterAndResizeImagesForMaximalSize(images, max_image_size, &result_images,
&result_original_image_sizes);

+ DCHECK_EQ(result_images.size(), result_original_image_sizes.size());
+
std::move(callback).Run(http_status_code, result_images,
result_original_image_sizes);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Andrey Belenko <andrey.belenko@gmail.com>
Date: Wed, 19 May 2021 14:10:46 +0200
Subject: Media Feeds: Disable Media Feeds and related features

Media Feeds is deleted in M91 and later and is unused in previous
versions as well. There is a security issue with Media Feeds though, so
we'd like to force it to be disabled in previous versions, so this CL
turns it off for M90.

(cherry picked from commit b064a73431541e520d273c227e762983c2f177b7)

Bug: 1195340
Change-Id: I29e18be2abe4c1b4560d6324af3b6da93a97d947
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2847504
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2883741

diff --git a/media/base/media_switches.cc b/media/base/media_switches.cc
index f8b848e7283b7d89b256b179871a1b82844a6363..a38bdb825bd443fcf12a7b9b74dc99aa33397bcf 100644
--- a/media/base/media_switches.cc
+++ b/media/base/media_switches.cc
@@ -765,15 +765,16 @@ const base::Feature kMediaEngagementHTTPSOnly{

// Enables Media Feeds to allow sites to provide specific recommendations for
// users.
-const base::Feature kMediaFeeds{"MediaFeeds", base::FEATURE_ENABLED_BY_DEFAULT};
+const base::Feature kMediaFeeds{"MediaFeeds",
+ base::FEATURE_DISABLED_BY_DEFAULT};

// Enables fetching Media Feeds periodically in the background.
const base::Feature kMediaFeedsBackgroundFetching{
- "MediaFeedsBackgroundFetching", base::FEATURE_ENABLED_BY_DEFAULT};
+ "MediaFeedsBackgroundFetching", base::FEATURE_DISABLED_BY_DEFAULT};

// Enables checking Media Feeds against safe search to prevent adult content.
const base::Feature kMediaFeedsSafeSearch{"MediaFeedsSafeSearch",
- base::FEATURE_ENABLED_BY_DEFAULT};
+ base::FEATURE_DISABLED_BY_DEFAULT};

// Enables experimental local learning for media. Used in the context of media
// capabilities only. Adds reporting only; does not change media behavior.
Loading