[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>
Gbp-Pq: Name cve-2023-37369.diff

diff --git a/src/corelib/serialization/qxmlstream.cpp b/src/corelib/serialization/qxmlstream.cpp
index b76243c1760fe43b52487e91f0736e5ffe5ea53d..adb57a02398dc753ab47ae22ecbcba80be58d1cc 100644 (file)
--- a/src/corelib/serialization/qxmlstream.cpp
+++ b/src/corelib/serialization/qxmlstream.cpp
@@ -1243,7 +1243,10 @@ inline qsizetype QXmlStreamReaderPrivate::fastScanContentCharList()
     return n;
 }
 
-inline qsizetype QXmlStreamReaderPrivate::fastScanName(qint16 *prefix)
+// Fast scan an XML attribute name (e.g. "xml:lang").
+inline QXmlStreamReaderPrivate::FastScanNameResult
+QXmlStreamReaderPrivate::fastScanName(Value *val)
+
 {
     qsizetype n = 0;
     uint c;
@@ -1251,7 +1254,8 @@ inline qsizetype QXmlStreamReaderPrivate::fastScanName(qint16 *prefix)
         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':
@@ -1280,23 +1284,24 @@ inline qsizetype QXmlStreamReaderPrivate::fastScanName(qint16 *prefix)
         case '+':
         case '*':
             putChar(c);
-            if (prefix && *prefix == n+1) {
-                *prefix = 0;
+            if (val && val->prefix == n + 1) {
+                val->prefix = 0;
+
                 putChar(':');
                 --n;
             }
-            return n;
+            return FastScanNameResult(n);
         case ':':
-            if (prefix) {
-                if (*prefix == 0) {
-                    *prefix = qint16(n + 2);
+            if (val) {
+                if (val->prefix == 0) {
+                    val->prefix = 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:
@@ -1305,12 +1310,13 @@ inline qsizetype QXmlStreamReaderPrivate::fastScanName(qint16 *prefix)
         }
     }
 
-    if (prefix)
-        *prefix = 0;
+    if (val)
+        val->prefix = 0;
+
     qsizetype pos = textBuffer.size() - n;
     putString(textBuffer, pos);
     textBuffer.resize(pos);
-    return 0;
+    return FastScanNameResult(0);
 }
 
 enum NameChar { NameBeginning, NameNotBeginning, NotName };
@@ -1791,6 +1797,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 d06c371eb87cddeaeb76de9650e48d4968cb10da..fc122e668118d828a60c96b216c3345e09dffc1e 100644 (file)
--- a/src/corelib/serialization/qxmlstream.g
+++ b/src/corelib/serialization/qxmlstream.g
@@ -1419,7 +1419,12 @@ space_opt ::= space;
 qname ::= LETTER;
 /.
         case $rule_number: {
-            sym(1).len += fastScanName(&sym(1).prefix);
+            Value &val = sym(1);
+            if (auto res = fastScanName(&val))
+                val.len += *res;
+            else
+                return false;
+
             if (atEnd) {
                 resume($rule_number);
                 return false;
@@ -1430,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 1fd69a2c1f8718796053dd9e702bcc5396121c73..fba6aeb496b50db3c9b8d11c47bd5adf42982d7c 100644 (file)
--- 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)
@@ -471,7 +471,16 @@ public:
     qsizetype fastScanLiteralContent();
     qsizetype fastScanSpace();
     qsizetype fastScanContentCharList();
-    qsizetype fastScanName(qint16 *prefix = 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();
 
 
@@ -480,6 +489,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 e3ae6faa442d72d14966b1510775685bcb104cf8..afd83381b3fe227b4ba42aa0661eb9741b0f2c84 100644 (file)
--- a/src/corelib/serialization/qxmlstreamparser_p.h
+++ b/src/corelib/serialization/qxmlstreamparser_p.h
@@ -947,7 +947,12 @@ bool QXmlStreamReaderPrivate::parse()
         break;
 
         case 262: {
-            sym(1).len += fastScanName(&sym(1).prefix);
+            Value &val = sym(1);
+            if (auto res = fastScanName(&val))
+                val.len += *res;
+            else
+                return false;
+
             if (atEnd) {
                 resume(262);
                 return false;
@@ -955,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 2799e7a999b0bae2a67237b589a84752d147a78e..7eb0aac5cc8023fb9fcbd5d314b2357eb8420ef4 100644 (file)
--- 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;
 
@@ -1753,5 +1755,42 @@ 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);
+}
+
 #include "tst_qxmlstream.moc"
 // vim: et:ts=4:sw=4:sts=4