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

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2
Cc:
Components:
EstimatedDays: 1
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug-Security



Sign in to add a comment

Security: global-buffer-overflow in SkBitmap IPC Deserialization

Reported by nedwilli...@gmail.com, Oct 30 2017

Issue description

VULNERABILITY DETAILS
ParamTraits<SkBitmap>::Read in gfx_skia_param_traits.cc handles
deserialization of SkBitmap parameters for old-style IPC messages.
When a compromised renderer sends a message with an oversized
color type, an (arbitrary) out of bounds read occurs.

The problem is here:
```
bool SK_WARN_UNUSED_RESULT tryAllocPixels(const SkImageInfo& info) {
    return this->tryAllocPixels(info, info.minRowBytes());
}
```

The real `this->tryAllocPixels` does validation on the color and alpha
types, but the call `info.minRowBytes()` leads to a call to
`this->bytesPerPixel` which indexes into a global buffer using the
renderer-provided value.

A full patch should fix this at both the Skia layer and the Chromium
layer, but for simplicity the attached patch only adds validation at
the IPC deserialization level in Chromium code.

VERSION
Chrome Version: 62 Stable
Operating System: All

REPRODUCTION CASE
Apply renderer.patch and open Chrome.

FOR CRASHES, PLEASE INCLUDE THE FOLLOWING ADDITIONAL INFORMATION
Type of crash: Browser
Crash State: See crash.log

 
crash.log
7.7 KB View Download
fix.patch
1.7 KB Download
renderer.patch
1.7 KB Download
Components: Internals>Skia Internals>Core
EstimatedDays: 1

Comment 3 by hcm@chromium.org, Oct 30 2017

Cc: danakj@chromium.org hcm@chromium.org markdittmer@chromium.org reed@google.com
Labels: Security_Severity-Medium Security_Impact-Stable OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows
Project Member

Comment 5 by sheriffbot@chromium.org, Oct 31 2017

Labels: M-63
Project Member

Comment 6 by sheriffbot@chromium.org, Oct 31 2017

Labels: Pri-1

Comment 7 by vakh@chromium.org, Nov 2 2017

Cc: -reed@google.com
Owner: reed@google.com
Status: Assigned (was: Unconfirmed)
reed@ -- please help triage and find the right owner for this issue. thanks.
Project Member

Comment 8 by sheriffbot@chromium.org, Nov 13 2017

reed: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

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

Comment 9 by reed@google.com, Nov 16 2017

Cc: reed@google.com
Owner: markdittmer@chromium.org
Funny there is a patch, but not a CL.

Having glanced at the patch, I'm not sure if this is the best approach. We seems to be validating the data coming from SkBitmap, but not validating it when it came directly out of deserialization... If this patch fixes things, how did the SkBitmap get passed bad values?
Project Member

Comment 10 by sheriffbot@chromium.org, Nov 28 2017

markdittmer: Uh oh! This issue still open and hasn't been updated in the last 29 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Does this line not prevent the aforementioned bad indexing?

https://cs.chromium.org/chromium/src/third_party/skia/include/core/SkImageInfo.h?gsn=SkBitmap&l=103

Comment 12 by reed@google.com, Nov 28 2017

SkASSERT only fires in debug builds. I think the gfx code that has decided to deserialize a bitmap needs to validate its parameters before creating the skia object.
re comment 9: This patch addresses the bug at the IPC deserialization layer, making sure the input data is well-formed before passing it to Skia. Is there another patch you had in mind here? Also I didn't know how to CL before, but I've since made one. I can open a CL if that's easier for you.
nedwilliamson@gmail.com a CL would be great!

My only feedback on the code as-is would be this: could the unit test be based on serializing then attempting to deserialize an actual SkBitmap instance that is otherwise well formed, but has only the color or alpha perturbed? Two cases (one for each of alpha and color) would also be ideal.

I'm happy to review the CL, but I don't have the OWNERS power to give final approval.

Thanks for catching and suggesting a fix for this!
CL is up at https://chromium-review.googlesource.com/c/chromium/src/+/809294

Thanks for the suggestions!
Friendly ping on this. Mark: how does the CL look at this point?
Project Member

Comment 17 by sheriffbot@chromium.org, Jan 25 2018

