New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 740603 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug-Security



Sign in to add a comment

Security: heap-buffer-overflow in gpu::gles2::GLES2Implementation::ReadPixels

Reported by tk.chrom...@googlemail.com, Jul 10 2017

Issue description

VULNERABILITY DETAILS

There is a heap-based buffer overflow (memory write) in gpu::gles2::GLES2Implementation::ReadPixels.

The attached testcase crashes the latest ASAN build of Chrome on Windows 10 as follows (the complete stack trace can be found in asan.txt):

=================================================================
==2556==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x23375a40 at pc 0x015500cc bp 0x0096a354 sp 0x0096a344
WRITE of size 1000000 at 0x23375a40 thread T0
    #0 0x15500e6 in __asan_memcpy e:\b\build\slave\win_upload_clang\build\src\third_party\llvm\projects\compiler-rt\lib\asan\asan_interceptors.cc:462
    #1 0x14ceaf52 in gpu::gles2::GLES2Implementation::ReadPixels(int,int,int,int,unsigned int,unsigned int,void *) C:\b\c\b\win_asan_release\src\gpu\command_buffer\client\gles2_implementation.cc:4158:7
..

The arguments to memcpy are as follows (see WinDbg_Chrome_Stable.txt for more details):
  - src  : canvas pixel (framebuffer) data (user-controlled)
  - dest : heap-based buffer (pixels array) + unchecked offset (user-controlled)
  - count: canvas width * canvas height * 4 (user-controlled)

This bug is obviously exploitable for remote code execution.
  
To reliably trigger a crash, the width and height of the canvas are set to the value 500. This results in a value of 1000000 (500 * 500 * 4) bytes to copy (see the second line of the ASAN output 'WRITE of size 1000000 ..').

