From b239f27d53ea2e3fcd7598a50b186c5a9acf45c4 Mon Sep 17 00:00:00 2001 From: "trop[bot]" <37223003+trop[bot]@users.noreply.github.com> Date: Sat, 11 Apr 2026 13:42:22 -0700 Subject: [PATCH] fix: validate OSR frame geometry against shared-memory mapping size (#50940) Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com> Co-authored-by: Sam Attard --- shell/browser/osr/osr_host_display_client.cc | 22 ++++++++++----- shell/browser/osr/osr_video_consumer.cc | 28 ++++++++++++++++---- 2 files changed, 38 insertions(+), 12 deletions(-) diff --git a/shell/browser/osr/osr_host_display_client.cc b/shell/browser/osr/osr_host_display_client.cc index e2c41c8442..9464358174 100644 --- a/shell/browser/osr/osr_host_display_client.cc +++ b/shell/browser/osr/osr_host_display_client.cc @@ -4,6 +4,7 @@ #include "shell/browser/osr/osr_host_display_client.h" +#include #include #include "components/viz/common/resources/shared_image_format_utils.h" @@ -38,26 +39,33 @@ void LayeredWindowUpdater::OnAllocatedSharedMemory( if (!region.IsValid()) return; - // Make sure |pixel_size| is sane. - if (!SharedMemorySizeForSharedImageFormat(viz::SinglePlaneFormat::kRGBA_8888, - pixel_size)) { + // |pixel_size| and |region| arrive from the GPU process. Reject geometry that + // overflows or that claims more pixels than the region can actually back so a + // hostile peer cannot make the canvas read past the shared-memory mapping. + std::optional expected_bytes = SharedMemorySizeForSharedImageFormat( + viz::SinglePlaneFormat::kRGBA_8888, pixel_size); + if (!expected_bytes || region.GetSize() < *expected_bytes) { + DLOG(ERROR) << "Shared memory region too small for pixel_size " + << pixel_size.ToString(); return; } #if defined(WIN32) canvas_ = skia::CreatePlatformCanvasWithSharedSection( pixel_size.width(), pixel_size.height(), false, - region.GetPlatformHandle(), skia::CRASH_ON_FAILURE); + region.GetPlatformHandle(), skia::RETURN_NULL_ON_FAILURE); #else shm_mapping_ = region.Map(); - if (!shm_mapping_.IsValid()) { + if (!shm_mapping_.IsValid() || shm_mapping_.size() < *expected_bytes) { DLOG(ERROR) << "Failed to map shared memory region"; + shm_mapping_ = base::WritableSharedMemoryMapping(); return; } canvas_ = skia::CreatePlatformCanvasWithPixels( pixel_size.width(), pixel_size.height(), false, - static_cast(shm_mapping_.memory()), 0, skia::CRASH_ON_FAILURE); + static_cast(shm_mapping_.memory()), 0, + skia::RETURN_NULL_ON_FAILURE); #endif } @@ -66,7 +74,7 @@ void LayeredWindowUpdater::Draw(const gfx::Rect& damage_rect, SkPixmap pixmap; SkBitmap bitmap; - if (active_ && canvas_->peekPixels(&pixmap)) { + if (active_ && canvas_ && canvas_->peekPixels(&pixmap)) { bitmap.installPixels(pixmap); callback_.Run(damage_rect, bitmap, {}); } diff --git a/shell/browser/osr/osr_video_consumer.cc b/shell/browser/osr/osr_video_consumer.cc index bc21c7840d..0a8edf84bc 100644 --- a/shell/browser/osr/osr_video_consumer.cc +++ b/shell/browser/osr/osr_video_consumer.cc @@ -14,6 +14,7 @@ #include "shell/browser/osr/osr_render_widget_host_view.h" #include "third_party/skia/include/core/SkImageInfo.h" #include "third_party/skia/include/core/SkRegion.h" +#include "ui/gfx/geometry/rect.h" #include "ui/gfx/skbitmap_operations.h" namespace { @@ -167,6 +168,27 @@ void OffScreenVideoConsumer::OnFrameCaptured( return; } + // |content_rect| and |info->coded_size| arrive from the GPU process over + // Mojo. The bitmap below is sized from |content_rect| but backed by the + // shared-memory mapping, so a hostile peer could send a tiny region with an + // oversized |content_rect| and have us read past the mapping. Clamp both the + // geometry and the resulting byte size to the mapping before wrapping it. + const size_t row_bytes = + media::VideoFrame::RowBytes(media::VideoFrame::Plane::kARGB, + info->pixel_format, info->coded_size.width()); + const SkImageInfo image_info = SkImageInfo::MakeN32( + content_rect.width(), content_rect.height(), kPremul_SkAlphaType); + const size_t required_bytes = image_info.computeByteSize(row_bytes); + if (!gfx::Rect(info->coded_size).Contains(content_rect) || + SkImageInfo::ByteSizeOverflowed(required_bytes) || + required_bytes > mapping.size()) { + DLOG(ERROR) << "content_rect " << content_rect.ToString() + << " is not bounded by coded_size " + << info->coded_size.ToString() << " / mapping size " + << mapping.size(); + return; + } + // The SkBitmap's pixels will be marked as immutable, but the installPixels() // API requires a non-const pointer. So, cast away the const. void* const pixels = const_cast(mapping.memory()); @@ -185,11 +207,7 @@ void OffScreenVideoConsumer::OnFrameCaptured( SkBitmap bitmap; bitmap.installPixels( - SkImageInfo::MakeN32(content_rect.width(), content_rect.height(), - kPremul_SkAlphaType), - pixels, - media::VideoFrame::RowBytes(media::VideoFrame::Plane::kARGB, - info->pixel_format, info->coded_size.width()), + image_info, pixels, row_bytes, [](void* addr, void* context) { delete static_cast(context); },