Labels: -M-63 M-64
Hey guys, it seems Mark hasn't visited in a couple weeks and we're approaching Google's own 90 day deadline for security bugs. Should I reassign to another owner? The CL has been open for a couple months now and it's important that we get this fixed since it crosses the sandbox boundary.

If we can't get this fixed (I'm willing to help over the weekend if necessary) by 90 days I will have to publicly disclose it: https://googleprojectzero.blogspot.com/2015/02/feedback-and-data-driven-updates-to.html
Owner: sky@chromium.org
sky could review the paramtraits stuff
Thanks for reassigning. Sky's suggested fix sounds much better than the existing one. I can try to attempt a fix over the weekend, and I can bump the disclosure 14 days as long as we keep moving forward :)
For the future, you can find a reviewer by looking at OWNERS files in the source tree, by walking up the tree from the modified files.
Labels: OS-Fuchsia
This should affect Fuchsia too, right? https://skia.org/dev/flutter
Fuschia would be affected, yes.

nedwilliamson: are you still planning on updating the CL for this? Otherwise, I'll adopt it. I'd like to get this fixed for M64.
dcheng: feel free to start from my initial CL or start a new one. I think rewriting the serialization field-by-field is the way to go here, as was noted on the CL. I agree we want to get this fixed and unfortunately I work full time somewhere else. ;)
Cc: sky@chromium.org
Owner: dcheng@chromium.org
Status: Started (was: Assigned)
Cc: mtklein@chromium.org bsalomon@chromium.org
Labels: Merge-Request-65
Status: Fixed (was: Started)
https://skia-review.googlesource.com/c/skia/+/99781 appears to have fixed this already, by getting rid of the table and using a switch. So we could merge that patch...

(Normally using memcpy like this is also an info leak. I think we lucked here, in that the struct happens to have no padding)

For now, I'll request that this be merged into M-65. If someone from Skia can help with the merge if it's approved, that'd be appreciated.
Project Member

Comment 27 by bugdroid1@chromium.org, Feb 3

Labels: merge-merged-m65
The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/528018f579c3647a5acd6a27e83c47192ea65baf

commit 528018f579c3647a5acd6a27e83c47192ea65baf
Author: Brian Salomon <bsalomon@google.com>
Date: Sat Feb 03 00:24:40 2018

Add kRGBX_8888, kRGBA_1010102, and kRGBX_1010102 color types. Unused for now.

BUG=  skia:7533 

Cherry pick to M65
BUG =  chromium:779428 

No-Tree-Checks: true
No-Try: true
No-Presubmit: true
Change-Id: I4b3f6b827fd833ba2d07895884d2abc9a3132366
Reviewed-On: https://skia-review.googlesource.com/99781
Reviewed-By: Mike Klein <mtklein@chromium.org>
Commit-Queue: Brian Salomon <bsalomon@google.com>
Reviewed-on: https://skia-review.googlesource.com/103200
Reviewed-by: Brian Salomon <bsalomon@google.com>

[modify] https://crrev.com/528018f579c3647a5acd6a27e83c47192ea65baf/src/image/SkImage_Lazy.cpp
[modify] https://crrev.com/528018f579c3647a5acd6a27e83c47192ea65baf/tools/debugger/SkObjectParser.cpp
[modify] https://crrev.com/528018f579c3647a5acd6a27e83c47192ea65baf/src/core/SkImageInfo.cpp
[modify] https://crrev.com/528018f579c3647a5acd6a27e83c47192ea65baf/src/gpu/gl/GrGLCaps.cpp
[modify] https://crrev.com/528018f579c3647a5acd6a27e83c47192ea65baf/src/gpu/SkGr.cpp
[modify] https://crrev.com/528018f579c3647a5acd6a27e83c47192ea65baf/src/codec/SkWebpCodec.cpp
[modify] https://crrev.com/528018f579c3647a5acd6a27e83c47192ea65baf/gm/bitmapcopy.cpp
[modify] https://crrev.com/528018f579c3647a5acd6a27e83c47192ea65baf/include/core/SkImageInfo.h
[modify] https://crrev.com/528018f579c3647a5acd6a27e83c47192ea65baf/src/gpu/vk/GrVkCaps.cpp

Awesome! A fix at the Skia layer is much better since it protects future clients.
Cc: cma...@chromium.org amineer@chromium.org
Cl listed #27 got merged to M65 without approval and it is breaking M65 builds (Pls see bug 808763 for more details). Next time, pls wait for approval. Thank you.
Project Member