Relevant source code snippets (see the comments inline marked with ###):

See https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:

..
1056 bool DrawingBuffer::PaintRenderingResultsToImageData(
1057     int& width,
1058     int& height,
1059     SourceDrawingBuffer source_buffer,
1060     WTF::ArrayBufferContents& contents) {
1061   ScopedStateRestorer scoped_state_restorer(this);
1062
1063   DCHECK(!premultiplied_alpha_);
1064   width = Size().Width();
1065   height = Size().Height();
1066
1067   CheckedNumeric<int> data_size = 4;
1068   data_size *= width;
1069   data_size *= height;
1070   if (!data_size.IsValid())
1071     return false;
1072
1073   WTF::ArrayBufferContents pixels(width * height, 4,   ### allocation of the heap-based destination buffer    
1074                                   WTF::ArrayBufferContents::kNotShared,
1075                                   WTF::ArrayBufferContents::kDontInitialize);
..

See https://cs.chromium.org/chromium/src/gpu/command_buffer/client/gles2_implementation.cc:

..
4001 void GLES2Implementation::ReadPixels(
4002    GLint xoffset, GLint yoffset, GLsizei width, GLsizei height, GLenum format,
4003    GLenum type, void* pixels) {
..
4106  // Advance pixels pointer past the skip rows and skip pixels
4107  dest += skip_size;   ### heap-based destination buffer + unchecked user-controlled offset
..
4153    if (padded_row_size == unpadded_row_size &&
4154       (pack_row_length_ == 0 || pack_row_length_ == width) &&
4155       result->row_length == width && result->num_rows == num_rows) {
4156     // The pixels are tightly packed.
4157     uint32_t copy_size = unpadded_row_size * num_rows;
4158     memcpy(dest, src, copy_size);   ### heap buffer overflow
..


VERSION

Tested on: 

- Chrome Stable Version 59.0.3071.115 (64-Bit, Windows 10)
- asan-win32-release-485195


REPRODUCTION CASE

Provided files:

- testcase.html            -- Minimal testcase to trigger the issue.
- asan.txt                 -- Detailed ASAN output (Windows 10).
- WinDbg_Chrome_Stable.txt -- Further debugging information (Chrome Stable on Windows 10).


FOR CRASHES, PLEASE INCLUDE THE FOLLOWING ADDITIONAL INFORMATION

Type of crash: tab
 
testcase.html
623 bytes View Download
asan.txt
7.6 KB View Download
WinDbg_Chrome_Stable.txt
12.2 KB View Download
Project Member

Comment 1 by ClusterFuzz, Jul 10 2017

ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=5980605807591424.
Project Member

Comment 2 by ClusterFuzz, Jul 10 2017

ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=6622678185410560.

Comment 3 by raymes@chromium.org, Jul 11 2017

Cc: piman@chromium.org
Components: Internals>GPU>Internals
Labels: Security_Severity-High Security_Impact-Stable OS-Windows Pri-1
Owner: jbau...@chromium.org
Status: Assigned (was: Unconfirmed)
jbauman: could you please help triage? 
Cc: jbau...@chromium.org
Owner: zmo@chromium.org
zmo@, I think the implementation of pack_skip_rows was added in https://chromium.googlesource.com/chromium/src/+/c6c114178c562feeddfc4d41a33b9999698a4144
Project Member

Comment 5 by sheriffbot@chromium.org, Jul 11 2017

Labels: M-59

Comment 6 by piman@chromium.org, Jul 11 2017

Cc: kbr@chromium.org
We need to reset pack parameters before ReadPixels. For example we currently reset GL_PACK_ALIGNMENT on https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp?l=1110 . But there are more such parameters in ES3/WebGL2 contexts, namely GL_PACK_ROW_LENGTH, GL_PACK_SKIP_ROWS, GL_PACK_SKIP_PIXELS.

I wonder if similar things might be true when uploading textures from e.g. images (GL_UNPACK_* states).

Comment 7 by piman@chromium.org, Jul 11 2017

Oh, GL_PIXEL_PACK_BUFFER needs to be reset too...

Comment 8 by zmo@chromium.org, Jul 12 2017

Status: Started (was: Assigned)

Comment 9 by zmo@chromium.org, Jul 13 2017

Conformance test is added in https://github.com/KhronosGroup/WebGL/pull/2457/

Chromium side fix is in https://chromium-review.googlesource.com/c/570840/
Project Member

Comment 10 by bugdroid1@chromium.org, Jul 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f6ac1dba5e36f338a490752a2cbef3339096d9fe

commit f6ac1dba5e36f338a490752a2cbef3339096d9fe
Author: Zhenyao Mo <zmo@chromium.org>
Date: Thu Jul 13 23:08:53 2017

Reset ES3 pixel pack parameters and PIXEL_PACK_BUFFER binding in DrawingBuffer before ReadPixels() and recover them later.

BUG= 740603 
TEST=new conformance test
R=kbr@chromium.org,piman@chromium.org

Change-Id: I3ea54c6cc34f34e249f7c8b9f792d93c5e1958f4
Reviewed-on: https://chromium-review.googlesource.com/570840
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Commit-Queue: Zhenyao Mo <zmo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486518}
[modify] https://crrev.com/f6ac1dba5e36f338a490752a2cbef3339096d9fe/third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
[modify] https://crrev.com/f6ac1dba5e36f338a490752a2cbef3339096d9fe/third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h
[modify] https://crrev.com/f6ac1dba5e36f338a490752a2cbef3339096d9fe/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
[modify] https://crrev.com/f6ac1dba5e36f338a490752a2cbef3339096d9fe/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h
[modify] https://crrev.com/f6ac1dba5e36f338a490752a2cbef3339096d9fe/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
[modify] https://crrev.com/f6ac1dba5e36f338a490752a2cbef3339096d9fe/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h
[modify] https://crrev.com/f6ac1dba5e36f338a490752a2cbef3339096d9fe/third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTestHelpers.h

Comment 11 by zmo@chromium.org, Jul 14 2017

Labels: -M-59 M-60 Merge-Request-60
Status: Fixed (was: Started)
M59 is too late to merge in, but I think it's worth merging back to M60.
Project Member

Comment 12 by sheriffbot@chromium.org, Jul 14 2017

Labels: -Merge-Request-60 Hotlist-Merge-Review Merge-Review-60
This bug requires manual review: We are only 10 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-60 Merge-Rejected-60
Since we are only 10 days away from Stable, the bar for merges to M60 is very high. My recommendation is to wait until M61. I'm rejecting this merge for now, but if you think otherwise please re-apply Merge-Request-60 label with justification for why this should be merged, and if it's a very safe merge.
Cc: awhalley@chromium.org
Project Member

Comment 15 by sheriffbot@chromium.org, Jul 15 2017

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: -M-60 M-61
Labels: reward-topanel
Labels: -reward-topanel reward-unpaid reward-5000
*** Boilerplate reminders! ***
Please do NOT publicly disclose details until a fix has been released to all our users. Early public disclosure may cancel the provisional reward. Also, please be considerate about disclosure when the bug affects a core library that may be used by other products. Please do NOT share this information with third parties who are not directly involved in fixing the bug. Doing so may cancel the provisional reward. Please be honest if you have already disclosed anything publicly or to third parties. Lastly, we understand that some of you are not interested in money. We offer the option to donate your reward to an eligible charity. If you prefer this option, let us know and we will also match your donation - subject to our discretion. Any rewards that are unclaimed after 12 months will be donated to a charity of our choosing.
*********************************
tk.chromium@ - nice one! The VRP panel decided to award $5,000 for this bug!

Also, if this appears in Chrome release notes, how would you like to be credited?
Labels: -reward-unpaid reward-inprocess
Thanks for the award! Please credit me as "Tobias Klein (www.trapkit.de)".
Labels: Release-0-M61
Labels: CVE-2017-5112
Project Member

Comment 24 by sheriffbot@chromium.org, Oct 22 2017

Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: CVE_description-submitted

Sign in to add a comment