[Backport] CVE-2020-6540: Heap buffer overflow in Skia

Manual backport of patch originally reviewed on
https://skia-review.googlesource.com/c/skia/+/293349:
Rewrite SkVertices serialization to use SkReadBuffer/SkWriteBuffer

These classes are much safer (there's no way to safely deserialize a
string with SkReader32 without knowledge of how it works internally).
Prior to this CL, SkVertices was the only complex type that had manual
serialization using the lower level types - now it works like everything
else. Additionally: the versioning can now be tied to picture versions
going forward (like everything else).

Bug: chromium:1105720
Bug: chromium:1105723
Bug: oss-fuzz:22909
Bug: oss-fuzz:22918
Bug: skia:9984
Bug: skia:10304
Change-Id: I3cf537eb765b5c8ce98b554c0f200e5d67c33d14
Reviewed-by: Michal Klocek <michal.klocek@qt.io>
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>

4 files changed, 77 insertions, 70 deletions

diff --git a/chromium/third_party/skia/include/core/SkVertices.h b/chromium/third_party/skia/include/core/SkVertices.h
index c07e2dae632..ad7fc09d58d 100644
--- a/chromium/third_party/skia/include/core/SkVertices.h
+++ b/chromium/third_party/skia/include/core/SkVertices.h
@@ -14,6 +14,9 @@
 #include "include/core/SkRect.h"
 #include "include/core/SkRefCnt.h"
 
+class SkReadBuffer;
+class SkWriteBuffer;
+
 /**
  * An immutable set of vertex data that can be used with SkCanvas::drawVertices.
  */
@@ -238,13 +241,13 @@ public:
      *  Recreate a vertices from a buffer previously created by calling encode().
      *  Returns null if the data is corrupt or the length is incorrect for the contents.
      */
-    static sk_sp<SkVertices> Decode(const void* buffer, size_t length);
+    static sk_sp<SkVertices> Decode(SkReadBuffer&);
 
     /**
      *  Pack the vertices object into a byte buffer. This can be used to recreate the vertices
      *  by calling Decode() with the buffer.
      */
-    sk_sp<SkData> encode() const;
+    void encode(SkWriteBuffer&) const;
 
 private:
     SkVertices() {}
diff --git a/chromium/third_party/skia/src/core/SkPictureData.cpp b/chromium/third_party/skia/src/core/SkPictureData.cpp
index 9fd2768d6e2..672b59ea659 100644
--- a/chromium/third_party/skia/src/core/SkPictureData.cpp
+++ b/chromium/third_party/skia/src/core/SkPictureData.cpp
@@ -175,7 +175,7 @@ void SkPictureData::flattenToBuffer(SkWriteBuffer& buffer, bool textBlobsOnly) c
         if (!fVertices.empty()) {
             write_tag_size(buffer, SK_PICT_VERTICES_BUFFER_TAG, fVertices.count());
             for (const auto& vert : fVertices) {
-                buffer.writeDataAsByteArray(vert->encode().get());
+                vert->encode(buffer);
             }
         }
 