Comment 30 by bugdroid1@chromium.org, Feb 3

The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/73137ff6ba51b3f329d0da9e50a312c4cedee32c

commit 73137ff6ba51b3f329d0da9e50a312c4cedee32c
Author: Brian Salomon <bsalomon@google.com>
Date: Sat Feb 03 16:36:11 2018

Revert "Add kRGBX_8888, kRGBA_1010102, and kRGBX_1010102 color types. Unused for now."

This reverts commit 528018f579c3647a5acd6a27e83c47192ea65baf.

Reason for revert: Breaks branch build w/out corresponding Chrome change.

Original change's description:
> Add kRGBX_8888, kRGBA_1010102, and kRGBX_1010102 color types. Unused for now.
> 
> BUG=  skia:7533 
> 
> Cherry pick to M65
> BUG =  chromium:779428 
> 
> No-Tree-Checks: true
> No-Try: true
> No-Presubmit: true
> Change-Id: I4b3f6b827fd833ba2d07895884d2abc9a3132366
> Reviewed-On: https://skia-review.googlesource.com/99781
> Reviewed-By: Mike Klein <mtklein@chromium.org>
> Commit-Queue: Brian Salomon <bsalomon@google.com>
> Reviewed-on: https://skia-review.googlesource.com/103200
> Reviewed-by: Brian Salomon <bsalomon@google.com>

TBR=mtklein@chromium.org,bsalomon@google.com

Change-Id: I3a8fc24ae48519b1da21e54c5e85899e8577f288
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  skia:7533 
Reviewed-on: https://skia-review.googlesource.com/103300
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>

[modify] https://crrev.com/73137ff6ba51b3f329d0da9e50a312c4cedee32c/src/image/SkImage_Lazy.cpp
[modify] https://crrev.com/73137ff6ba51b3f329d0da9e50a312c4cedee32c/tools/debugger/SkObjectParser.cpp
[modify] https://crrev.com/73137ff6ba51b3f329d0da9e50a312c4cedee32c/src/core/SkImageInfo.cpp
[modify] https://crrev.com/73137ff6ba51b3f329d0da9e50a312c4cedee32c/src/gpu/gl/GrGLCaps.cpp
[modify] https://crrev.com/73137ff6ba51b3f329d0da9e50a312c4cedee32c/src/gpu/SkGr.cpp
[modify] https://crrev.com/73137ff6ba51b3f329d0da9e50a312c4cedee32c/src/codec/SkWebpCodec.cpp
[modify] https://crrev.com/73137ff6ba51b3f329d0da9e50a312c4cedee32c/gm/bitmapcopy.cpp
[modify] https://crrev.com/73137ff6ba51b3f329d0da9e50a312c4cedee32c/include/core/SkImageInfo.h
[modify] https://crrev.com/73137ff6ba51b3f329d0da9e50a312c4cedee32c/src/gpu/vk/GrVkCaps.cpp

Cc: awhalley@chromium.org
Labels: -merge-merged-m65
Removing "merge-merged-m65" label as CL got reverted from M65 at #30.
Project Member

Comment 32 by sheriffbot@chromium.org, Feb 3

Labels: -Merge-Request-65 Hotlist-Merge-Approved Merge-Approved-65
Your change meets the bar and is auto-approved for M65. Please go ahead and merge the CL to branch 3325 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

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

Comment 33 Deleted

Project Member

Comment 34 by bugdroid1@chromium.org, Feb 5

Labels: -merge-approved-65 merge-merged-3325
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/247c9292e111304b0bf92a507744bce42356ab68

commit 247c9292e111304b0bf92a507744bce42356ab68
Author: Brian Salomon <bsalomon@google.com>
Date: Mon Feb 05 13:40:51 2018

Prepare mojom for new Skia color types.

Bug:  skia:7533 
Change-Id: If19339a4a0aaa8ea655184db054058ea2697c6f3
Reviewed-on: https://chromium-review.googlesource.com/887381
Commit-Queue: Brian Salomon <bsalomon@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Florin Malita <fmalita@chromium.org>
Reviewed-by: Mike Klein <mtklein@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532698}
(cherry picked from commit 8ae77702133e0921f9a6491eb3b6c5f1db67c885)

TBR=bsalomon@google.com

