diff options
author | Brian Osman <brianosman@google.com> | 2020-06-01 16:33:28 -0400 |
---|---|---|
committer | Michael BrĂ¼ning <michael.bruning@qt.io> | 2020-08-19 12:35:42 +0000 |
commit | ee1811a7e86ba314e79911bee254552b85340326 (patch) | |
tree | fa64e11447c8e61890b7b4e9b3c863ed5a6702b1 | |
parent | 0561a33d0f59114ef815e86d6fadcb1111eb9ca8 (diff) | |
download | qtwebengine-chromium-ee1811a7e86ba314e79911bee254552b85340326.tar.gz |
[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>
7 files changed, 77 insertions, 75 deletions
diff --git a/chromium/third_party/skia/include/core/SkPicture.h b/chromium/third_party/skia/include/core/SkPicture.h index a88c101eda3..31e55dcaa85 100644 --- a/chromium/third_party/skia/include/core/SkPicture.h +++ b/chromium/third_party/skia/include/core/SkPicture.h @@ -220,7 +220,7 @@ private: // Only SKPs within the min/current picture version range (inclusive) can be read. static const uint32_t MIN_PICTURE_VERSION = 56; // august 2017 - static const uint32_t CURRENT_PICTURE_VERSION = 63; + static const uint32_t CURRENT_PICTURE_VERSION = 75; static_assert(MIN_PICTURE_VERSION <= 62, "Remove kFontAxes_bad from SkFontDescriptor.cpp"); diff --git a/chromium/third_party/skia/include/core/SkVertices.h b/chromium/third_party/skia/include/core/SkVertices.h index 9c1300b0a54..a5917c2edcc 100644 --- a/chromium/third_party/skia/include/core/SkVertices.h +++ b/chromium/third_party/skia/include/core/SkVertices.h @@ -14,6 +14,9 @@ #include "SkRect.h" #include "SkRefCnt.h" +class SkReadBuffer; +class SkWriteBuffer; + /** * An immutable set of vertex data that can be used with SkCanvas::drawVertices. */ @@ -175,13 +178,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 14a6d94c88e..4d1949dbcd6 100644 --- a/chromium/third_party/skia/src/core/SkPictureData.cpp +++ b/chromium/third_party/skia/src/core/SkPictureData.cpp @@ -163,7 +163,7 @@ void SkPictureData::flattenToBuffer(SkWriteBuffer& buffer) const { 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); } } @@ -343,10 +343,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)); @@ -408,7 +404,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/SkReadBuffer.h b/chromium/third_party/skia/src/core/SkReadBuffer.h index 1904cc825e2..c378fb73f32 100644 --- a/chromium/third_party/skia/src/core/SkReadBuffer.h +++ b/chromium/third_party/skia/src/core/SkReadBuffer.h @@ -79,6 +79,7 @@ public: kTwoColorDrawShadow_Version = 61, kDontNegateImageSize_Version = 62, kStoreImageBounds_Version = 63, + kVerticesUseReadBuffer_Version = 75, }; /** diff --git a/chromium/third_party/skia/src/core/SkVertices.cpp b/chromium/third_party/skia/src/core/SkVertices.cpp index 97a136c5488..abbc2a54ae7 100644 --- a/chromium/third_party/skia/src/core/SkVertices.cpp +++ b/chromium/third_party/skia/src/core/SkVertices.cpp @@ -9,11 +9,11 @@ #include "SkAtomics.h" #include "SkData.h" -#include "SkReader32.h" +#include "SkReadBuffer.h" #include "SkSafeMath.h" #include "SkSafeRange.h" #include "SkTo.h" -#include "SkWriter32.h" +#include "SkWriteBuffer.h" #include <new> static int32_t gNextID = 1; @@ -265,7 +265,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 @@ -290,75 +290,80 @@ 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); + 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 - writer.writePad(fIndices, sizes.fISize); - - return data; + 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(SkReadBuffer::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; - } - - Builder builder(mode, vertexCount, indexCount, isVolatile, sizes); + 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; + } - 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; + 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; + } + 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) diff --git a/chromium/third_party/skia/src/pipe/SkPipeCanvas.cpp b/chromium/third_party/skia/src/pipe/SkPipeCanvas.cpp index 258160c4fe9..8034efb1ba2 100644 --- a/chromium/third_party/skia/src/pipe/SkPipeCanvas.cpp +++ b/chromium/third_party/skia/src/pipe/SkPipeCanvas.cpp @@ -735,7 +735,7 @@ void SkPipeCanvas::onDrawVerticesObject(const SkVertices* vertices, const SkMatr SkPipeWriter writer(this); writer.write32(pack_verb(SkPipeVerb::kDrawVertices, extra)); // TODO: dedup vertices? - writer.writeDataAsByteArray(vertices->encode().get()); + vertices->encode(writer); writer.write32(boneCount); writer.write(bones, sizeof(SkMatrix) * boneCount); write_paint(writer, paint, kVertices_PaintUsage); diff --git a/chromium/third_party/skia/src/pipe/SkPipeReader.cpp b/chromium/third_party/skia/src/pipe/SkPipeReader.cpp index 909276f41ba..0fac7127bc6 100644 --- a/chromium/third_party/skia/src/pipe/SkPipeReader.cpp +++ b/chromium/third_party/skia/src/pipe/SkPipeReader.cpp @@ -562,10 +562,7 @@ static void drawImageLattice_handler(SkPipeReader& reader, uint32_t packedVerb, static void drawVertices_handler(SkPipeReader& reader, uint32_t packedVerb, SkCanvas* canvas) { SkASSERT(SkPipeVerb::kDrawVertices == unpack_verb(packedVerb)); SkBlendMode bmode = (SkBlendMode)unpack_verb_extra(packedVerb); - sk_sp<SkVertices> vertices = nullptr; - if (sk_sp<SkData> data = reader.readByteArrayAsData()) { - vertices = SkVertices::Decode(data->data(), data->size()); - } + sk_sp<SkVertices> vertices = SkVertices::Decode(reader); int boneCount = reader.read32(); const SkMatrix* bones = boneCount ? reader.skipT<SkMatrix>(boneCount) : nullptr; if (vertices) { |