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
Change-Id: Ie0824c32a5921de30cf07c1fc1b49a084e6d07b2
Reviewed-by: Eirik Aavitsland <eirik.aavitsland@qt.io>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
(cherry picked from commit 28ecb523ce8490bff38b251b3df703c72e057519)
Reviewed-by: Jonas Karlsson <jonas.karlsson@qt.io>

src/gui/util/qktxhandler.cpp

@@ -41,7 +41,7 @@ struct KTXHeader {
     quint32 bytesOfKeyValueData;
 };
 
-static const quint32 qktxh_headerSize = sizeof(KTXHeader);
+static constexpr quint32 qktxh_headerSize = sizeof(KTXHeader);
 
 // Currently unused, declared for future reference
 struct KTXKeyValuePairItem {
@@ -71,11 +71,24 @@ struct KTXMipmapLevel {
     */
 };
 
-// Returns the nearest multiple of 'rounding' greater than or equal to 'value'
-constexpr quint32 withPadding(quint32 value, quint32 rounding)
+// Returns the nearest multiple of 4 greater than or equal to 'value'
+static const std::optional<quint32> nearestMultipleOf4(quint32 value)
 {
-    Q_ASSERT(rounding > 1);
-    return value + (rounding - 1) - ((value + (rounding - 1)) % rounding);
+    constexpr quint32 rounding = 4;
+    quint32 result = 0;
+    if (qAddOverflow(value, rounding - 1, &result))
+        return std::nullopt;
+    result &= ~(rounding - 1);
+    return result;
+}
+
+// Returns a view with prechecked bounds
+static QByteArrayView safeView(QByteArrayView view, quint32 start, quint32 length)
+{
+    quint32 end = 0;
+    if (qAddOverflow(start, length, &end) || end > quint32(view.length()))
+        return {};
+    return view.sliced(start, length);
 }
 
 QKtxHandler::~QKtxHandler() = default;
@@ -83,8 +96,7 @@ QKtxHandler::~QKtxHandler() = default;
 bool QKtxHandler::canRead(const QByteArray &suffix, const QByteArray &block)
 {
     Q_UNUSED(suffix);
-
-    return (qstrncmp(block.constData(), ktxIdentifier, KTX_IDENTIFIER_LENGTH) == 0);
+    return block.startsWith(ktxIdentifier);
 }
 
 QTextureFileData QKtxHandler::read()
@@ -93,55 +105,122 @@ QTextureFileData QKtxHandler::read()
         return QTextureFileData();
 
     const QByteArray buf = device()->readAll();
-    const quint32 dataSize = quint32(buf.size());
-    if (dataSize < qktxh_headerSize || !canRead(QByteArray(), buf)) {
-        qCDebug(lcQtGuiTextureIO, "Invalid KTX file %s", logName().constData());
+    if (buf.size() > std::numeric_limits<quint32>::max()) {
+        qWarning(lcQtGuiTextureIO, "Too big KTX file %s", logName().constData());
         return QTextureFileData();
     }
 
-    const KTXHeader *header = reinterpret_cast<const KTXHeader *>(buf.data());
-    if (!checkHeader(*header)) {
-        qCDebug(lcQtGuiTextureIO, "Unsupported KTX file format in %s", logName().constData());
+    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();
+    }
+
+    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.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));
-    texData.setNumFaces(decode(header->numberOfFaces));
+    texData.setNumLevels(decode(header.numberOfMipmapLevels));
+    texData.setNumFaces(decode(header.numberOfFaces));
 
-    const quint32 bytesOfKeyValueData = decode(header->bytesOfKeyValueData);
-    if (qktxh_headerSize + bytesOfKeyValueData < quint64(buf.size())) // oob check
-        texData.setKeyValueMetadata(decodeKeyValues(
-                QByteArrayView(buf.data() + qktxh_headerSize, bytesOfKeyValueData)));
-    quint32 offset = qktxh_headerSize + bytesOfKeyValueData;
+    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();
+    }
 
-    constexpr int MAX_ITERATIONS = 32; // cap iterations in case of corrupt data
+    if (headerKeyValueSize >= quint32(buf.size())) {
+        qWarning(lcQtGuiTextureIO, "OOB request in KTX file %s", logName().constData());
+        return QTextureFileData();
+    }
 
