123456789101112131415161718192021222324252627282930313233343536373839404142434445464748495051525354555657585960616263646566676869707172737475767778798081828384858687888990919293949596979899100101102103104105106107108109110111112113114115116117118119120121122123124125126127128129130131132133134135136137138139140141142143144145146147148149150151152153154155156157158159160161162163164165166167168169170171172173174175176177178179180181182183184185186187188189190191192193194195196197198199200201202203204205206207208209210211212213214215216217218219220221222223224225226227228229230231232233234235236237238239240241242243244245246247248249250251252253254255256257258259260261262263264265266267268269270271272273274275276277 |
- From 87f0d5c9212df3087b053a36165453f10205924d Mon Sep 17 00:00:00 2001
- From: Ahmad Samir <a.samirh78@gmail.com>
- Date: Thu, 22 Jun 2023 15:56:07 +0300
- Subject: [PATCH] QXmlStreamReader: make fastScanName() indicate parsing status
- to callers
- This fixes a crash while parsing an XML file with garbage data, the file
- starts with '<' then garbage data:
- - The loop in the parse() keeps iterating until it hits "case 262:",
- which calls fastScanName()
- - fastScanName() iterates over the text buffer scanning for the
- attribute name (e.g. "xml:lang"), until it finds ':'
- - Consider a Value val, fastScanName() is called on it, it would set
- val.prefix to a number > val.len, then it would hit the 4096 condition
- and return (returned 0, now it returns the equivalent of
- std::null_opt), which means that val.len doesn't get modified, making
- it smaller than val.prefix
- - The code would try constructing an XmlStringRef with negative length,
- which would hit an assert in one of QStringView's constructors
- Add an assert to the XmlStringRef constructor.
- Add unittest based on the file from the bug report.
- Later on I will replace FastScanNameResult with std::optional<qsizetype>
- (std::optional is C++17, which isn't required by Qt 5.15, and we want to
- backport this fix).
- Credit to OSS-Fuzz.
- Fixes: QTBUG-109781
- Fixes: QTBUG-114829
- Pick-to: 6.6 6.5 6.2 5.15
- Change-Id: I455a5eeb47870c2ac9ffd0cbcdcd99c1ae2dd374
- Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>
- Fixes: https://security-tracker.debian.org/tracker/CVE-2023-37369
- Upstream: https://github.com/qt/qtbase/commit/6326bec46a618c72feba4a2bb994c4d475050aed
- Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
- ---
- src/corelib/serialization/qxmlstream.cpp | 23 ++++++++---
- src/corelib/serialization/qxmlstream.g | 12 +++++-
- src/corelib/serialization/qxmlstream_p.h | 14 ++++++-
- .../serialization/qxmlstreamparser_p.h | 12 +++++-
- .../qxmlstream/tst_qxmlstream.cpp | 39 +++++++++++++++++++
- 5 files changed, 88 insertions(+), 12 deletions(-)
- diff --git a/src/corelib/serialization/qxmlstream.cpp b/src/corelib/serialization/qxmlstream.cpp
- index 466f456ee63..2c879fb4c20 100644
- --- a/src/corelib/serialization/qxmlstream.cpp
- +++ b/src/corelib/serialization/qxmlstream.cpp
- @@ -1247,7 +1247,9 @@ inline qsizetype QXmlStreamReaderPrivate::fastScanContentCharList()
- return n;
- }
-
- -inline qsizetype QXmlStreamReaderPrivate::fastScanName(Value *val)
- +// Fast scan an XML attribute name (e.g. "xml:lang").
- +inline QXmlStreamReaderPrivate::FastScanNameResult
- +QXmlStreamReaderPrivate::fastScanName(Value *val)
- {
- qsizetype n = 0;
- uint c;
- @@ -1255,7 +1257,8 @@ inline qsizetype QXmlStreamReaderPrivate::fastScanName(Value *val)
- if (n >= 4096) {
- // This is too long to be a sensible name, and
- // can exhaust memory, or the range of decltype(*prefix)
- - return 0;
- + raiseNamePrefixTooLongError();
- + return {};
- }
- switch (c) {
- case '\n':
- @@ -1289,18 +1292,18 @@ inline qsizetype QXmlStreamReaderPrivate::fastScanName(Value *val)
- putChar(':');
- --n;
- }
- - return n;
- + return FastScanNameResult(n);
- case ':':
- if (val) {
- if (val->prefix == 0) {
- val->prefix = qint16(n + 2);
- } else { // only one colon allowed according to the namespace spec.
- putChar(c);
- - return n;
- + return FastScanNameResult(n);
- }
- } else {
- putChar(c);
- - return n;
- + return FastScanNameResult(n);
- }
- Q_FALLTHROUGH();
- default:
- @@ -1314,7 +1317,7 @@ inline qsizetype QXmlStreamReaderPrivate::fastScanName(Value *val)
- qsizetype pos = textBuffer.size() - n;
- putString(textBuffer, pos);
- textBuffer.resize(pos);
- - return 0;
- + return FastScanNameResult(0);
- }
-
- enum NameChar { NameBeginning, NameNotBeginning, NotName };
- @@ -1795,6 +1798,14 @@ void QXmlStreamReaderPrivate::raiseWellFormedError(const QString &message)
- raiseError(QXmlStreamReader::NotWellFormedError, message);
- }
-
- +void QXmlStreamReaderPrivate::raiseNamePrefixTooLongError()
- +{
- + // TODO: add a ImplementationLimitsExceededError and use it instead
- + raiseError(QXmlStreamReader::NotWellFormedError,
- + QXmlStream::tr("Length of XML attribute name exceeds implemnetation limits (4KiB "
- + "characters)."));
- +}
- +
- void QXmlStreamReaderPrivate::parseError()
- {
-
- diff --git a/src/corelib/serialization/qxmlstream.g b/src/corelib/serialization/qxmlstream.g
- index f3152bff378..fc122e66811 100644
- --- a/src/corelib/serialization/qxmlstream.g
- +++ b/src/corelib/serialization/qxmlstream.g
- @@ -1420,7 +1420,11 @@ qname ::= LETTER;
- /.
- case $rule_number: {
- Value &val = sym(1);
- - val.len += fastScanName(&val);
- + if (auto res = fastScanName(&val))
- + val.len += *res;
- + else
- + return false;
- +
- if (atEnd) {
- resume($rule_number);
- return false;
- @@ -1431,7 +1435,11 @@ qname ::= LETTER;
- name ::= LETTER;
- /.
- case $rule_number:
- - sym(1).len += fastScanName();
- + if (auto res = fastScanName())
- + sym(1).len += *res;
- + else
- + return false;
- +
- if (atEnd) {
- resume($rule_number);
- return false;
- diff --git a/src/corelib/serialization/qxmlstream_p.h b/src/corelib/serialization/qxmlstream_p.h
- index efee742963b..be69921c7c3 100644
- --- a/src/corelib/serialization/qxmlstream_p.h
- +++ b/src/corelib/serialization/qxmlstream_p.h
- @@ -38,7 +38,7 @@ public:
-
- constexpr XmlStringRef() = default;
- constexpr inline XmlStringRef(const QString *string, qsizetype pos, qsizetype length)
- - : m_string(string), m_pos(pos), m_size(length)
- + : m_string(string), m_pos(pos), m_size((Q_ASSERT(length >= 0), length))
- {
- }
- XmlStringRef(const QString *string)
- @@ -482,7 +482,16 @@ public:
- qsizetype fastScanLiteralContent();
- qsizetype fastScanSpace();
- qsizetype fastScanContentCharList();
- - qsizetype fastScanName(Value *val = nullptr);
- +
- + struct FastScanNameResult {
- + FastScanNameResult() : ok(false) {}
- + explicit FastScanNameResult(qsizetype len) : addToLen(len), ok(true) { }
- + operator bool() { return ok; }
- + qsizetype operator*() { Q_ASSERT(ok); return addToLen; }
- + qsizetype addToLen;
- + bool ok;
- + };
- + FastScanNameResult fastScanName(Value *val = nullptr);
- inline qsizetype fastScanNMTOKEN();
-
-
- @@ -491,6 +500,7 @@ public:
-
- void raiseError(QXmlStreamReader::Error error, const QString& message = QString());
- void raiseWellFormedError(const QString &message);
- + void raiseNamePrefixTooLongError();
-
- QXmlStreamEntityResolver *entityResolver;
-
- diff --git a/src/corelib/serialization/qxmlstreamparser_p.h b/src/corelib/serialization/qxmlstreamparser_p.h
- index 59370a93106..afd83381b3f 100644
- --- a/src/corelib/serialization/qxmlstreamparser_p.h
- +++ b/src/corelib/serialization/qxmlstreamparser_p.h
- @@ -948,7 +948,11 @@ bool QXmlStreamReaderPrivate::parse()
-
- case 262: {
- Value &val = sym(1);
- - val.len += fastScanName(&val);
- + if (auto res = fastScanName(&val))
- + val.len += *res;
- + else
- + return false;
- +
- if (atEnd) {
- resume(262);
- return false;
- @@ -956,7 +960,11 @@ bool QXmlStreamReaderPrivate::parse()
- } break;
-
- case 263:
- - sym(1).len += fastScanName();
- + if (auto res = fastScanName())
- + sym(1).len += *res;
- + else
- + return false;
- +
- if (atEnd) {
- resume(263);
- return false;
- diff --git a/tests/auto/corelib/serialization/qxmlstream/tst_qxmlstream.cpp b/tests/auto/corelib/serialization/qxmlstream/tst_qxmlstream.cpp
- index 8d2738700ea..ac41528e2a8 100644
- --- a/tests/auto/corelib/serialization/qxmlstream/tst_qxmlstream.cpp
- +++ b/tests/auto/corelib/serialization/qxmlstream/tst_qxmlstream.cpp
- @@ -581,6 +581,8 @@ private slots:
- void readBack() const;
- void roundTrip() const;
- void roundTrip_data() const;
- + void test_fastScanName_data() const;
- + void test_fastScanName() const;
-
- void entityExpansionLimit() const;
-
- @@ -1757,6 +1759,43 @@ void tst_QXmlStream::roundTrip() const
- QCOMPARE(out, in);
- }
-
- +void tst_QXmlStream::test_fastScanName_data() const
- +{
- + QTest::addColumn<QByteArray>("data");
- + QTest::addColumn<QXmlStreamReader::Error>("errorType");
- +
- + // 4096 is the limit in QXmlStreamReaderPrivate::fastScanName()
- +
- + QByteArray arr = "<a"_ba + ":" + QByteArray("b").repeated(4096 - 1);
- + QTest::newRow("data1") << arr << QXmlStreamReader::PrematureEndOfDocumentError;
- +
- + arr = "<a"_ba + ":" + QByteArray("b").repeated(4096);
- + QTest::newRow("data2") << arr << QXmlStreamReader::NotWellFormedError;
- +
- + arr = "<"_ba + QByteArray("a").repeated(4000) + ":" + QByteArray("b").repeated(96);
- + QTest::newRow("data3") << arr << QXmlStreamReader::PrematureEndOfDocumentError;
- +
- + arr = "<"_ba + QByteArray("a").repeated(4000) + ":" + QByteArray("b").repeated(96 + 1);
- + QTest::newRow("data4") << arr << QXmlStreamReader::NotWellFormedError;
- +
- + arr = "<"_ba + QByteArray("a").repeated(4000 + 1) + ":" + QByteArray("b").repeated(96);
- + QTest::newRow("data5") << arr << QXmlStreamReader::NotWellFormedError;
- +}
- +
- +void tst_QXmlStream::test_fastScanName() const
- +{
- + QFETCH(QByteArray, data);
- + QFETCH(QXmlStreamReader::Error, errorType);
- +
- + QXmlStreamReader reader(data);
- + QXmlStreamReader::TokenType tokenType;
- + while (!reader.atEnd())
- + tokenType = reader.readNext();
- +
- + QCOMPARE(tokenType, QXmlStreamReader::Invalid);
- + QCOMPARE(reader.error(), errorType);
- +}
- +
- void tst_QXmlStream::tokenErrorHandling_data() const
- {
- QTest::addColumn<QString>("fileName");
- --
- 2.46.0
|