Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Starred by 0 users
Status: Fixed
Owner:
Closed: May 2015
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security



Sign in to add a comment
UNKNOWN in SkReader32::readString
Project Member Reported by clusterf...@chromium.org, May 11 2015 Back to list
Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5652598367977472

Uploader: mbarbella@google.com
Job Type: Linux_asan_filter_fuzz_stub_32bit

Crash Type: UNKNOWN
Crash Address: 0x26560629
Crash State:
  SkReader32::readString
  SkPicturePlayback::handleOp
  SkPicturePlayback::draw
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_filter_fuzz_stub_32bit&range=321145:321361

Minimized Testcase (1.12 Kb): https://cluster-fuzz.appspot.com/download/AMIfv97Zl-NFe4CsPe5inlV-0kO_DmAyDYtwy0dTsWd2RldDYry9pIaM2Y-Udxcpz8jJ6uCVtQQQMvjkWQggHZUda80K1o0TXWYdvdK_u4T7NzZhC5xTLSsiP7nDiNvvP_2l8oTQND8sB6s0RjiUeFSjLZqSzEsLtQ

Filer: mbarbella
 
Cc: reed@chromium.org mbarbe...@chromium.org sugoi@chromium.org senorblanco@chromium.org
Labels: reward-topanel
Bulk edit: I'm starting to look at some of the crashes from the batch of test cases we got now, but could use help with triage.
Cc: cloudfuz...@gmail.com
Project Member Comment 3 by clusterf...@chromium.org, May 12 2015
Labels: M-43 Pri-1
Project Member Comment 4 by clusterf...@chromium.org, May 12 2015
Labels: ReleaseBlock-Stable
Project Member Comment 5 by clusterf...@chromium.org, May 15 2015
Labels: -Security_Impact-Beta Security_Impact-Stable Missing_Owner-1
Project Member Comment 6 by clusterf...@chromium.org, May 18 2015
Labels: -Missing_Owner-1 Missing_Owner-2
Comment 7 by och...@chromium.org, May 18 2015
The issue here appears to be an unchecked string size (in this case):

readString is called twice in SkPicturePlayback::handleOp for COMMENTs:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/src/core/SkPicturePlayback.cpp&sq=package:chromium&type=cs&l=211

Inside readString, the length is read through a readInt and used is passed to ->skip(). This skip size is not checked in Release builds (potentially leading to fCurr >= fStop), and the subsequent readString will attempt to read invalid data. 

This could seems like a serious arbitrary (relative) read as the result of the string read (assuming we didn't crash) is added as a comment to the canvas (I'm assuming this can be read by an attacker).

sugoi@, reed@, senorblanco@, do any of you want to contribute or suggest a patch for this? There may be other similar instances so the real fix might be somewhere else rather than in the functions described above?
Comment 8 by och...@chromium.org, May 18 2015
Cc: och...@chromium.org
Comment 9 by reed@chromium.org, May 18 2015
Cc: -reed@chromium.org reed@google.com
After taking another closer look at this, a lot of the SkPicturePlayback/SkReader32 code look very scary. Is there any reason why an SkValidatingReadBuffer isn't used here instead of an SkReader32?
Comment 11 by reed@google.com, May 19 2015
Cc: mtklein@chromium.org
Just a performance concern. We added the Validating variant when we wanted to deserialize drawings for cross-process use, but have been assuming it would be a slow-down for the simpler case. This is something we will measure.
I believe SkPipe and so SkDeferredCanvas remain as performance-critical parts of Skia using SkReader32.  SkValidatingReadBuffer and SkReader32 serve slightly different functions with different APIs, and SkValidatingReadBuffer _is_ in use here, but it'd certainly be fine to use SkValidatingReadBuffer more when deserializing pictures, or to buff SkReader32 up with a validating mode.

This looks like another case where we're deserializing an SkPictureShader.  Don't we shut up all these fuzzer alerts at once by disallowing their serialization in Chrome (i.e. when   SK_DISALLOW_CROSSPROCESS_PICTUREIMAGEFILTERS is set)?
SkPictureShader != SkPictureImageFilter. SK_DISALLOW_CROSSPROCESS_PICTUREIMAGEFILTERS handles the latter. If it's in danger of being deserialized in Chrome (which it seems it is), SkPictureShader should be protected by an #ifdef as well. Or the same one, renamed.

