New issue
Advanced search Search tips
Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 5
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows
Pri: 1
Type: Bug-Security



Sign in to add a comment

Security: Floating-point precision errors in Swiftshader blitting

Project Member Reported by markbrand@google.com, May 31

Issue description

This template is ONLY for reporting security bugs. If you are reporting a
Download Protection Bypass bug, please use the "Security - Download
Protection" template. For all other reports, please use a different
template.

Please READ THIS FAQ before filing a bug: https://chromium.googlesource.com
/chromium/src/+/master/docs/security/faq.md

Please see the following link for instructions on filing security bugs:
https://www.chromium.org/Home/chromium-security/reporting-security-bugs

NOTE: Security bugs are normally made public once a fix has been widely
deployed.

VULNERABILITY DETAILS
There is a bug in the Swiftshader renderer handling blitting between surfaces (Renderer/Blitter.cpp).

For simplicity's sake, we'll consider the case when JITting fails (so ignoring the call to blitReactor) - in practice, the JITted code has the same problem, but it's somewhat more difficult to understand/explain that code.

void Blitter::blit(Surface *source, const SliceRectF &sourceRect, Surface *dest, const SliceRect &destRect, const Blitter::Options& options)
{
  if(dest->getInternalFormat() == FORMAT_NULL)
  {
    return;
  }

  if(blitReactor(source, sourceRect, dest, destRect, options))
  {
    return;
  }

  SliceRectF sRect = sourceRect;
  SliceRect dRect = destRect;

  bool flipX = destRect.x0 > destRect.x1;
  bool flipY = destRect.y0 > destRect.y1;

  if(flipX)
  {
    swap(dRect.x0, dRect.x1);
    swap(sRect.x0, sRect.x1);
  }
  if(flipY)
  {
    swap(dRect.y0, dRect.y1);
    swap(sRect.y0, sRect.y1);
  }

  source->lockInternal((int)sRect.x0, (int)sRect.y0, sRect.slice, sw::LOCK_READONLY, sw::PUBLIC);
  dest->lockInternal(dRect.x0, dRect.y0, dRect.slice, sw::LOCK_WRITEONLY, sw::PUBLIC);

  float w = sRect.width() / dRect.width();
  float h = sRect.height() / dRect.height();

  const float xStart = sRect.x0 + 0.5f * w;
  float y = sRect.y0 + 0.5f * h;
  float x = xStart;

  for(int j = dRect.y0; j < dRect.y1; j++)
  {
    x = xStart;

    for(int i = dRect.x0; i < dRect.x1; i++)
    {
      // FIXME: Support RGBA mask
      dest->copyInternal(source, i, j, x, y, options.filter);

      x += w;
    }

    y += h;
  }

  source->unlockInternal();
  dest->unlockInternal();
}

For context, dest->copyInternal will simply cast x and y to type int, and use them to read the colour at (x, y) from the source surface, then write it to (i, j) in the destination surface. No further bounds checking is performed in this function, since the inputs to it should previously have been checked.

In every calling path, we should have also previously checked that sourceRect and destRect are within the bounds oftheir respective surfaces, so all of these accesses should be safe.

If we look at the method of calculation for w and x however, we can see a potential problem.

float w = sRect.width() / dRect.width();

