From 6326bec46a618c72feba4a2bb994c4d475050aed Mon Sep 17 00:00:00 2001 From: Ahmad Samir 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 (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 --- 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 24e9357d7b..e8999d6224 100644 --- a/src/corelib/serialization/qxmlstream.cpp +++ b/src/corelib/serialization/qxmlstream.cpp @@ -1296,7 +1296,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; @@ -1304,7 +1306,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': @@ -1338,18 +1341,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: @@ -1363,7 +1366,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 }; @@ -1841,6 +1844,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 8bde0e4319..30a6491d7d 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 8bc83327b4..ca8bb51ff7 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) @@ -498,7 +498,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(); @@ -507,6 +516,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 89a23b9b33..1363bf4d41 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 5e8533abef..46f8653fea 100644 --- a/tests/auto/corelib/serialization/qxmlstream/tst_qxmlstream.cpp +++ b/tests/auto/corelib/serialization/qxmlstream/tst_qxmlstream.cpp @@ -586,6 +586,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; @@ -1828,5 +1830,42 @@ void tst_QXmlStream::roundTrip() const QCOMPARE(out, in); } +void tst_QXmlStream::test_fastScanName_data() const +{ + QTest::addColumn("data"); + QTest::addColumn("errorType"); + + // 4096 is the limit in QXmlStreamReaderPrivate::fastScanName() + + QByteArray arr = "