In general, we should be very cautious about what gets added to SkGlobalInitialization_chromium.cpp, since IIUC this controls what is whitelisted for deserialization in Chrome. At a minimum, any new classes added for deserialization should be added to SampleFilterFuzz.cpp.
Uh, sure, SK_DISALLOW_CROSSPROCESS_PICTURES seems fine.

We're still only serializing (and thus fuzzing) image filters right?  No plans to expand to pictures?  If I'm reading this right, the chain we're seeing here is SkRectShaderImageFilter -> SkPictureShader -> SkPicture, so snuffing out SkPictureShader should do the trick.

Speaking of SkGlobalInitialization_chromium.cpp, why is SkPictureImageFilter in there?
Re: #14: When capturing SKPs in Trace Viewer, it's nice to be able to capture them with the SkPictureImageFilters intact. Although they're not currently supported for the accelerated reference filter implementation which serializes to the browser process (due to the security issues above), they are supported for the in-process case (e.g., SVG filters on SVG content).

Doing a compile with the #ifdef disabled allows folks to capture SKPs containing them.
Ah, yeah, that is handy.

Do we get the same effect by moving the #ifdef guard to SkGlobalInitialization_chromium.cpp?

#ifndef SK_DISALLOW_CROSSPROCESS_PICTURES
        SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkPictureImageFilter)
        SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkPictureShader)