-    for (int level = 0; level < qMin(texData.numLevels(), MAX_ITERATIONS); level++) {
-        if (offset + sizeof(quint32) > dataSize) // Corrupt file; avoid oob read
-            break;
+    // File contains key/values
+    if (bytesOfKeyValueData > 0) {
+        auto keyValueDataView = safeView(buf, qktxh_headerSize, bytesOfKeyValueData);
+        if (keyValueDataView.isEmpty()) {
+            qWarning(lcQtGuiTextureIO, "Invalid view in KTX file %s", logName().constData());
+            return QTextureFileData();
+        }
 
-        const quint32 imageSize = decode(qFromUnaligned<quint32>(buf.data() + offset));
-        offset += sizeof(quint32);
+        auto keyValues = decodeKeyValues(keyValueDataView);
+        if (!keyValues) {
+            qWarning(lcQtGuiTextureIO, "Could not parse key values in KTX file %s",
+                     logName().constData());
+            return QTextureFileData();
+        }
 
-        for (int face = 0; face < qMin(texData.numFaces(), MAX_ITERATIONS); face++) {
+        texData.setKeyValueMetadata(*keyValues);
+    }
+
+    // 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();
+    }
+
+    if (texData.numFaces() != 1 && texData.numFaces() != 6) {
+        qWarning(lcQtGuiTextureIO, "Invalid number of faces in KTX file %s", logName().constData());
+        return QTextureFileData();
+    }
+
+    quint32 offset = headerKeyValueSize;
+    for (int level = 0; level < texData.numLevels(); level++) {
+        const auto imageSizeView = safeView(buf, offset, sizeof(quint32));
+        if (imageSizeView.isEmpty()) {
+            qWarning(lcQtGuiTextureIO, "OOB request in KTX file %s", logName().constData());
+            return QTextureFileData();
+        }
+
+        const quint32 imageSize = decode(qFromUnaligned<quint32>(imageSizeView.data()));
+        offset += sizeof(quint32); // overflow checked indirectly above
+
+        for (int face = 0; face < texData.numFaces(); face++) {
             texData.setDataOffset(offset, level, face);
             texData.setDataLength(imageSize, level, face);
 
             // Add image data and padding to offset
-            offset += withPadding(imageSize, 4);
+            const auto padded = nearestMultipleOf4(imageSize);
+            if (!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();
     }
 
@@ -187,33 +266,83 @@ bool QKtxHandler::checkHeader(const KTXHeader &header)
     return is2D && (isCubeMap || isCompressedImage);
 }
 
-QMap<QByteArray, QByteArray> QKtxHandler::decodeKeyValues(QByteArrayView view) const
+std::optional<QMap<QByteArray, QByteArray>> QKtxHandler::decodeKeyValues(QByteArrayView view) const
 {
     QMap<QByteArray, QByteArray> output;
     quint32 offset = 0;
-    while (offset < view.size() + sizeof(quint32)) {
-        const quint32 keyAndValueByteSize =
-                decode(qFromUnaligned<quint32>(view.constData() + offset));
-        offset += sizeof(quint32);
+    while (offset < quint32(view.size())) {
+        const auto keyAndValueByteSizeView = safeView(view, offset, sizeof(quint32));
+        if (keyAndValueByteSizeView.isEmpty()) {
+            qWarning(lcQtGuiTextureIO, "Invalid view in KTX key-value");
+            return std::nullopt;
+        }
 
-        if (offset + keyAndValueByteSize > quint64(view.size()))
-            break; // oob read
+        const quint32 keyAndValueByteSize =
+                decode(qFromUnaligned<quint32>(keyAndValueByteSizeView.data()));
+
+        quint32 offsetKeyAndValueStart;
+        if (qAddOverflow(offset, quint32(sizeof(quint32)), &offsetKeyAndValueStart)) {
+            qWarning(lcQtGuiTextureIO, "Overflow in KTX key-value");
+            return std::nullopt;
+        }
+
+        quint32 offsetKeyAndValueEnd;
+        if (qAddOverflow(offsetKeyAndValueStart, keyAndValueByteSize, &offsetKeyAndValueEnd)) {
+            qWarning(lcQtGuiTextureIO, "Overflow in KTX key-value");
+            return std::nullopt;
+        }
+
+        const auto keyValueView = safeView(view, offsetKeyAndValueStart, keyAndValueByteSize);
+        if (keyValueView.isEmpty()) {
+            qWarning(lcQtGuiTextureIO, "Invalid view in KTX key-value");
+            return std::nullopt;
+        }
 
         // 'key' is a UTF-8 string ending with a null terminator, 'value' is the rest.
         // To separate the key and value we convert the complete data to utf-8 and find the first
         // null terminator from the left, here we split the data into two.
-        const auto str = QString::fromUtf8(view.constData() + offset, keyAndValueByteSize);
-        const int idx = str.indexOf('\0'_L1);
-        if (idx == -1)
-            continue;
 
-        const QByteArray key = str.left(idx).toUtf8();
-        const size_t keySize = key.size() + 1; // Actual data size
-        const QByteArray value = QByteArray::fromRawData(view.constData() + offset + keySize,
-                                                         keyAndValueByteSize - keySize);
+        const int idx = keyValueView.indexOf('\0');
+        if (idx == -1) {
+            qWarning(lcQtGuiTextureIO, "Invalid key in KTX key-value");
+            return std::nullopt;
+        }
 
-        offset = withPadding(offset + keyAndValueByteSize, 4);
-        output.insert(key, value);
+        const QByteArrayView keyView = safeView(view, offsetKeyAndValueStart, idx);
+        if (keyView.isEmpty()) {
+            qWarning(lcQtGuiTextureIO, "Overflow in KTX key-value");
+            return std::nullopt;
+        }
+
+        const quint32 keySize = idx + 1; // Actual data size
+
+        quint32 offsetValueStart;
+        if (qAddOverflow(offsetKeyAndValueStart, keySize, &offsetValueStart)) {
+            qWarning(lcQtGuiTextureIO, "Overflow in KTX key-value");
+            return std::nullopt;
+        }
+
+        quint32 valueSize;
+        if (qSubOverflow(keyAndValueByteSize, keySize, &valueSize)) {
+            qWarning(lcQtGuiTextureIO, "Underflow in KTX key-value");
+            return std::nullopt;
+        }
+
+        const QByteArrayView valueView = safeView(view, offsetValueStart, valueSize);
+        if (valueView.isEmpty()) {
+            qWarning(lcQtGuiTextureIO, "Invalid view in KTX key-value");
+            return std::nullopt;
+        }
+
+        output.insert(keyView.toByteArray(), valueView.toByteArray());
+
+        const auto offsetNext = nearestMultipleOf4(offsetKeyAndValueEnd);
+        if (!offsetNext) {
+            qWarning(lcQtGuiTextureIO, "Overflow in KTX key-value");
+            return std::nullopt;
+        }
+
+        offset = *offsetNext;
     }
 
     return output;

src/gui/util/qktxhandler_p.h

@@ -17,6 +17,8 @@
 
 #include "qtexturefilehandler_p.h"
 
+#include <optional>
+
 QT_BEGIN_NAMESPACE
 
 struct KTXHeader;
@@ -33,7 +35,7 @@ public:
 
 private:
     bool checkHeader(const KTXHeader &header);
-    QMap<QByteArray, QByteArray> decodeKeyValues(QByteArrayView view) const;
+    std::optional<QMap<QByteArray, QByteArray>> decodeKeyValues(QByteArrayView view) const;
     quint32 decode(quint32 val) const;
 
     bool inverseEndian = false;

tests/auto/gui/util/qtexturefilereader/CMakeLists.txt

@@ -11,6 +11,7 @@ set(qtexturefilereader_resource_files
     "texturefiles/car_mips.ktx"
     "texturefiles/cubemap_float32_rgba.ktx"
     "texturefiles/cubemap_metadata.ktx"
+    "texturefiles/invalid.ktx"
     "texturefiles/newlogo.astc"
     "texturefiles/newlogo_srgb.astc"
     "texturefiles/pattern.pkm"

tests/auto/gui/util/qtexturefilereader/tst_qtexturefilereader.cpp

@@ -11,6 +11,7 @@ class tst_qtexturefilereader : public QObject
 private slots:
     void checkHandlers_data();
     void checkHandlers();
+    void checkInvalid();
     void checkMetadata();
 };
 
@@ -140,6 +141,18 @@ void tst_qtexturefilereader::checkMetadata()
     QCOMPARE(kvs.value("test C"), QByteArrayLiteral("3\x0000"));
 }
 
+void tst_qtexturefilereader::checkInvalid()
+{
+    QFile f(":/texturefiles/invalid.ktx");
+    QVERIFY(f.open(QIODevice::ReadOnly));
+    QTextureFileReader r(&f);
+    QTextureFileData d = r.read();
+    auto kvs = d.keyValueMetadata();
+
+    // Basically just checking that we don't crash on and invalid file
+    QVERIFY(kvs.empty());
+}
+
 QTEST_MAIN(tst_qtexturefilereader)
 
 #include "tst_qtexturefilereader.moc"