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}
Denger-Network · Oct 14, 2017 · 9b078f4 · 9b078f4
1 parent d6c6e08
commit 9b078f4
Show file tree

Hide file tree

Showing 5 changed files with 137 additions and 24 deletions.
diff --git a/cc/ipc/cc_param_traits.cc b/cc/ipc/cc_param_traits.cc
@@ -293,7 +293,7 @@ bool ParamTraits<sk_sp<SkImageFilter>>::Read(const base::Pickle* m,
   if (!iter->ReadData(&data, &length))
     return false;
   if (length > 0) {
-    SkFlattenable* flattenable = SkValidatingDeserializeFlattenable(
+    SkFlattenable* flattenable = skia::ValidatingDeserializeFlattenable(
         data, length, SkImageFilter::GetFlattenableType());
     *r = sk_sp<SkImageFilter>(static_cast<SkImageFilter*>(flattenable));
   } else {

diff --git a/skia/ext/skia_utils_base.cc b/skia/ext/skia_utils_base.cc
@@ -8,25 +8,42 @@
 
 #include "base/pickle.h"
 #include "third_party/skia/include/core/SkData.h"
+#include "third_party/skia/include/core/SkImageDeserializer.h"
 #include "third_party/skia/include/core/SkWriteBuffer.h"
+#include "third_party/skia/src/core/SkValidatingReadBuffer.h"
 
 namespace skia {
 namespace {
 
-class CodecDisallowingPixelSerializer : public SkPixelSerializer {
+class CodecDecodingPixelSerializer : public SkPixelSerializer {
  public:
-  CodecDisallowingPixelSerializer() = default;
-  ~CodecDisallowingPixelSerializer() override = default;
+  CodecDecodingPixelSerializer() = default;
+  ~CodecDecodingPixelSerializer() override = default;
 
  protected:
-  bool onUseEncodedData(const void* data, size_t len) override {
-    CHECK(false) << "We should not have codec backed image filters";
-    return false;
-  }
+  // Disallowing serializing the encoded data.
+  bool onUseEncodedData(const void* data, size_t len) override { return false; }
 
+  // Don't return any encoded data to ensure the decoded bitmap is serialized.
   SkData* onEncode(const SkPixmap&) override { return nullptr; }
 };
 
+class CodecDisallowingImageDeserializer : public SkImageDeserializer {
+ public:
+  ~CodecDisallowingImageDeserializer() override = default;
+
+  sk_sp<SkImage> makeFromData(SkData*, const SkIRect* subset) override {
+    LOG(ERROR) << "Encoded image rejected during SkFlattenable deserialization";
+    return nullptr;
+  }
+  sk_sp<SkImage> makeFromMemory(const void* data,
+                                size_t length,
+                                const SkIRect* subset) override {
+    LOG(ERROR) << "Encoded image rejected during SkFlattenable deserialization";
+    return nullptr;
+  }
+};
+
 }  // namespace
 
 bool ReadSkString(base::PickleIterator* iter, SkString* str) {
@@ -98,12 +115,21 @@ void WriteSkFontStyle(base::Pickle* pickle, SkFontStyle style) {
 
 sk_sp<SkData> ValidatingSerializeFlattenable(SkFlattenable* flattenable) {
   SkBinaryWriteBuffer writer;
-  writer.setPixelSerializer(sk_make_sp<CodecDisallowingPixelSerializer>());
+  writer.setPixelSerializer(sk_make_sp<CodecDecodingPixelSerializer>());
   writer.writeFlattenable(flattenable);
   size_t size = writer.bytesWritten();
   auto data = SkData::MakeUninitialized(size);
   writer.writeToMemory(data->writable_data());
   return data;
 }
 
+SkFlattenable* ValidatingDeserializeFlattenable(const void* data,
+                                                size_t size,
+                                                SkFlattenable::Type type) {
+  SkValidatingReadBuffer buffer(data, size);
+  CodecDisallowingImageDeserializer image_deserializer;
+  buffer.setImageDeserializer(&image_deserializer);
+  return buffer.readFlattenable(type);
+}
+
 }  // namespace skia
diff --git a/skia/ext/skia_utils_base.h b/skia/ext/skia_utils_base.h
@@ -5,6 +5,7 @@
 #ifndef SKIA_EXT_SKIA_UTILS_BASE_H_
 #define SKIA_EXT_SKIA_UTILS_BASE_H_
 
+#include "third_party/skia/include/core/SkFlattenable.h"
 #include "third_party/skia/include/ports/SkFontConfigInterface.h"
 
 namespace base {
@@ -41,10 +42,17 @@ SK_API void WriteSkFontIdentity(
 // Writes style into the request pickle.
 SK_API void WriteSkFontStyle(base::Pickle* pickle, SkFontStyle style);
 
-// Serializes the SkFlattenable. This method should never be used with an
-// SkFlattenable containing encoded images.
+// Serializes the SkFlattenable. Any encoded images contained in the flattenable
+// will be decoded during serialization.
 SK_API sk_sp<SkData> ValidatingSerializeFlattenable(SkFlattenable* flattenable);
 
+// Deserializes the SkFlattenable. This method must not be called with data
+// containing encoded images.
+SK_API SkFlattenable* ValidatingDeserializeFlattenable(
+    const void* data,
+    size_t size,
+    SkFlattenable::Type type);
+
 }  // namespace skia
 
 #endif  // SKIA_EXT_SKIA_UTILS_BASE_H_
diff --git a/skia/ext/skia_utils_base_unittest.cc b/skia/ext/skia_utils_base_unittest.cc
@@ -6,44 +6,123 @@
 
 #include "base/memory/ptr_util.h"
 #include "base/test/gtest_util.h"
+#include "base/test/test_discardable_memory_allocator.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/skia/include/core/SkFlattenableSerialization.h"
 #include "third_party/skia/include/core/SkImageGenerator.h"
+#include "third_party/skia/include/core/SkPixelSerializer.h"
+#include "third_party/skia/include/core/SkWriteBuffer.h"
 #include "third_party/skia/include/effects/SkImageSource.h"
 #include "third_party/skia/include/effects/SkPaintImageFilter.h"
 
 namespace skia {
 namespace {
 
+// Raw data for a PNG file with 1x1 white pixels.
+const unsigned char kWhitePNG[] = {
+    0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0x00, 0x00, 0x00, 0x0d,
+    0x49, 0x48, 0x44, 0x52, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x01,
+    0x08, 0x02, 0x00, 0x00, 0x00, 0x90, 0x77, 0x53, 0xde, 0x00, 0x00, 0x00,
+    0x01, 0x73, 0x52, 0x47, 0x42, 0x00, 0xae, 0xce, 0x1c, 0xe9, 0x00, 0x00,
+    0x00, 0x09, 0x70, 0x48, 0x59, 0x73, 0x00, 0x00, 0x0b, 0x13, 0x00, 0x00,
+    0x0b, 0x13, 0x01, 0x00, 0x9a, 0x9c, 0x18, 0x00, 0x00, 0x00, 0x0c, 0x49,
+    0x44, 0x41, 0x54, 0x08, 0xd7, 0x63, 0xf8, 0xff, 0xff, 0x3f, 0x00, 0x05,
+    0xfe, 0x02, 0xfe, 0xdc, 0xcc, 0x59, 0xe7, 0x00, 0x00, 0x00, 0x00, 0x49,
+    0x45, 0x4e, 0x44, 0xae, 0x42, 0x60, 0x82,
+};
+
 class FakeImageGenerator : public SkImageGenerator {
  public:
   FakeImageGenerator() : SkImageGenerator(SkImageInfo::MakeN32Premul(10, 10)) {}
-  ~FakeImageGenerator() override = default;
+  ~FakeImageGenerator() override { EXPECT_TRUE(decoded_); }
 
   SkData* onRefEncodedData() override {
     // Use a generator that lets the caller encode it.
     return SkData::MakeWithCString("evilimage").release();
   }
 
-  bool onGetPixels(const SkImageInfo&, void*, size_t, const Options&) override {
-    NOTREACHED();
+  bool onGetPixels(const SkImageInfo& info,
+                   void* pixels,
+                   size_t,
+                   const Options&) override {
+    decoded_ = true;
+    memset(pixels, 0, info.computeMinByteSize());
+    return true;
+  }
+
+ private:
+  bool decoded_ = false;
+};
+
+class PixelSerializer : public SkPixelSerializer {
+ public:
+  bool onUseEncodedData(const void* data, size_t len) override {
+    CHECK(false);
     return false;
   }
+
+  SkData* onEncode(const SkPixmap&) override {
+    EXPECT_TRUE(has_decoded_images_);
+    return nullptr;
+  }
+
+  bool has_decoded_images_ = false;
 };
 
-#if GTEST_HAS_DEATH_TEST
-TEST(SkiaUtilsBaseTest, ImageSerializationFails) {
+TEST(SkiaUtilsBaseTest, ImageSerializationDecodesImage) {
+  base::TestDiscardableMemoryAllocator allocator;
+  base::DiscardableMemoryAllocator::SetInstance(&allocator);
+
   auto image =
       SkImage::MakeFromGenerator(base::MakeUnique<FakeImageGenerator>());
   auto filter = SkImageSource::Make(image);
-  EXPECT_DEATH(ValidatingSerializeFlattenable(filter.get()), "");
 
-  SkPaint paint;
-  paint.setShader(
-      image->makeShader(SkShader::kClamp_TileMode, SkShader::kClamp_TileMode));
-  filter = SkPaintImageFilter::Make(paint);
-  EXPECT_DEATH(ValidatingSerializeFlattenable(filter.get()), "");
+  // Serialize the filter. This decodes the image.
+  sk_sp<SkData> data = ValidatingSerializeFlattenable(filter.get());
+
+  // Deserialize.
+  auto deserialized_filter =
+      sk_sp<SkImageFilter>((SkImageFilter*)ValidatingDeserializeFlattenable(
+          data->data(), data->size(), SkImageFilter::GetFlattenableType()));
+
+  // Now serialize again to ensure that the deserialized filter did not have any
+  // encoded images. Serialization is the only way to deep inspect the filter.
+  SkBinaryWriteBuffer writer;
+  auto pixel_serializer = sk_make_sp<PixelSerializer>();
+  pixel_serializer->has_decoded_images_ = true;
+  writer.setPixelSerializer(pixel_serializer);
+  writer.writeFlattenable(deserialized_filter.get());
+  EXPECT_GT(writer.bytesWritten(), 0u);
+
+  base::DiscardableMemoryAllocator::SetInstance(nullptr);
+}
+
+TEST(SkiaUtilsBaseTest, DeserializationWithEncodedImagesFails) {
+  base::TestDiscardableMemoryAllocator allocator;
+  base::DiscardableMemoryAllocator::SetInstance(&allocator);
+
+  auto image = SkImage::MakeFromGenerator(SkImageGenerator::MakeFromEncoded(
+      SkData::MakeWithoutCopy(kWhitePNG, sizeof(kWhitePNG))));
+  auto filter = SkImageSource::Make(image);
+
+  // Serialize the filter using default serialization.
+  sk_sp<SkData> data(SkValidatingSerializeFlattenable(filter.get()));
+
+  // Deserialize with images disabled.
+  auto deserialized_filter =
+      sk_sp<SkImageFilter>((SkImageFilter*)ValidatingDeserializeFlattenable(
+          data->data(), data->size(), SkImageFilter::GetFlattenableType()));
+
+  // Now serialize again to make sure that all encoded images were rejected
+  // during serialization.
+  SkBinaryWriteBuffer writer;
+  auto pixel_serializer = sk_make_sp<PixelSerializer>();
+  writer.setPixelSerializer(pixel_serializer);
+  writer.writeFlattenable(deserialized_filter.get());
+  EXPECT_GT(writer.bytesWritten(), 0u);
+
+  base::DiscardableMemoryAllocator::SetInstance(nullptr);
 }
-#endif
 
 }  // namespace
 }  // namespace skia
diff --git a/skia/public/interfaces/image_filter_struct_traits.h b/skia/public/interfaces/image_filter_struct_traits.h
@@ -44,7 +44,7 @@ struct StructTraits<skia::mojom::ImageFilterDataView, sk_sp<SkImageFilter>> {
     ImageFilterBuffer buffer;
     if (!data.ReadData(&buffer))
       return false;
-    SkFlattenable* flattenable = SkValidatingDeserializeFlattenable(
+    SkFlattenable* flattenable = skia::ValidatingDeserializeFlattenable(
         buffer.data->bytes(), buffer.data->size(),
         SkImageFilter::GetFlattenableType());
     *out = sk_sp<SkImageFilter>(static_cast<SkImageFilter*>(flattenable));