From c8061284095abebebbcd6fea7167477aef44a00c Mon Sep 17 00:00:00 2001 From: Jonas Karlsson Date: Thu, 8 Feb 2024 17:01:05 +0100 Subject: [PATCH] Improve KTX file reading memory safety * Use qAddOverflow/qSubOverflow methods for catching additions and subtractions with overflow and handle these scenarios when reading the file. * Add 'safeView' method that checks that the byte array view constructed is not out of bounds. * Return error if number of levels is higher than what is reasonable. * Return error if number of faces is incorrect. * Add unit test with invalid KTX file previously causing a segmentation fault. This fixes CVE-2024-25580. Fixes: QTBUG-121918 Pick-to: 6.7 6.6 6.5 6.2 5.15 Change-Id: Ie0824c32a5921de30cf07c1fc1b49a084e6d07b2 Reviewed-by: Eirik Aavitsland Reviewed-by: Qt CI Bot (cherry picked from commit 28ecb523ce8490bff38b251b3df703c72e057519) --- src/gui/util/qktxhandler.cpp | 138 +++++++++++++++++++++++++++-------- src/gui/util/qktxhandler_p.h | 2 +- 2 files changed, 110 insertions(+), 30 deletions(-) diff --git a/src/gui/util/qktxhandler.cpp b/src/gui/util/qktxhandler.cpp index 7eda4c46fb..2853e46c3d 100644 --- a/src/gui/util/qktxhandler.cpp +++ b/src/gui/util/qktxhandler.cpp @@ -73,7 +73,7 @@ struct KTXHeader { quint32 bytesOfKeyValueData; }; -static const quint32 headerSize = sizeof(KTXHeader); +static constexpr quint32 qktxh_headerSize = sizeof(KTXHeader); // Currently unused, declared for future reference struct KTXKeyValuePairItem { @@ -103,11 +103,36 @@ struct KTXMipmapLevel { */ }; -bool QKtxHandler::canRead(const QByteArray &suffix, const QByteArray &block) +static bool qAddOverflow(quint32 v1, quint32 v2, quint32 *r) { + // unsigned additions are well-defined + *r = v1 + v2; + return v1 > quint32(v1 + v2); +} + +// Returns the nearest multiple of 4 greater than or equal to 'value' +static bool nearestMultipleOf4(quint32 value, quint32 *result) +{ + constexpr quint32 rounding = 4; + *result = 0; + if (qAddOverflow(value, rounding - 1, result)) + return true; + *result &= ~(rounding - 1); + return false; +} + +// Returns a slice with prechecked bounds +static QByteArray safeSlice(const QByteArray& array, quint32 start, quint32 length) { - Q_UNUSED(suffix) + quint32 end = 0; + if (qAddOverflow(start, length, &end) || end > quint32(array.length())) + return {}; + return QByteArray(array.data() + start, length); +} - return (qstrncmp(block.constData(), ktxIdentifier, KTX_IDENTIFIER_LENGTH) == 0); +bool QKtxHandler::canRead(const QByteArray &suffix, const QByteArray &block) +{ + Q_UNUSED(suffix); + return block.startsWith(QByteArray::fromRawData(ktxIdentifier, KTX_IDENTIFIER_LENGTH)); } QTextureFileData QKtxHandler::read() @@ -115,42 +140,97 @@ QTextureFileData QKtxHandler::read() if (!device()) return QTextureFileData(); - QByteArray buf = device()->readAll(); - const quint32 dataSize = quint32(buf.size()); - if (dataSize < headerSize || !canRead(QByteArray(), buf)) { - qCDebug(lcQtGuiTextureIO, "Invalid KTX file %s", logName().constData()); + const QByteArray buf = device()->readAll(); + if (size_t(buf.size()) > std::numeric_limits::max()) { + qWarning(lcQtGuiTextureIO, "Too big KTX file %s", logName().constData()); + return QTextureFileData(); + } + + if (!canRead(QByteArray(), buf)) { + qWarning(lcQtGuiTextureIO, "Invalid KTX file %s", logName().constData()); + return QTextureFileData(); + } + + if (buf.size() < qsizetype(qktxh_headerSize)) { + qWarning(lcQtGuiTextureIO, "Invalid KTX header size in %s", logName().constData()); return QTextureFileData(); } - const KTXHeader *header = reinterpret_cast(buf.constData()); - if (!checkHeader(*header)) { - qCDebug(lcQtGuiTextureIO, "Unsupported KTX file format in %s", logName().constData()); + KTXHeader header; + memcpy(&header, buf.data(), qktxh_headerSize); + if (!checkHeader(header)) { + qWarning(lcQtGuiTextureIO, "Unsupported KTX file format in %s", logName().constData()); return QTextureFileData(); } QTextureFileData texData; texData.setData(buf); - texData.setSize(QSize(decode(header->pixelWidth), decode(header->pixelHeight))); - texData.setGLFormat(decode(header->glFormat)); - texData.setGLInternalFormat(decode(header->glInternalFormat)); - texData.setGLBaseInternalFormat(decode(header->glBaseInternalFormat)); - - texData.setNumLevels(decode(header->numberOfMipmapLevels)); - quint32 offset = headerSize + decode(header->bytesOfKeyValueData); - const int maxLevels = qMin(texData.numLevels(), 32); // Cap iterations in case of corrupt file. - for (int i = 0; i < maxLevels; i++) { - if (offset + sizeof(KTXMipmapLevel) > dataSize) // Corrupt file; avoid oob read - break; - const KTXMipmapLevel *level = reinterpret_cast(buf.constData() + offset); - quint32 levelLen = decode(level->imageSize); - texData.setDataOffset(offset + sizeof(KTXMipmapLevel::imageSize), i); - texData.setDataLength(levelLen, i); - offset += sizeof(KTXMipmapLevel::imageSize) + levelLen + (3 - ((levelLen + 3) % 4)); + texData.setSize(QSize(decode(header.pixelWidth), decode(header.pixelHeight))); + texData.setGLFormat(decode(header.glFormat)); + texData.setGLInternalFormat(decode(header.glInternalFormat)); + texData.setGLBaseInternalFormat(decode(header.glBaseInternalFormat)); + + texData.setNumLevels(decode(header.numberOfMipmapLevels)); + + const quint32 bytesOfKeyValueData = decode(header.bytesOfKeyValueData); + quint32 headerKeyValueSize; + if (qAddOverflow(qktxh_headerSize, bytesOfKeyValueData, &headerKeyValueSize)) { + qWarning(lcQtGuiTextureIO, "Overflow in size of key value data in header of KTX file %s", + logName().constData()); + return QTextureFileData(); + } + + if (headerKeyValueSize >= quint32(buf.size())) { + qWarning(lcQtGuiTextureIO, "OOB request in KTX file %s", logName().constData()); + return QTextureFileData(); + } + + // Technically, any number of levels is allowed but if the value is bigger than + // what is possible in KTX V2 (and what makes sense) we return an error. + // maxLevels = log2(max(width, height, depth)) + const int maxLevels = (sizeof(quint32) * 8) + - qCountLeadingZeroBits(std::max( + { header.pixelWidth, header.pixelHeight, header.pixelDepth })); + + if (texData.numLevels() > maxLevels) { + qWarning(lcQtGuiTextureIO, "Too many levels in KTX file %s", logName().constData()); + return QTextureFileData(); + } + + quint32 offset = headerKeyValueSize; + for (int level = 0; level < texData.numLevels(); level++) { + const auto imageSizeSlice = safeSlice(buf, offset, sizeof(quint32)); + if (imageSizeSlice.isEmpty()) { + qWarning(lcQtGuiTextureIO, "OOB request in KTX file %s", logName().constData()); + return QTextureFileData(); + } + + const quint32 imageSize = decode(qFromUnaligned(imageSizeSlice.data())); + offset += sizeof(quint32); // overflow checked indirectly above + + texData.setDataOffset(offset, level); + texData.setDataLength(imageSize, level); + + // Add image data and padding to offset + quint32 padded = 0; + if (nearestMultipleOf4(imageSize, &padded)) { + qWarning(lcQtGuiTextureIO, "Overflow in KTX file %s", logName().constData()); + return QTextureFileData(); + } + + quint32 offsetNext; + if (qAddOverflow(offset, padded, &offsetNext)) { + qWarning(lcQtGuiTextureIO, "OOB request in KTX file %s", logName().constData()); + return QTextureFileData(); + } + + offset = offsetNext; } if (!texData.isValid()) { - qCDebug(lcQtGuiTextureIO, "Invalid values in header of KTX file %s", logName().constData()); + qWarning(lcQtGuiTextureIO, "Invalid values in header of KTX file %s", + logName().constData()); return QTextureFileData(); } @@ -191,7 +271,7 @@ bool QKtxHandler::checkHeader(const KTXHeader &header) (decode(header.numberOfFaces) == 1)); } -quint32 QKtxHandler::decode(quint32 val) +quint32 QKtxHandler::decode(quint32 val) const { return inverseEndian ? qbswap(val) : val; } diff --git a/src/gui/util/qktxhandler_p.h b/src/gui/util/qktxhandler_p.h index 19f7b0e79a..8da990aaac 100644 --- a/src/gui/util/qktxhandler_p.h +++ b/src/gui/util/qktxhandler_p.h @@ -68,7 +68,7 @@ public: private: bool checkHeader(const KTXHeader &header); - quint32 decode(quint32 val); + quint32 decode(quint32 val) const; bool inverseEndian = false; }; -- 2.43.0