Automatically assigning owner based on suspected regression changelist https://chromium.googlesource.com/chromium/src/+/116159dfe170dcc2cf6f3b2a04a292130f6b79e2 (cc/ipc: Disallow codec backed SkImageFilters on CompositorFrames.).

If this is incorrect, please remove the owner and apply the Test-Predator-Wrong-CLs label.

On removing the check at serialization time, I can see the decode happening in the browser process, which is a serious security bug.

The test case has an FEImage filter. Removing the image here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/svg/graphics/filters/SVGFEImage.cpp?l=48, fixes it. So clearly the filter from SVGFEImage is making its way to a composited layer.

pdr@, since this is SVG, could you take a look? If its not possible to remove this in blink then we can do a decode at serialization time instead of crashing.

Regardless, wouldn't it be best to hook up an SkImageDeserializer on the deserialization side to reject any encoded data? If a renderer is compromised, you can't really rely on a CHECK to save you.

I don't think FEImage can be reasonable changed - making it decode or the serialization code seems like it doesn't matter much. There's one potential benefit I can see to doing it "directly" in Blink, and that would be that then we can rasterize the non-raster image content that would otherwise not work at all.

I wanted to do the deserialization time hook up as well but skia doesn't export SkValidatingReadBuffer/SkReadBuffer. I do agree that a deserialization time CHECK would be more robust.

Actually doing it at serialization isn't that great either, it can stall the compositor thread for a long time. The change I was suggesting in blink was to restrict such filter cases to painted content, instead of having them on composited layers. Sorry but I didn't quite follow the "we can rasterize the non-raster image content that would otherwise not work at all" part.

Stalling the main thread usually isn't too popular either I hear ;-), so that part essentially boils down to choosing which is the lesser of two evils... Preventing the layer from being a composited layer could have some unpleasant side-effects as well (like if you have a filter applied to a video or similar.) This (<feImage>) should be uncommon enough though that, in practice, whichever method we end up with shouldn't matter very much.

As for the "rasterize the non-raster image content", I was getting at that <feImage> can also reference a SVG subtree that will end up producing an SkPicture(ImageFilter), which will be rejected. So if we rasterize in FEImage, we could rasterize that bit too. (Perhaps sharing some code with the box-reflect case.)

Oh yes, I didn't realize that this means we get such cases with picture filters as well, which the serialization code completely ignores right now. And I'm assuming we haven't received any bugs so far? In which case, its uncommon enough that the least complex solution would be better.

At least for image filters, which is the source of this bug, I think doing a decode at serialization sounds fine. Its trivial to do with SkPixelSerializer and skia will cache it, so even for those uncommon cases it wouldn't be too bad. If you think its worth doing a better solution in blink for both these and picture filter cases, could we discuss that as a followup in a separate bug?

That SGTM. Hooking up the reject at deserialization should probably be higher prio than fixing picture filters.

Assigning it back to me for taking care of changing serialization and adding a deserialization check to ignore encoded images.

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

commit 9b078f490e80c0de0c221b7d6dbb5e18fa47e550
Author: Khushal <khushalsagar@chromium.org>
Date: Sat Oct 14 01:06:51 2017

cc/ipc: Ensure images are decoded during serialization.

CompositorFrames can have image filters with encoded images. We
currently assert such cases should never occur resulting in a renderer
crash. This change ensures instead that any encoded image is decoded
and the decoded bitmap is serialized during filter serialization. As an
additional security precaution, the deserialization ensures that if any
encoded image is present in the serialized data, it is rejected during
deserialization.

TBR=reed@google.com
R=palmer@chromium.org

Bug:  772047 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I3916cd22b03a0972684207e4c4e35877fd084841
Reviewed-on: https://chromium-review.googlesource.com/717857
Commit-Queue: Khushal <khushalsagar@chromium.org>
Reviewed-by: Mike Reed <reed@chromium.org>
Reviewed-by: Chris Palmer <palmer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508897}
[modify] https://crrev.com/9b078f490e80c0de0c221b7d6dbb5e18fa47e550/cc/ipc/cc_param_traits.cc
[modify] https://crrev.com/9b078f490e80c0de0c221b7d6dbb5e18fa47e550/skia/ext/skia_utils_base.cc
[modify] https://crrev.com/9b078f490e80c0de0c221b7d6dbb5e18fa47e550/skia/ext/skia_utils_base.h
[modify] https://crrev.com/9b078f490e80c0de0c221b7d6dbb5e18fa47e550/skia/ext/skia_utils_base_unittest.cc
[modify] https://crrev.com/9b078f490e80c0de0c221b7d6dbb5e18fa47e550/skia/public/interfaces/image_filter_struct_traits.h

ClusterFuzz has detected this issue as fixed in range 508862:508906.

Detailed report: https://clusterfuzz.com/testcase?key=5683505685331968

Fuzzer: ochang_domfuzzer
Job Type: linux_asan_chrome_mp
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  false. We should not have codec backed image filters in skia_utils_base.cc
  skia::CodecDisallowingPixelSerializer::onUseEncodedData
  SkImage::encodeToData
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_mp&range=506552:506627
Fixed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_mp&range=508862:508906

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5683505685331968

See https://github.com/google/clusterfuzz-tools for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

ClusterFuzz testcase 5683505685331968 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.