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 the security fixes from Chromium release M90-3 #29249

Merged
merged 2 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions patches/chromium/.patches
Expand Up @@ -178,3 +178,11 @@ cherry-pick-5745eaf16077.patch
cherry-pick-02f5ef8c88d7.patch
cherry-pick-668cf831e912.patch
cherry-pick-f37149c4434f.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
@@ -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:24:48 +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 ded9c514894eee34a34eb562b08e7484673dfc4a..ca3b53835c2cacfeab893128a75e4d427d7993e8 100644
--- a/components/autofill/core/browser/autofill_manager.cc
+++ b/components/autofill/core/browser/autofill_manager.cc
@@ -1740,7 +1740,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) {
filling_contexts_map_[form_structure->GetIdentifierForRefill()] =
@@ -1784,8 +1787,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();
@@ -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:29:29 +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 978d71510a5a0a790ea86dcd5413665ff624619c..8549fbef76a79cc4eff0fdc262e39720bfff40ba 100644
--- a/third_party/blink/renderer/core/fileapi/file_reader.cc
+++ b/third_party/blink/renderer/core/fileapi/file_reader.cc
@@ -337,7 +337,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);

@@ -349,15 +352,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(ScriptState* state,
@@ -429,6 +429,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.
@@ -0,0 +1,231 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Andrey Belenko <andrey.belenko@gmail.com>
Date: Wed, 19 May 2021 12:19:16 +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 64eab051de73b0b259c1f31e93f750d352724521..410ffc64e8f70a29a39cc4fb228dfd720bfe04d7 100644
--- a/components/favicon/core/favicon_handler.cc
+++ b/components/favicon/core/favicon_handler.cc
@@ -492,6 +492,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 8a1cf2ef85745711e94ec6e7f6f27acb1482cbd6..4192138711de470eee713fc1fb200cc29d333342 100644
--- a/components/favicon/core/favicon_handler.h
+++ b/components/favicon/core/favicon_handler.h
@@ -237,7 +237,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 6efaf651bdf939d83e1a7f9df339f714410565ad..71ae020efd8862d2b33cb91df9180ef63df1f6f9 100644
--- a/components/favicon/ios/web_favicon_driver.mm
+++ b/components/favicon/ios/web_favicon_driver.mm
@@ -75,6 +75,7 @@ int WebFaviconDriver::DownloadImage(const GURL& url,
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 90b58e621492d0e503f7965de91581c0a451d163..73cd7dd5331a818f413d831b4ae596bc88805a8a 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 ab0a195c2da772ea5825bb97e54743318bd06841..efc803efe24f61012882399035f6d96aed799595 100644
--- a/content/browser/bad_message.h
+++ b/content/browser/bad_message.h
@@ -256,6 +256,15 @@ enum BadMessageReason {
INPUT_ROUTER_INVALID_EVENT_SOURCE = 228,
RWH_CLOSE_PORTAL = 233,
MSDH_INVALID_STREAM_TYPE = 234,
+ RFH_CREATE_CHILD_FRAME_TOKENS_NOT_FOUND = 235,
+ ASGH_ASSOCIATED_INTERFACE_REQUEST = 236,
+ ASGH_RECEIVED_CONTROL_MESSAGE = 237,
+ 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 a7c76413b86fac18f6f1f54c87e67218f094e6b2..67deba5a7ab4265c76fe18990ede82c2705efe70 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -160,6 +160,7 @@
#include "third_party/blink/public/common/security/security_style.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"
@@ -4295,18 +4296,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;
}

@@ -6829,12 +6830,28 @@ bool WebContentsImpl::CompletedFirstVisuallyNonEmptyPaint() {
}

void WebContentsImpl::OnDidDownloadImage(
+ base::WeakPtr<RenderFrameHostImpl> rfh,
ImageDownloadCallback callback,
int id,
const GURL& image_url,
int32_t http_status_code,
const std::vector<SkBitmap>& images,
const std::vector<gfx::Size>& original_image_sizes) {
+
+ // 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 14586316aedab1e99f1b700ecc0e092c86414ee3..ffd53b3dc3ef0553711106f302e9f69b76b49081 100644
--- a/content/browser/web_contents/web_contents_impl.h
+++ b/content/browser/web_contents/web_contents_impl.h
@@ -1403,7 +1403,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 ca7651246f812259e43d5dbc429ae4a95ed60e94..a4572de2ee43ef0c2bb3a785f29974ebb8c1362c 100644
--- a/content/public/browser/web_contents.h
+++ b/content/public/browser/web_contents.h
@@ -896,8 +896,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 fb0fc97d689c8c6b1f6f9fa27d250e0438550725..740c6c01318a1d9d2914ddef9949db9ced559d14 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);
}
@@ -0,0 +1,30 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Andrey Belenko <andrey.belenko@gmail.com>
Date: Wed, 19 May 2021 14:17:10 +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 162d2e2991aa34c8fc6acb14f8cbad5af27b12b9..e7b5ea2103bdca2faebac1d8d0a86b42418eed6b 100644
--- a/media/base/media_switches.cc
+++ b/media/base/media_switches.cc
@@ -676,7 +676,7 @@ const base::Feature kMediaFeedsBackgroundFetching{

// 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};

// Send events to devtools rather than to chrome://media-internals
const base::Feature kMediaInspectorLogging{"MediaInspectorLogging",