206 lines
9.4 KiB
Diff
206 lines
9.4 KiB
Diff
From d0cd01d60dcf9e2422dc720ed8321a195867afe6 Mon Sep 17 00:00:00 2001
|
|
From: Ivan Solovev <ivan.solovev@qt.io>
|
|
Date: Mon, 10 Aug 2026 10:56:49 +0200
|
|
Subject: [PATCH] QBinaryJson: prevent OOB read in fromBinaryData()
|
|
|
|
The fromBinaryData() function computes the document bound in
|
|
size_t, but then truncates it to uint. As a result, when
|
|
provided a malformed binary json with root.size in the range of
|
|
[0xFFFFFFF8, 0xFFFFFFFF], the total size overflows to the values
|
|
between 0 and 7, and the size check incorrectly passes.
|
|
When combined with a malicious tableOffset, this results in an
|
|
out-of-bounds read in the range of up to 4 GiB past the end of
|
|
the allocated buffer - depending on the memory layout it can
|
|
lead to either a crash or a read of some potentially sensitive
|
|
data.
|
|
|
|
Do the calculations in uint and use qAddOverflow() to detect
|
|
overflows. Since the underlying data structure uses uint to
|
|
store the size of the allocated data, we can simply return
|
|
early if we detect an overflow.
|
|
|
|
The newly-added test creates a larger buffer and copies a correct
|
|
binary json blob there. Then it injects the malformed size of the
|
|
payload and a new offset of the data table, together with the new
|
|
table. Then the test uses this large buffer to demonstrate an
|
|
out-of-bounds read on a modified blob. If the data was not backed
|
|
by a large buffer, it would have been an out-of-bounds read.
|
|
|
|
The fromRawData() method is not affected (assuming that the passed
|
|
size is correct), because it uses the input size parameter
|
|
directly, without analyzing the internal structure of the binary
|
|
json blob.
|
|
|
|
Pick-to: 6.8 6.5
|
|
Fixes: QTBUG-149036
|
|
Change-Id: I1a81deebb86cd7251ebeef9eea789b692f9d94b9
|
|
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
|
|
Reviewed-by: Marc Mutz <marc.mutz@qt.io>
|
|
(cherry picked from commit c0a495894d83a3c48055bd1e544f93147fc6ade1)
|
|
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
|
|
(cherry picked from commit 173cc93ec160719dfb0268971ba16e4e8f46dc57)
|
|
---
|
|
|
|
diff --git a/src/core5/serialization/qbinaryjson.cpp b/src/core5/serialization/qbinaryjson.cpp
|
|
index 76a232f..7653924 100644
|
|
--- a/src/core5/serialization/qbinaryjson.cpp
|
|
+++ b/src/core5/serialization/qbinaryjson.cpp
|
|
@@ -162,7 +162,10 @@
|
|
memcpy(&root, data.constData() + sizeof(QBinaryJsonPrivate::Header),
|
|
sizeof(QBinaryJsonPrivate::Base));
|
|
|
|
- const uint size = sizeof(QBinaryJsonPrivate::Header) + root.size;
|
|
+ uint size = 0;
|
|
+ if (qAddOverflow(uint{sizeof(QBinaryJsonPrivate::Header)}, uint{root.size}, &size))
|
|
+ return QJsonDocument(); // not representable in Private::ConstData
|
|
+
|
|
if (h.tag != QJsonDocument::BinaryFormatTag || h.version != 1U || size > uint(data.size()))
|
|
return QJsonDocument();
|
|
|
|
diff --git a/tests/auto/core5/serialization/json/tst_qtjson.cpp b/tests/auto/core5/serialization/json/tst_qtjson.cpp
|
|
index 9138254..c0ce487 100644
|
|
--- a/tests/auto/core5/serialization/json/tst_qtjson.cpp
|
|
+++ b/tests/auto/core5/serialization/json/tst_qtjson.cpp
|
|
@@ -5,6 +5,7 @@
|
|
|
|
#include "qbinaryjson.h"
|
|
|
|
+#include <QtCore/qendian.h>
|
|
#include "qjsonarray.h"
|
|
#include "qjsonobject.h"
|
|
#include "qjsonvalue.h"
|
|
@@ -33,6 +34,9 @@
|
|
void compactObject();
|
|
void validation();
|
|
void testCompactionError();
|
|
+
|
|
+ void binaryDataOutOfBoundsRead_data();
|
|
+ void binaryDataOutOfBoundsRead();
|
|
private:
|
|
QString testDataDir;
|
|
};
|
|
@@ -272,5 +276,123 @@
|
|
}
|
|
}
|
|
|
|
+enum class CallType : quint8
|
|
+{
|
|
+ Binary,
|
|
+ Raw,
|
|
+};
|
|
+
|
|
+void tst_QtJson::binaryDataOutOfBoundsRead_data()
|
|
+{
|
|
+ QTest::addColumn<quint32>("corruptedSize");
|
|
+ QTest::addColumn<int>("corruptedTableOffset");
|
|
+ QTest::addColumn<CallType>("callType");
|
|
+
|
|
+ // The case with 0xFFFFFFF4 should be handled properly, because the
|
|
+ // calculation does not roundtrip
|
|
+ QTest::newRow("0xFFFFFFF4_128_binary") << 0xFFFFFFF4u << 128 << CallType::Binary;
|
|
+ QTest::newRow("0xFFFFFFF8_32_binary") << 0xFFFFFFF8u << 32 << CallType::Binary;
|
|
+ QTest::newRow("0xFFFFFFFC_512_binary") << 0xFFFFFFFCu << 512 << CallType::Binary;
|
|
+ QTest::newRow("0xFFFFFFFF_4088_binary") << 0xFFFFFFFFu<< 4088 << CallType::Binary;
|
|
+
|
|
+ QTest::newRow("0xFFFFFFF4_128_raw") << 0xFFFFFFF4u << 128 << CallType::Raw;
|
|
+ QTest::newRow("0xFFFFFFF8_32_raw") << 0xFFFFFFF8u << 32 << CallType::Raw;
|
|
+ QTest::newRow("0xFFFFFFFC_512_raw") << 0xFFFFFFFCu << 512 << CallType::Raw;
|
|
+ QTest::newRow("0xFFFFFFFF_4088_raw") << 0xFFFFFFFFu<< 4088 << CallType::Raw;
|
|
+}
|
|
+
|
|
+void tst_QtJson::binaryDataOutOfBoundsRead()
|
|
+{
|
|
+ QFETCH(const quint32, corruptedSize);
|
|
+ QFETCH(const int, corruptedTableOffset);
|
|
+ QFETCH(const CallType, callType);
|
|
+
|
|
+ // Two single element arrays whose value is small enough to be stored inline
|
|
+ // in the root's value table: Value packs a 27 bit signed int, and
|
|
+ // Value::isValid() accepts an inline int without checking any offset. Both
|
|
+ // therefore encode to Header(8) + Base(12) + one 4 byte Value = 24 bytes
|
|
+ // with no out-of-line data, so moving the table moves the value with it.
|
|
+ const QJsonDocument original = QJsonDocument::fromJson("[1]");
|
|
+ const QJsonDocument planted = QJsonDocument::fromJson("[424242]");
|
|
+ QVERIFY(!original.isNull());
|
|
+ QVERIFY(!planted.isNull());
|
|
+
|
|
+ const QByteArray originalBlob = QBinaryJson::toBinaryData(original);
|
|
+ const QByteArray plantedBlob = QBinaryJson::toBinaryData(planted);
|
|
+
|
|
+ const qsizetype originalBlobSize = originalBlob.size();
|
|
+
|
|
+ constexpr qsizetype HeaderSize = 8; // Header { qle_uint tag; qle_uint version; }
|
|
+ constexpr qsizetype RootSizeOffset = HeaderSize; // Base::size
|
|
+ constexpr qsizetype TableOffsetOffset = HeaderSize + 8; // Base::tableOffset
|
|
+
|
|
+ // A backing allocation much larger than the buffer the parser will be
|
|
+ // given. Everything past the first originalBlobSize bytes is memory that
|
|
+ // a correct parser must never touch; planting known bytes there is what
|
|
+ // makes the out-of-bounds read observable rather than merely undefined.
|
|
+ // Pointing tableOffset past a tightly sized allocation instead would be a
|
|
+ // genuine heap-buffer-overflow, but it would only be diagnosed under a
|
|
+ // sanitizer and would otherwise crash the test run.
|
|
+ QByteArray buffer(4096, '\0');
|
|
+ Q_ASSERT(corruptedTableOffset + qsizetype(sizeof(quint32)) <= buffer.size());
|
|
+ // Copy the original blob at the beginning
|
|
+ memcpy(buffer.data(), originalBlob.constData(), originalBlobSize);
|
|
+
|
|
+ // Copy the other document's 4 byte table entry out of bounds.
|
|
+ const quint32 plantedBlobTableOffset =
|
|
+ qFromLittleEndian<quint32>(plantedBlob.constData() + TableOffsetOffset);
|
|
+ memcpy(buffer.data() + corruptedTableOffset,
|
|
+ plantedBlob.constData() + HeaderSize + plantedBlobTableOffset,
|
|
+ sizeof(quint32));
|
|
+
|
|
+ // fromBinaryData() computes the document bound as
|
|
+ // const uint size = sizeof(Header) + root.size;
|
|
+ // sizeof() is 64 bit while root.size is a 32 bit field taken straight from
|
|
+ // the input, so the sum is evaluated in 64 bits and then truncated on
|
|
+ // assignment to uint. Every root.size in [0xfffffff8, 0xffffffff] collapses
|
|
+ // to 0..7 and trivially passes the "size > uint(data.size())" check that is
|
|
+ // supposed to keep the root inside the buffer. ConstData::isValid() then
|
|
+ // recomputes maxSize as "alloc - sizeof(Header)", which underflows straight
|
|
+ // back to root.size, so the root ends up being validated against its own
|
|
+ // attacker-supplied length instead of against the real buffer.
|
|
+ //
|
|
+ // Once that happens nothing constrains tableOffset either: the guard
|
|
+ // "tableOffset + length() * sizeof(offset) > size" now compares against
|
|
+ // root.size instead of against the buffer, so both Array::isValid() and
|
|
+ // Array::toJsonArray() read the value table from 32 bytes past the end.
|
|
+
|
|
+ // Modify the size of the original blob
|
|
+ qToLittleEndian(corruptedSize, buffer.data() + RootSizeOffset);
|
|
+ // Modify the tableOffset to point to a newly-injected table
|
|
+ qToLittleEndian(quint32(corruptedTableOffset - HeaderSize),
|
|
+ buffer.data() + TableOffsetOffset);
|
|
+
|
|
+ // Hand the parser a view over the first origianlBlobSize bytes only.
|
|
+ // That size is the only bound it is given, so anything it reads beyond it
|
|
+ // is out of bounds.
|
|
+ const QByteArray nonOwningArray =
|
|
+ QByteArray::fromRawData(buffer.constData(), originalBlobSize);
|
|
+ QCOMPARE(nonOwningArray.size(), originalBlobSize);
|
|
+
|
|
+ // The code below passes a correct size to fromRawData() call.
|
|
+ // Surely we could observe the same problem, if we passed corruptedSize
|
|
+ // instead, but that's a typical problem for all APIs that take a pointer
|
|
+ // and a size.
|
|
+ const QJsonDocument doc = (callType == CallType::Binary)
|
|
+ ? QBinaryJson::fromBinaryData(nonOwningArray)
|
|
+ : QBinaryJson::fromRawData(nonOwningArray.constData(), int(nonOwningArray.size()));
|
|
+ if (!doc.isNull()) {
|
|
+ QFAIL(qPrintable(QString::asprintf(
|
|
+ "fromBinaryData() was handed %lld bytes but read the root array's value "
|
|
+ "table from offset %lld: it returned %s, which was planted out of bounds, "
|
|
+ "instead of rejecting the document",
|
|
+ qlonglong(nonOwningArray.size()), qlonglong(corruptedTableOffset),
|
|
+ doc.toJson(QJsonDocument::Compact).simplified().constData())));
|
|
+ }
|
|
+
|
|
+ // Check that the original blob can still be parsed correctly
|
|
+ QCOMPARE(QBinaryJson::fromBinaryData(originalBlob), original);
|
|
+}
|
|
+
|
|
QTEST_MAIN(tst_QtJson)
|
|
#include "tst_qtjson.moc"
|