...
for(int j = dRect.y0; j < dRect.y1; j++)
{
  x = xStart;

  for(int i = dRect.x0; i < dRect.x1; i++)
  {
    ...

    x += w;
  }

...


We're performing repeated additions of floating point values, and in this case the attacker has sufficient control over the input values to arrange matters so that this has interesting results. The example used in the PoC is when the source width is 5828, and the destination width is 8132. Below shows the results of the calculation performed in the code, and a second calculation where a multiplication is used instead of the iterative addition:

0:    1.075012 1.075012
1:    1.791687 1.791687
...
1000: 717.749878 717.749878   Up to here the precision used the values are still identical
1001: 718.466553 718.466553
...
2046: 1467.391724 1467.391724 At this point, the first significant errors start to occur, but note
2047: 1468.108398 1468.108521 that the "incorrect" result is smaller than the more precise one.
...
2856: 2047.898315 2047.898438
2857: 2048.614990 2048.614990 Here our two computations coincide again, briefly, and from here onwards
2858: 2049.331787 2049.331787 the precision errors consistently favour a larger result than the more
2859: 2050.048584 2050.048340 precise calculation.
...
8129: 5827.567871 5826.924805
8130: 5828.284668 5827.641602
8131: 5829.001465 5828.358398 The last index is now sufficiently different that int conversion results in an oob index.

The result here is that we end up taking our sample values for the last row of the result from one-row-past-the-end of the source buffer.

If we build with ASAN and disable the JIT by commenting out the blitReactor call, we can see this from the ASAN report:

=================================================================
==26029==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7f808f3c205c at pc 0x7f809fdfcd34 bp 0x7fff0b816250 sp 0x7fff0b816248
READ of size 4 at 0x7f808f3c205c thread T0 (chrome)
==26029==WARNING: invalid path to external symbolizer!
==26029==WARNING: Failed to use and restart external symbolizer!
    #0 0x7f809fdfcd33 in sw::Surface::Buffer::read(void*) const /ssd/chrome/src/out/asan/../../third_party/swiftshader/src/Renderer/Surface.cpp:580:25
    #1 0x7f809fdc088a in sw::Blitter::blit(sw::Surface*, sw::SliceRectT<float> const&, sw::Surface*, sw::SliceRectT<int> const&, sw::Blitter::Options const&) /ssd/chrome/src/out/asan/../../third_party/swiftshader/src/Renderer/Blitter.cpp:187:11

0x7f808f3c205c is located 0 bytes to the right of 135862364-byte region [0x7f8087230800,0x7f808f3c205c)
allocated by thread T0 (chrome) here:
    #0 0x55f41d3e45c2 in operator new[](unsigned long) _asan_rtl_:3
    #1 0x7f809ff6b82a in allocateRaw /ssd/chrome/src/out/asan/../../third_party/swiftshader/src/Common/Memory.cpp:68:25
    #2 0x7f809ff6b82a in sw::allocate(unsigned long, unsigned long) /ssd/chrome/src/out/asan/../../third_party/swiftshader/src/Common/Memory.cpp:85:0

SUMMARY: AddressSanitizer: heap-buffer-overflow (/ssd/chrome/src/out/asan/swiftshader/libGLESv2.so+0x4ddd33)
Shadow bytes around the buggy address:
  0x0ff091e703b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ff091e703c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ff091e703d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ff091e703e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ff091e703f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0ff091e70400: 00 00 00 00 00 00 00 00 00 00 00[04]fa fa fa fa
  0x0ff091e70410: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0ff091e70420: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0ff091e70430: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0ff091e70440: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0ff091e70450: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==26029==ABORTING

The bug is however also present in the JIT version of the function, and the attached PoC when run on a normal chrome build (with --disable-gpu, or forcing swiftshader as per my previously reported issue) should output leaked memory from the gpu process to the console. The very rough heap-spray implemented makes it likely the leaked memory will contain pointers to the chrome binary, to libswiftshader, and to the heap.

This bug is subject to a 90 day disclosure deadline. After 90 days elapse
or a patch has been made broadly available (whichever is earlier), the bug
report will become visible to the public.

VERSION
Chrome Version: 66.0.3359.181 (Developer Build) (64-bit)
Operating System: [linux]

REPRODUCTION CASE
Please include a demonstration of the security bug, such as an attached
HTML or binary file that reproduces the bug when loaded in Chrome. PLEASE
make the file as small as possible and remove any content not required to
demonstrate the bug.

FOR CRASHES, PLEASE INCLUDE THE FOLLOWING ADDITIONAL INFORMATION
Type of crash: [tab, browser, etc.]
Crash State: [see link above: stack trace *with symbols*, registers,
exception record]
Client ID (if relevant): [see link above]

 
blit.html
3.1 KB View Download
Components: Internals>GPU>SwiftShader
Owner: capn@chromium.org
Status: Assigned (was: Unconfirmed)
capn: Can you PTAL?
(Looks similar to  bug 825545 )
Cc: sugoi@chromium.org
Labels: Pri-1
Status: Started (was: Assigned)
Nice find, Mark! This is concerning to us from a filtering accuracy perspective as well.

WIP: https://swiftshader-review.googlesource.com/c/SwiftShader/+/19148
Please log into swiftshader-review.googlesource.com so I can add you to the review.
Labels: Security_Severity-High Security_Impact-Stable
I assume this impacts stable. Please readjust if incorrect.
Project Member

Comment 5 by bugdroid1@chromium.org, Jun 2

The following revision refers to this bug:
  https://swiftshader.googlesource.com/SwiftShader.git/+/ae3d875253f4aef9b5df10887631c5f0453c588e

commit ae3d875253f4aef9b5df10887631c5f0453c588e
Author: Nicolas Capens <capn@google.com>
Date: Sat Jun 02 01:11:57 2018

Prevent floating-point error accumulation during blitting.

The numeric imprecision can accumulate quickly and cause visible
sampling errors.

 Bug chromium:848238 

Change-Id: Ie41c10a0462e5b5e5c3ed5f7329ece00dab3f7f9
Reviewed-on: https://swiftshader-review.googlesource.com/19148
Reviewed-by: Alexis Hétu <sugoi@google.com>
Tested-by: Nicolas Capens <nicolascapens@google.com>

[modify] https://crrev.com/ae3d875253f4aef9b5df10887631c5f0453c588e/src/Common/Version.h
[modify] https://crrev.com/ae3d875253f4aef9b5df10887631c5f0453c588e/src/Renderer/Blitter.cpp

Project Member

Comment 6 by sheriffbot@chromium.org, Jun 2

Labels: Target-67 M-67
Project Member

Comment 7 by bugdroid1@chromium.org, Jun 4

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

commit c1d1d91f0d74adaef3aff961a313a8b9c6a5aedf
Author: Alexis Hetu <sugoi@google.com>
Date: Mon Jun 04 23:24:57 2018

Roll SwiftShader 419e8a7..57776df

https://swiftshader.googlesource.com/SwiftShader.git/+log/419e8a7..57776df

BUG= chromium:847094   chromium:848238 

TBR=kbr@chromium.org

TEST=bots

CQ_INCLUDE_TRYBOTS=luci.chromium.try:win_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_chromium_cfi_rel_ng;luci.chromium.try:android_optional_gpu_tests_rel

Change-Id: Ib1ad80065f888a0aab0f9a4b13143967b0ca5a57
Reviewed-on: https://chromium-review.googlesource.com/1085483
Commit-Queue: Alexis Hétu <sugoi@chromium.org>
Reviewed-by: Alexis Hétu <sugoi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564288}
[modify] https://crrev.com/c1d1d91f0d74adaef3aff961a313a8b9c6a5aedf/DEPS
[delete] https://crrev.com/00152a250f55c4fa37bb9ee3fe2ed7344f9648d8/third_party/WebKit/LayoutTests/platform/mac-mac10.12/virtual/video-surface-layer/media/controls-without-preload-expected.png
[delete] https://crrev.com/00152a250f55c4fa37bb9ee3fe2ed7344f9648d8/third_party/WebKit/LayoutTests/platform/mac/virtual/video-surface-layer/media/controls-without-preload-expected.png
[modify] https://crrev.com/c1d1d91f0d74adaef3aff961a313a8b9c6a5aedf/third_party/WebKit/LayoutTests/virtual/gpu-rasterization/images/rgb-png-with-cmyk-color-profile-expected.png

Status: Fixed (was: Started)
Project Member

Comment 9 by sheriffbot@chromium.org, Jun 5

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member

Comment 10 by sheriffbot@chromium.org, Jun 8

Labels: Merge-Request-68
Project Member

Comment 11 by sheriffbot@chromium.org, Jun 8

Labels: -Merge-Request-68 Hotlist-Merge-Review Merge-Review-68
This bug requires manual review: DEPS changes referenced in bugdroid comments.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Can you please specify which OS this impacts?
Labels: OS-Linux OS-Windows
I'm not sure if a merge is necessary? If I understand correctly other vulnerabilities would have to exist and be exploited before this can be used for a successful attack. This bug has existed for a long time, and while it's certainly great to have it fixed, it doesn't seem critical roll it out sooner.
Labels: -Target-67 -Merge-Review-68 Merge-Rejected-68 Target-69
Thanks for more details. Per #13, let's wait until M69 for the fix then. 
Labels: -M-67 M-69
Labels: Release-0-M69
Labels: CVE-2018-16069 CVE_description-missing
Project Member

Comment 18 by sheriffbot@chromium.org, Sep 11

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

Sign in to add a comment