@@ -375,10 +375,6 @@ bool SkPictureData::parseStreamTag(SkStream* stream,
 static sk_sp<SkImage> create_image_from_buffer(SkReadBuffer& buffer) {
     return buffer.readImage();
 }
-static sk_sp<SkVertices> create_vertices_from_buffer(SkReadBuffer& buffer) {
-    auto data = buffer.readByteArrayAsData();
-    return data ? SkVertices::Decode(data->data(), data->size()) : nullptr;
-}
 
 static sk_sp<SkDrawable> create_drawable_from_buffer(SkReadBuffer& buffer) {
     return sk_sp<SkDrawable>((SkDrawable*)buffer.readFlattenable(SkFlattenable::kSkDrawable_Type));
@@ -441,7 +437,7 @@ void SkPictureData::parseBufferTag(SkReadBuffer& buffer, uint32_t tag, uint32_t
             new_array_from_buffer(buffer, size, fTextBlobs, SkTextBlobPriv::MakeFromBuffer);
             break;
         case SK_PICT_VERTICES_BUFFER_TAG:
-            new_array_from_buffer(buffer, size, fVertices, create_vertices_from_buffer);
+            new_array_from_buffer(buffer, size, fVertices, SkVertices::Decode);
             break;
         case SK_PICT_IMAGE_BUFFER_TAG:
             new_array_from_buffer(buffer, size, fImages, create_image_from_buffer);
diff --git a/chromium/third_party/skia/src/core/SkPicturePriv.h b/chromium/third_party/skia/src/core/SkPicturePriv.h
index 7c95675ffbf..aeb67f3a20b 100644
--- a/chromium/third_party/skia/src/core/SkPicturePriv.h
+++ b/chromium/third_party/skia/src/core/SkPicturePriv.h
@@ -74,7 +74,7 @@ public:
     // V71: Unify erode and dilate image filters
     // V72: SkColorFilter_Matrix domain (rgba vs. hsla)
     // V73: Use SkColor4f in per-edge AA quad API
-
+    // V75: SkVertices switched from unsafe use of SkReader32 to SkReadBuffer (like everything else)
     enum Version {
         kTileModeInBlurImageFilter_Version  = 56,
         kTileInfoInSweepGradient_Version    = 57,
@@ -94,10 +94,11 @@ public:
         kUnifyErodeDilateImpls_Version      = 71,
         kMatrixColorFilterDomain_Version    = 72,
         kEdgeAAQuadColor4f_Version          = 73,
+        kVerticesUseReadBuffer_Version      = 75,
 
         // Only SKPs within the min/current picture version range (inclusive) can be read.
         kMin_Version     = kTileModeInBlurImageFilter_Version,
-        kCurrent_Version = kEdgeAAQuadColor4f_Version
+        kCurrent_Version = kVerticesUseReadBuffer_Version
     };
 
     static_assert(kMin_Version <= 62, "Remove kFontAxes_bad from SkFontDescriptor.cpp");
diff --git a/chromium/third_party/skia/src/core/SkVertices.cpp b/chromium/third_party/skia/src/core/SkVertices.cpp
index dc2070372c1..f7b4d461c3e 100644
--- a/chromium/third_party/skia/src/core/SkVertices.cpp
+++ b/chromium/third_party/skia/src/core/SkVertices.cpp
@@ -9,10 +9,10 @@
 
 #include "include/core/SkData.h"
 #include "include/private/SkTo.h"
-#include "src/core/SkReader32.h"
+#include "src/core/SkReadBuffer.h"
 #include "src/core/SkSafeMath.h"
 #include "src/core/SkSafeRange.h"
-#include "src/core/SkWriter32.h"
+#include "src/core/SkWriteBuffer.h"
 #include <atomic>
 #include <new>
 
@@ -329,7 +329,7 @@ size_t SkVertices::approximateSize() const {
 #define kIsNonVolatile_Mask 0x800
 #define kHeaderSize         (3 * sizeof(uint32_t))
 
-sk_sp<SkData> SkVertices::encode() const {
+void SkVertices::encode(SkWriteBuffer& buffer) const {
     // packed has room for addtional flags in the future (e.g. versioning)
     uint32_t packed = static_cast<uint32_t>(fMode);
     SkASSERT((packed & ~kMode_Mask) == 0);  // our mode fits in the mask bits
@@ -354,75 +354,82 @@ sk_sp<SkData> SkVertices::encode() const {
                 this->hasBones());
     SkASSERT(sizes.isValid());
     SkASSERT(!sizes.fBuilderTriFanISize);
-    // need to force alignment to 4 for SkWriter32 -- will pad w/ 0s as needed
-    const size_t size = SkAlign4(kHeaderSize + sizes.fArrays);
-
-    sk_sp<SkData> data = SkData::MakeUninitialized(size);
-    SkWriter32 writer(data->writable_data(), data->size());
-
-    writer.write32(packed);
-    writer.write32(fVertexCnt);
-    writer.write32(fIndexCnt);
-    writer.write(fPositions, sizes.fVSize);
-    writer.write(fTexs, sizes.fTSize);
-    writer.write(fColors, sizes.fCSize);
-    writer.write(fBoneIndices, sizes.fBISize);
-    writer.write(fBoneWeights, sizes.fBWSize);
-    // if index-count is odd, we won't be 4-bytes aligned, so we call the pad version
-    writer.writePad(fIndices, sizes.fISize);
 
-    return data;
+    buffer.writeUInt(packed);
+    buffer.writeInt(fVertexCnt);
+    buffer.writeInt(fIndexCnt);
+    buffer.writeByteArray(fPositions, sizes.fVSize);
+    buffer.writeByteArray(fTexs, sizes.fTSize);
+    buffer.writeByteArray(fColors, sizes.fCSize);
+    buffer.writeByteArray(fBoneIndices, sizes.fBISize);
+    buffer.writeByteArray(fBoneWeights, sizes.fBWSize);
+    // if index-count is odd, we won't be 4-bytes aligned, so we call the pad version
+    buffer.writeByteArray(fIndices, sizes.fISize);
 }
 
-sk_sp<SkVertices> SkVertices::Decode(const void* data, size_t length) {
-    if (length < kHeaderSize) {
-        return nullptr;
+sk_sp<SkVertices> SkVertices::Decode(SkReadBuffer& buffer) {
+    if (buffer.isVersionLT(SkPicturePriv::kVerticesUseReadBuffer_Version)) {
+        // Old versions used an embedded blob that was serialized with SkWriter32/SkReader32.
+        // We don't support loading those, but skip over the vertices to keep the buffer valid.
+        auto data = buffer.readByteArrayAsData();
+        (void)data;
+         return nullptr;
     }
 
-    SkReader32 reader(data, length);
-    SkSafeRange safe;
+    auto decode = [](SkReadBuffer &buffer) -> sk_sp<SkVertices> {
+        SkSafeRange safe;
 
-    const uint32_t packed = reader.readInt();
-    const int vertexCount = safe.checkGE(reader.readInt(), 0);
-    const int indexCount = safe.checkGE(reader.readInt(), 0);
-    const VertexMode mode = safe.checkLE<VertexMode>(packed & kMode_Mask,
+        const uint32_t packed = buffer.readUInt();
+        const int vertexCount = safe.checkGE(buffer.readInt(), 0);
+        const int indexCount = safe.checkGE(buffer.readInt(), 0);
+        const VertexMode mode = safe.checkLE<VertexMode>(packed & kMode_Mask,
                                                      SkVertices::kLast_VertexMode);
-    if (!safe) {
-        return nullptr;
-    }
-    const bool hasTexs = SkToBool(packed & kHasTexs_Mask);
-    const bool hasColors = SkToBool(packed & kHasColors_Mask);
-    const bool hasBones = SkToBool(packed & kHasBones_Mask);
-    const bool isVolatile = !SkToBool(packed & kIsNonVolatile_Mask);
-    Sizes sizes(mode, vertexCount, indexCount, hasTexs, hasColors, hasBones);
-    if (!sizes.isValid()) {
-        return nullptr;
-    }
-    // logically we can be only 2-byte aligned, but our buffer is always 4-byte aligned
-    if (SkAlign4(kHeaderSize + sizes.fArrays) != length) {
-        return nullptr;
-    }
+        if (!safe) {
+            return nullptr;
+        }
+        const bool hasTexs = SkToBool(packed & kHasTexs_Mask);
+        const bool hasColors = SkToBool(packed & kHasColors_Mask);
+        const bool hasBones = SkToBool(packed & kHasBones_Mask);
+        const bool isVolatile = !SkToBool(packed & kIsNonVolatile_Mask);
+        Sizes sizes(mode, vertexCount, indexCount, hasTexs, hasColors, hasBones);
+        if (!sizes.isValid()) {
+            return nullptr;
+        }
 
-    Builder builder(mode, vertexCount, indexCount, isVolatile, sizes);
+        Builder builder(mode, vertexCount, indexCount, isVolatile, sizes);
+        if (!builder.isValid()) {
+            return nullptr;
+        }
+        buffer.readByteArray(builder.positions(), sizes.fVSize);
+        buffer.readByteArray(builder.texCoords(), sizes.fTSize);
+        buffer.readByteArray(builder.colors(), sizes.fCSize);
+        buffer.readByteArray(builder.boneIndices(), sizes.fBISize);
+        buffer.readByteArray(builder.boneWeights(), sizes.fBWSize);
+        size_t isize = (mode == kTriangleFan_VertexMode) ? sizes.fBuilderTriFanISize : sizes.fISize;
+        buffer.readByteArray(builder.indices(), isize);
+        if (!buffer.isValid()) {
+            return nullptr;
+        }
 
-    reader.read(builder.positions(), sizes.fVSize);
-    reader.read(builder.texCoords(), sizes.fTSize);
-    reader.read(builder.colors(), sizes.fCSize);
-    reader.read(builder.boneIndices(), sizes.fBISize);
-    reader.read(builder.boneWeights(), sizes.fBWSize);
-    size_t isize = (mode == kTriangleFan_VertexMode) ? sizes.fBuilderTriFanISize : sizes.fISize;
-    reader.read(builder.indices(), isize);
-    if (indexCount > 0) {
-        // validate that the indicies are in range
-        SkASSERT(indexCount == builder.indexCount());
-        const uint16_t* indices = builder.indices();
-        for (int i = 0; i < indexCount; ++i) {
-            if (indices[i] >= (unsigned)vertexCount) {
-                return nullptr;
+        if (indexCount > 0) {
+            // validate that the indicies are in range
+            SkASSERT(indexCount == builder.indexCount());
+            const uint16_t* indices = builder.indices();
+            for (int i = 0; i < indexCount; ++i) {
+                if (indices[i] >= (unsigned)vertexCount) {
+                    return nullptr;
+                }
             }
         }
+        return builder.detach();
+    };
+
+    if (auto verts = decode(buffer)) {
+        return verts;
     }
-    return builder.detach();
+
+    buffer.validate(false);
+    return nullptr;
 }
 
 void SkVertices::operator delete(void* p)