#endif
I don't think it's quite the same, since the serialization/deserialization still "works" with the disable on, it just writes out an SkPictureImageFilter with an empty SkPicture (doesn't traverse into the SkPicture) and renders black for that content. If we removed it from the initialization, it might fail at [de]serialization time and [read]write a NULL into the stream, causing other unpleasantness. To be honest, I'm not certain exactly what the result would be.
Gotcha.  Sounds like the best plan is to just guard SkPictureShader in the same way we do SkPicutreImageFilter today.
This sounds like a fairly simple patch if I'm understanding this correctly, and should fix quite a few high severity bugs we have right now. 

mtklein@, Could you please make this a priority if that's possible?


Project Member Comment 20 by bugdroid1@chromium.org, May 20 2015
The following revision refers to this bug:
  https://skia.googlesource.com/skia.git/+/76be9c8dc0e5306ef81c2987848088cdec7ccd3f

commit 76be9c8dc0e5306ef81c2987848088cdec7ccd3f
Author: mtklein <mtklein@chromium.org>
Date: Wed May 20 19:05:15 2015

Don't serialize SkPictures in SkPictureShaders when in untrusted mode.

This requires we "first" add a has-picture bool to SkPictureShader serialized format.

BUG= chromium:486947 , billions and billions of others.

Review URL: https://codereview.chromium.org/1151663002

[modify] http://crrev.com/76be9c8dc0e5306ef81c2987848088cdec7ccd3f/include/core/SkPicture.h
[modify] http://crrev.com/76be9c8dc0e5306ef81c2987848088cdec7ccd3f/src/core/SkPictureShader.cpp
[modify] http://crrev.com/76be9c8dc0e5306ef81c2987848088cdec7ccd3f/src/core/SkReadBuffer.h

Owner: mtklein@chromium.org
Status: Fixed
Thanks!
Labels: Merge-TBD
Is there a merge required here?
So continuing to use this bug as a proxy for all other bugs, I believe a merge is required for these bugs if and only if we possibly can actually try to send an SkRectShaderImageFilter wrapping an SkPictureShader from a renderer to the browser process.  If this is only produceable by the fuzzer, I don't think there's any need to merge anything.  That is, I see .fil files representing these tests cases; are they representable as web pages?  I don't personally know how to answer these questions.


clusterfuzz will go through and confirm that these failures cease, right?  As the author of the fix, I'd feel warmer and fuzzier if the bot can confirm the fix.
Yes CF will verify them in ~1 day.
The danger is that a compromised renderer could construct a malicious stream as above (SkPictureShader inside an SkRectShaderImageFilter) and mess with the browser process.
Ah, yeah, I guess from that perspective the answer to "can we possibly actually try to send an ... to the browser process" is yes for all "...".  A compromised renderer can do anything.

Sounds like this wants to be merged to all branches all the way back to when we started sending image filters to the browser process, right?
Well, back to when SkPictureShader was added, since that was more recent IIRC. But security team will tell us where they want the merges.

We should probably add a comment to src/ports/SkGlobalInitialization_chromium.cpp asking folks to add a change to SampleFilterFuzz when they add a new class to be serialized. Then we would have caught this sooner. (Unless such a comment draws too much attention to sensitive code. I don't know what the best practice is here.)
It's a very good idea to add the comment mentioned in c#27.
Project Member Comment 29 by clusterf...@chromium.org, May 20 2015
Labels: -Restrict-View-SecurityTeam Merge-Triage Restrict-View-SecurityNotify M-44
Adding Merge-Triage label for tracking purposes.

Once your fix had sufficient bake time (on canary, dev as appropriate), please nominate your fix for merge by adding the Merge-Requested label.

When your merge is approved by the release manager, please start merging with higher milestone label first. Make sure to re-request merge for every milestone in the label list. You can get branch information on omahaproxy.appspot.com.

- Your friendly ClusterFuzz
Project Member Comment 30 by clusterf...@chromium.org, May 21 2015
ClusterFuzz has detected this issue as fixed in range 330758:330903.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5652598367977472

Uploader: mbarbella@google.com
Job Type: Linux_asan_filter_fuzz_stub_32bit

Crash Type: UNKNOWN
Crash Address: 0x26560629
Crash State:
  SkReader32::readString
  SkPicturePlayback::handleOp
  SkPicturePlayback::draw
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_filter_fuzz_stub_32bit&range=321145:321361
Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_filter_fuzz_stub_32bit&range=330758:330903

Minimized Testcase (1.12 Kb): https://cluster-fuzz.appspot.com/download/AMIfv97Zl-NFe4CsPe5inlV-0kO_DmAyDYtwy0dTsWd2RldDYry9pIaM2Y-Udxcpz8jJ6uCVtQQQMvjkWQggHZUda80K1o0TXWYdvdK_u4T7NzZhC5xTLSsiP7nDiNvvP_2l8oTQND8sB6s0RjiUeFSjLZqSzEsLtQ

If you suspect that the result above is incorrect,try re-doing that job on the test case report page.
Cc: hcm@chromium.org
Labels: -Missing_Owner-2 -Merge-TBD -Merge-Triage Merge-Request-44
Labels: -Merge-Request-44 Merge-Approved-44 Hotlist-Merge-Approved
Approved for M44 (branch: 2403)
Project Member Comment 34 by bugdroid1@chromium.org, Jun 16 2015
Please finish merging your CL into M44 asap.  You have one week before stable candidate is built.
Thanks for the reminder.  I just landed a merge into Skia's M44 branch.

https://skia.googlesource.com/skia/+/1dd3830b7254eaea8d710b256f7f040668f02458

(Don't know if bugdroid will pick it up.)
Labels: -Merge-Approved-44 -Hotlist-Merge-Approved merge-merged-2403
Labels: Release-0-M44
Labels: CVE-2015-1280
Labels: -reward-topanel reward-5000 reward-unpaid
cloudfuzzer: $5000 for this report.
Project Member Comment 41 by clusterf...@chromium.org, Aug 26 2015
Labels: -Restrict-View-SecurityNotify
Bulk update: removing view restriction from closed bugs.
Labels: -reward-unpaid reward-inprocess
Labels: -reward-inprocess
Processing via our e-payment system takes ~7 days, but the reward should be on its way to you. Thanks again for your help!
Project Member Comment 44 by sheriffbot@chromium.org, Oct 1 2016
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
Project Member Comment 45 by sheriffbot@chromium.org, Oct 2 2016
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: allpublic
Sign in to add a comment