Bug:  779428 
Change-Id: Iaa32246899f64fb4704fa6c7d17ffb1528667cce
Reviewed-on: https://chromium-review.googlesource.com/901366
Reviewed-by: Brian Salomon <bsalomon@chromium.org>
Cr-Commit-Position: refs/branch-heads/3325@{#296}
Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369}
[modify] https://crrev.com/247c9292e111304b0bf92a507744bce42356ab68/skia/public/interfaces/bitmap_skbitmap_struct_traits.cc

Project Member

Comment 35 by bugdroid1@chromium.org, Feb 5

Labels: merge-merged-m65
The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/7f321dad5c6d767f031c41d966f3bbded165b6c4

commit 7f321dad5c6d767f031c41d966f3bbded165b6c4
Author: Brian Salomon <bsalomon@google.com>
Date: Mon Feb 05 14:07:37 2018

Revert "Revert "Add kRGBX_8888, kRGBA_1010102, and kRGBX_1010102 color types. Unused for now.""

This reverts commit 73137ff6ba51b3f329d0da9e50a312c4cedee32c.

No-Tree-Checks: true
No-Try: true
No-Presubmit: true
Bug:  chromium:779428 
Change-Id: I33ab6e3d5be79f3f97e14c50bfb632ca5126756e
Reviewed-on: https://skia-review.googlesource.com/103441
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>

[modify] https://crrev.com/7f321dad5c6d767f031c41d966f3bbded165b6c4/src/image/SkImage_Lazy.cpp
[modify] https://crrev.com/7f321dad5c6d767f031c41d966f3bbded165b6c4/tools/debugger/SkObjectParser.cpp
[modify] https://crrev.com/7f321dad5c6d767f031c41d966f3bbded165b6c4/src/core/SkImageInfo.cpp
[modify] https://crrev.com/7f321dad5c6d767f031c41d966f3bbded165b6c4/src/gpu/gl/GrGLCaps.cpp
[modify] https://crrev.com/7f321dad5c6d767f031c41d966f3bbded165b6c4/src/gpu/SkGr.cpp
[modify] https://crrev.com/7f321dad5c6d767f031c41d966f3bbded165b6c4/src/codec/SkWebpCodec.cpp
[modify] https://crrev.com/7f321dad5c6d767f031c41d966f3bbded165b6c4/gm/bitmapcopy.cpp
[modify] https://crrev.com/7f321dad5c6d767f031c41d966f3bbded165b6c4/include/core/SkImageInfo.h
[modify] https://crrev.com/7f321dad5c6d767f031c41d966f3bbded165b6c4/src/gpu/vk/GrVkCaps.cpp

Project Member

Comment 36 by bugdroid1@chromium.org, Feb 6

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

commit 9fe6e9f89a1c78b8b38e806d35651a15858b053b
Author: Daniel Cheng <dcheng@chromium.org>
Date: Tue Feb 06 01:02:04 2018

Update IPC ParamTraits for SkBitmap to follow best practices.

Using memcpy() to serialize a POD struct is highly discouraged. Just use
the standard IPC param traits macros for doing it.

Bug:  779428 
Change-Id: I48f52c1f5c245ba274d595829ed92e8b3cb41334
Reviewed-on: https://chromium-review.googlesource.com/899649
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534562}
[modify] https://crrev.com/9fe6e9f89a1c78b8b38e806d35651a15858b053b/ui/gfx/ipc/skia/gfx_skia_param_traits.cc
[modify] https://crrev.com/9fe6e9f89a1c78b8b38e806d35651a15858b053b/ui/gfx/ipc/skia/gfx_skia_param_traits.h
[add] https://crrev.com/9fe6e9f89a1c78b8b38e806d35651a15858b053b/ui/gfx/ipc/skia/gfx_skia_param_traits_macros.h

Labels: reward-topanel
Project Member

Comment 38 by sheriffbot@chromium.org, Feb 8

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: -reward-topanel reward-unpaid reward-2000
*** 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.
*********************************
Nice one nedwilliamson@! The VRP panel decided to award $2,000 for this bug, and also wish to thank you for the help on the bug and code review :-)
Labels: -reward-unpaid reward-inprocess
Labels: Release-0-M65
Labels: CVE-2018-6067
Project Member

Comment 44 by sheriffbot@chromium.org, Mar 27

Labels: -M-64 M-65
Labels: CVE_description-missing
Project Member

Comment 46 by sheriffbot@chromium.org, May 12

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