From bf95260f37736202e500597d9770662cc1cb1720 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Janiszewski?= Date: Wed, 3 Mar 2021 12:59:18 +0100 Subject: [PATCH 1/2] Fix issues found in SawyerChunk decoder --- src/openrct2/rct12/SawyerChunkReader.cpp | 36 ++++++++++++++++++------ 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/src/openrct2/rct12/SawyerChunkReader.cpp b/src/openrct2/rct12/SawyerChunkReader.cpp index 81339d93fa..5ac60f2aff 100644 --- a/src/openrct2/rct12/SawyerChunkReader.cpp +++ b/src/openrct2/rct12/SawyerChunkReader.cpp @@ -84,13 +84,22 @@ std::shared_ptr SawyerChunkReader::ReadChunk() } auto buffer = static_cast(AllocateLargeTempBuffer()); - size_t uncompressedLength = DecodeChunk(buffer, MAX_UNCOMPRESSED_CHUNK_SIZE, compressedData.get(), header); - if (uncompressedLength == 0) + try { - throw SawyerChunkException(EXCEPTION_MSG_ZERO_SIZED_CHUNK); + size_t uncompressedLength = DecodeChunk(buffer, MAX_UNCOMPRESSED_CHUNK_SIZE, compressedData.get(), header); + if (uncompressedLength == 0) + { + throw SawyerChunkException(EXCEPTION_MSG_ZERO_SIZED_CHUNK); + } + buffer = static_cast(FinaliseLargeTempBuffer(buffer, uncompressedLength)); + return std::make_shared( + static_cast(header.encoding), buffer, uncompressedLength); + } + catch (const std::exception&) + { + FreeLargeTempBuffer(buffer); + throw; } - buffer = static_cast(FinaliseLargeTempBuffer(buffer, uncompressedLength)); - return std::make_shared(static_cast(header.encoding), buffer, uncompressedLength); } default: throw SawyerChunkException(EXCEPTION_MSG_INVALID_CHUNK_ENCODING); @@ -192,10 +201,10 @@ size_t SawyerChunkReader::DecodeChunk(void* dst, size_t dstCapacity, const void* size_t SawyerChunkReader::DecodeChunkRLERepeat(void* dst, size_t dstCapacity, const void* src, size_t srcLength) { - auto immBuffer = AllocateLargeTempBuffer(); - auto immLength = DecodeChunkRLE(immBuffer, MAX_UNCOMPRESSED_CHUNK_SIZE, src, srcLength); - auto size = DecodeChunkRepeat(dst, dstCapacity, immBuffer, immLength); - FreeLargeTempBuffer(immBuffer); + auto immBuffer = std::unique_ptr( + static_cast(AllocateLargeTempBuffer()), &FreeLargeTempBuffer); + auto immLength = DecodeChunkRLE(immBuffer.get(), MAX_UNCOMPRESSED_CHUNK_SIZE, src, srcLength); + auto size = DecodeChunkRepeat(dst, dstCapacity, immBuffer.get(), immLength); return size; } @@ -267,6 +276,15 @@ size_t SawyerChunkReader::DecodeChunkRepeat(void* dst, size_t dstCapacity, const { throw SawyerChunkException(EXCEPTION_MSG_DESTINATION_TOO_SMALL); } + if (copySrc < dst) + { + throw SawyerChunkException(EXCEPTION_MSG_CORRUPT_RLE); + } + if ((copySrc < (dst8 + count) && copySrc >= dst8) + || ((copySrc + count) <= (dst8 + count) && (copySrc + count) > dst8)) + { + throw SawyerChunkException(EXCEPTION_MSG_CORRUPT_RLE); + } std::memcpy(dst8, copySrc, count); dst8 += count; From 29a1cf018db0ecda6216e1c356b2a1927b7955b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Janiszewski?= Date: Wed, 3 Mar 2021 18:03:19 +0100 Subject: [PATCH 2/2] Add negative tests for SawyerChunkReader --- src/openrct2/rct12/SawyerChunkReader.cpp | 13 ---- src/openrct2/rct12/SawyerChunkReader.h | 14 ++++ test/tests/sawyercoding_test.cpp | 82 ++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 13 deletions(-) diff --git a/src/openrct2/rct12/SawyerChunkReader.cpp b/src/openrct2/rct12/SawyerChunkReader.cpp index 5ac60f2aff..f1bf44933f 100644 --- a/src/openrct2/rct12/SawyerChunkReader.cpp +++ b/src/openrct2/rct12/SawyerChunkReader.cpp @@ -27,19 +27,6 @@ constexpr const char* EXCEPTION_MSG_DESTINATION_TOO_SMALL = "Chunk data larger t constexpr const char* EXCEPTION_MSG_INVALID_CHUNK_ENCODING = "Invalid chunk encoding."; constexpr const char* EXCEPTION_MSG_ZERO_SIZED_CHUNK = "Encountered zero-sized chunk."; -class SawyerChunkException : public IOException -{ -public: - explicit SawyerChunkException(const char* message) - : IOException(message) - { - } - explicit SawyerChunkException(const std::string& message) - : IOException(message) - { - } -}; - SawyerChunkReader::SawyerChunkReader(OpenRCT2::IStream* stream) : _stream(stream) { diff --git a/src/openrct2/rct12/SawyerChunkReader.h b/src/openrct2/rct12/SawyerChunkReader.h index 7c86e150d2..8fb370b25e 100644 --- a/src/openrct2/rct12/SawyerChunkReader.h +++ b/src/openrct2/rct12/SawyerChunkReader.h @@ -10,11 +10,25 @@ #pragma once #include "../common.h" +#include "../core/IStream.hpp" #include "../util/SawyerCoding.h" #include "SawyerChunk.h" #include +class SawyerChunkException : public IOException +{ +public: + explicit SawyerChunkException(const char* message) + : IOException(message) + { + } + explicit SawyerChunkException(const std::string& message) + : IOException(message) + { + } +}; + namespace OpenRCT2 { struct IStream; diff --git a/test/tests/sawyercoding_test.cpp b/test/tests/sawyercoding_test.cpp index aa92b59552..1a72af0ddb 100644 --- a/test/tests/sawyercoding_test.cpp +++ b/test/tests/sawyercoding_test.cpp @@ -22,6 +22,14 @@ protected: static const uint8_t rledata[1038]; static const uint8_t rlecompresseddata[1949]; static const uint8_t rotatedata[1029]; + static const uint8_t invalid1[11]; + static const uint8_t invalid2[11]; + static const uint8_t invalid3[14]; + static const uint8_t invalid4[7]; + static const uint8_t invalid5[8]; + static const uint8_t invalid6[7]; + static const uint8_t invalid7[6]; + static const uint8_t empty[1]; void test_encode_decode(uint8_t encoding_type) { @@ -106,6 +114,62 @@ TEST_F(SawyerCodingTest, decode_chunk_rotate) test_decode(rotatedata, sizeof(rotatedata)); } +TEST_F(SawyerCodingTest, invalid1) +{ + OpenRCT2::MemoryStream ms(invalid1, sizeof(invalid1)); + SawyerChunkReader reader(&ms); + EXPECT_THROW(reader.ReadChunk(), SawyerChunkException); +} + +TEST_F(SawyerCodingTest, invalid2) +{ + OpenRCT2::MemoryStream ms(invalid2, sizeof(invalid2)); + SawyerChunkReader reader(&ms); + EXPECT_THROW(reader.ReadChunk(), SawyerChunkException); +} + +TEST_F(SawyerCodingTest, invalid3) +{ + OpenRCT2::MemoryStream ms(invalid3, sizeof(invalid3)); + SawyerChunkReader reader(&ms); + EXPECT_THROW(reader.ReadChunk(), SawyerChunkException); +} + +TEST_F(SawyerCodingTest, invalid4) +{ + OpenRCT2::MemoryStream ms(invalid4, sizeof(invalid4)); + SawyerChunkReader reader(&ms); + EXPECT_THROW(reader.ReadChunk(), SawyerChunkException); +} + +TEST_F(SawyerCodingTest, invalid5) +{ + OpenRCT2::MemoryStream ms(invalid5, sizeof(invalid5)); + SawyerChunkReader reader(&ms); + EXPECT_THROW(reader.ReadChunk(), SawyerChunkException); +} + +TEST_F(SawyerCodingTest, invalid6) +{ + OpenRCT2::MemoryStream ms(invalid6, sizeof(invalid6)); + SawyerChunkReader reader(&ms); + EXPECT_THROW(reader.ReadChunk(), SawyerChunkException); +} + +TEST_F(SawyerCodingTest, invalid7) +{ + OpenRCT2::MemoryStream ms(invalid7, sizeof(invalid7)); + SawyerChunkReader reader(&ms); + EXPECT_THROW(reader.ReadChunk(), SawyerChunkException); +} + +TEST_F(SawyerCodingTest, empty) +{ + OpenRCT2::MemoryStream ms(empty, 0); + SawyerChunkReader reader(&ms); + EXPECT_THROW(reader.ReadChunk(), IOException); +} + // 1024 bytes of random data // use `dd if=/dev/urandom bs=1024 count=1 | xxd -i` to get your own const uint8_t SawyerCodingTest::randomdata[] = { @@ -430,3 +494,21 @@ const uint8_t SawyerCodingTest::rotatedata[] = { 0xee, 0x6d, 0x4b, 0x24, 0x20, 0x9c, 0x83, 0xdb, 0xce, 0x18, 0xb8, 0xa3, 0x8a, 0x52, 0xda, 0x1a, 0x33, 0xe4, 0xc5, 0x07, 0x40, 0x7d, 0xf4, 0xfa, 0x2f, 0x6b, 0x93, 0x44, 0x5e, }; + +const uint8_t SawyerCodingTest::invalid1[] = { 0x02, 0x04, 0x00, 0x00, 0x00, 0xd4, 0xff, 0xce, 0xcf, 0x2e, 0x00 }; + +const uint8_t SawyerCodingTest::invalid2[] = { 0x02, 0x03, 0x00, 0x00, 0x00, 0x01, 0x00, 0x27, 0x3f, 0x01, 0x00 }; + +const uint8_t SawyerCodingTest::invalid3[] = { 0x02, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x73 }; + +const uint8_t SawyerCodingTest::invalid4[] = { 0x02, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00 }; + +const uint8_t SawyerCodingTest::invalid5[] = { 0x00, 0x00, 0x00, 0x00, 0x00, 0xdf, 0x00, 0x00 }; + +const uint8_t SawyerCodingTest::invalid6[] = { 0x02, 0x02, 0x00, 0x00, 0x00, 0x28, 0x7f }; + +const uint8_t SawyerCodingTest::invalid7[] = { 0x01, 0x01, 0x00, 0x00, 0x00, 0x00 }; + +// This needs to be non-empty to satisfy MSVC. We still pass zero as length. +const uint8_t SawyerCodingTest::empty[] = { 0x00 };