0013-QXmlStreamReader-make-fastScanName-indicate-parsing-.patch 10 KB

123456789101112131415161718192021222324252627282930313233343536373839404142434445464748495051525354555657585960616263646566676869707172737475767778798081828384858687888990919293949596979899100101102103104105106107108109110111112113114115116117118119120121122123124125126127128129130131132133134135136137138139140141142143144145146147148149150151152153154155156157158159160161162163164165166167168169170171172173174175176177178179180181182183184185186187188189190191192193194195196197198199200201202203204205206207208209210211212213214215216217218219220221222223224225226227228229230231232233234235236237238239240241242243244245246247248249250251252253254255256257258259260261262263264265266267268269270271272273274275276277
  1. From 87f0d5c9212df3087b053a36165453f10205924d Mon Sep 17 00:00:00 2001
  2. From: Ahmad Samir <a.samirh78@gmail.com>
  3. Date: Thu, 22 Jun 2023 15:56:07 +0300
  4. Subject: [PATCH] QXmlStreamReader: make fastScanName() indicate parsing status
  5. to callers
  6. This fixes a crash while parsing an XML file with garbage data, the file
  7. starts with '<' then garbage data:
  8. - The loop in the parse() keeps iterating until it hits "case 262:",
  9. which calls fastScanName()
  10. - fastScanName() iterates over the text buffer scanning for the
  11. attribute name (e.g. "xml:lang"), until it finds ':'
  12. - Consider a Value val, fastScanName() is called on it, it would set
  13. val.prefix to a number > val.len, then it would hit the 4096 condition
  14. and return (returned 0, now it returns the equivalent of
  15. std::null_opt), which means that val.len doesn't get modified, making
  16. it smaller than val.prefix
  17. - The code would try constructing an XmlStringRef with negative length,
  18. which would hit an assert in one of QStringView's constructors
  19. Add an assert to the XmlStringRef constructor.
  20. Add unittest based on the file from the bug report.
  21. Later on I will replace FastScanNameResult with std::optional<qsizetype>
  22. (std::optional is C++17, which isn't required by Qt 5.15, and we want to
  23. backport this fix).
  24. Credit to OSS-Fuzz.
  25. Fixes: QTBUG-109781
  26. Fixes: QTBUG-114829
  27. Pick-to: 6.6 6.5 6.2 5.15
  28. Change-Id: I455a5eeb47870c2ac9ffd0cbcdcd99c1ae2dd374
  29. Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>
  30. Fixes: https://security-tracker.debian.org/tracker/CVE-2023-37369
  31. Upstream: https://github.com/qt/qtbase/commit/6326bec46a618c72feba4a2bb994c4d475050aed
  32. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
  33. ---
  34. src/corelib/serialization/qxmlstream.cpp | 23 ++++++++---
  35. src/corelib/serialization/qxmlstream.g | 12 +++++-
  36. src/corelib/serialization/qxmlstream_p.h | 14 ++++++-
  37. .../serialization/qxmlstreamparser_p.h | 12 +++++-
  38. .../qxmlstream/tst_qxmlstream.cpp | 39 +++++++++++++++++++
  39. 5 files changed, 88 insertions(+), 12 deletions(-)
  40. diff --git a/src/corelib/serialization/qxmlstream.cpp b/src/corelib/serialization/qxmlstream.cpp
  41. index 466f456ee63..2c879fb4c20 100644
  42. --- a/src/corelib/serialization/qxmlstream.cpp
  43. +++ b/src/corelib/serialization/qxmlstream.cpp
  44. @@ -1247,7 +1247,9 @@ inline qsizetype QXmlStreamReaderPrivate::fastScanContentCharList()
  45. return n;
  46. }
  47. -inline qsizetype QXmlStreamReaderPrivate::fastScanName(Value *val)
  48. +// Fast scan an XML attribute name (e.g. "xml:lang").
  49. +inline QXmlStreamReaderPrivate::FastScanNameResult
  50. +QXmlStreamReaderPrivate::fastScanName(Value *val)
  51. {
  52. qsizetype n = 0;
  53. uint c;
  54. @@ -1255,7 +1257,8 @@ inline qsizetype QXmlStreamReaderPrivate::fastScanName(Value *val)
  55. if (n >= 4096) {
  56. // This is too long to be a sensible name, and
  57. // can exhaust memory, or the range of decltype(*prefix)
  58. - return 0;
  59. + raiseNamePrefixTooLongError();
  60. + return {};
  61. }
  62. switch (c) {
  63. case '\n':
  64. @@ -1289,18 +1292,18 @@ inline qsizetype QXmlStreamReaderPrivate::fastScanName(Value *val)
  65. putChar(':');
  66. --n;
  67. }
  68. - return n;
  69. + return FastScanNameResult(n);
  70. case ':':
  71. if (val) {
  72. if (val->prefix == 0) {
  73. val->prefix = qint16(n + 2);
  74. } else { // only one colon allowed according to the namespace spec.
  75. putChar(c);
  76. - return n;
  77. + return FastScanNameResult(n);
  78. }
  79. } else {
  80. putChar(c);
  81. - return n;
  82. + return FastScanNameResult(n);
  83. }
  84. Q_FALLTHROUGH();
  85. default:
  86. @@ -1314,7 +1317,7 @@ inline qsizetype QXmlStreamReaderPrivate::fastScanName(Value *val)
  87. qsizetype pos = textBuffer.size() - n;
  88. putString(textBuffer, pos);
  89. textBuffer.resize(pos);
  90. - return 0;
  91. + return FastScanNameResult(0);
  92. }
  93. enum NameChar { NameBeginning, NameNotBeginning, NotName };
  94. @@ -1795,6 +1798,14 @@ void QXmlStreamReaderPrivate::raiseWellFormedError(const QString &message)
  95. raiseError(QXmlStreamReader::NotWellFormedError, message);
  96. }
  97. +void QXmlStreamReaderPrivate::raiseNamePrefixTooLongError()
  98. +{
  99. + // TODO: add a ImplementationLimitsExceededError and use it instead
  100. + raiseError(QXmlStreamReader::NotWellFormedError,
  101. + QXmlStream::tr("Length of XML attribute name exceeds implemnetation limits (4KiB "
  102. + "characters)."));
  103. +}
  104. +
  105. void QXmlStreamReaderPrivate::parseError()
  106. {
  107. diff --git a/src/corelib/serialization/qxmlstream.g b/src/corelib/serialization/qxmlstream.g
  108. index f3152bff378..fc122e66811 100644
  109. --- a/src/corelib/serialization/qxmlstream.g
  110. +++ b/src/corelib/serialization/qxmlstream.g
  111. @@ -1420,7 +1420,11 @@ qname ::= LETTER;
  112. /.
  113. case $rule_number: {
  114. Value &val = sym(1);
  115. - val.len += fastScanName(&val);
  116. + if (auto res = fastScanName(&val))
  117. + val.len += *res;
  118. + else
  119. + return false;
  120. +
  121. if (atEnd) {
  122. resume($rule_number);
  123. return false;
  124. @@ -1431,7 +1435,11 @@ qname ::= LETTER;
  125. name ::= LETTER;
  126. /.
  127. case $rule_number:
  128. - sym(1).len += fastScanName();
  129. + if (auto res = fastScanName())
  130. + sym(1).len += *res;
  131. + else
  132. + return false;
  133. +
  134. if (atEnd) {
  135. resume($rule_number);
  136. return false;
  137. diff --git a/src/corelib/serialization/qxmlstream_p.h b/src/corelib/serialization/qxmlstream_p.h
  138. index efee742963b..be69921c7c3 100644
  139. --- a/src/corelib/serialization/qxmlstream_p.h
  140. +++ b/src/corelib/serialization/qxmlstream_p.h
  141. @@ -38,7 +38,7 @@ public:
  142. constexpr XmlStringRef() = default;
  143. constexpr inline XmlStringRef(const QString *string, qsizetype pos, qsizetype length)
  144. - : m_string(string), m_pos(pos), m_size(length)
  145. + : m_string(string), m_pos(pos), m_size((Q_ASSERT(length >= 0), length))
  146. {
  147. }
  148. XmlStringRef(const QString *string)
  149. @@ -482,7 +482,16 @@ public:
  150. qsizetype fastScanLiteralContent();
  151. qsizetype fastScanSpace();
  152. qsizetype fastScanContentCharList();
  153. - qsizetype fastScanName(Value *val = nullptr);
  154. +
  155. + struct FastScanNameResult {
  156. + FastScanNameResult() : ok(false) {}
  157. + explicit FastScanNameResult(qsizetype len) : addToLen(len), ok(true) { }
  158. + operator bool() { return ok; }
  159. + qsizetype operator*() { Q_ASSERT(ok); return addToLen; }
  160. + qsizetype addToLen;
  161. + bool ok;
  162. + };
  163. + FastScanNameResult fastScanName(Value *val = nullptr);
  164. inline qsizetype fastScanNMTOKEN();
  165. @@ -491,6 +500,7 @@ public:
  166. void raiseError(QXmlStreamReader::Error error, const QString& message = QString());
  167. void raiseWellFormedError(const QString &message);
  168. + void raiseNamePrefixTooLongError();
  169. QXmlStreamEntityResolver *entityResolver;
  170. diff --git a/src/corelib/serialization/qxmlstreamparser_p.h b/src/corelib/serialization/qxmlstreamparser_p.h
  171. index 59370a93106..afd83381b3f 100644
  172. --- a/src/corelib/serialization/qxmlstreamparser_p.h
  173. +++ b/src/corelib/serialization/qxmlstreamparser_p.h
  174. @@ -948,7 +948,11 @@ bool QXmlStreamReaderPrivate::parse()
  175. case 262: {
  176. Value &val = sym(1);
  177. - val.len += fastScanName(&val);
  178. + if (auto res = fastScanName(&val))
  179. + val.len += *res;
  180. + else
  181. + return false;
  182. +
  183. if (atEnd) {
  184. resume(262);
  185. return false;
  186. @@ -956,7 +960,11 @@ bool QXmlStreamReaderPrivate::parse()
  187. } break;
  188. case 263:
  189. - sym(1).len += fastScanName();
  190. + if (auto res = fastScanName())
  191. + sym(1).len += *res;
  192. + else
  193. + return false;
  194. +
  195. if (atEnd) {
  196. resume(263);
  197. return false;
  198. diff --git a/tests/auto/corelib/serialization/qxmlstream/tst_qxmlstream.cpp b/tests/auto/corelib/serialization/qxmlstream/tst_qxmlstream.cpp
  199. index 8d2738700ea..ac41528e2a8 100644
  200. --- a/tests/auto/corelib/serialization/qxmlstream/tst_qxmlstream.cpp
  201. +++ b/tests/auto/corelib/serialization/qxmlstream/tst_qxmlstream.cpp
  202. @@ -581,6 +581,8 @@ private slots:
  203. void readBack() const;
  204. void roundTrip() const;
  205. void roundTrip_data() const;
  206. + void test_fastScanName_data() const;
  207. + void test_fastScanName() const;
  208. void entityExpansionLimit() const;
  209. @@ -1757,6 +1759,43 @@ void tst_QXmlStream::roundTrip() const
  210. QCOMPARE(out, in);
  211. }
  212. +void tst_QXmlStream::test_fastScanName_data() const
  213. +{
  214. + QTest::addColumn<QByteArray>("data");
  215. + QTest::addColumn<QXmlStreamReader::Error>("errorType");
  216. +
  217. + // 4096 is the limit in QXmlStreamReaderPrivate::fastScanName()
  218. +
  219. + QByteArray arr = "<a"_ba + ":" + QByteArray("b").repeated(4096 - 1);
  220. + QTest::newRow("data1") << arr << QXmlStreamReader::PrematureEndOfDocumentError;
  221. +
  222. + arr = "<a"_ba + ":" + QByteArray("b").repeated(4096);
  223. + QTest::newRow("data2") << arr << QXmlStreamReader::NotWellFormedError;
  224. +
  225. + arr = "<"_ba + QByteArray("a").repeated(4000) + ":" + QByteArray("b").repeated(96);
  226. + QTest::newRow("data3") << arr << QXmlStreamReader::PrematureEndOfDocumentError;
  227. +
  228. + arr = "<"_ba + QByteArray("a").repeated(4000) + ":" + QByteArray("b").repeated(96 + 1);
  229. + QTest::newRow("data4") << arr << QXmlStreamReader::NotWellFormedError;
  230. +
  231. + arr = "<"_ba + QByteArray("a").repeated(4000 + 1) + ":" + QByteArray("b").repeated(96);
  232. + QTest::newRow("data5") << arr << QXmlStreamReader::NotWellFormedError;
  233. +}
  234. +
  235. +void tst_QXmlStream::test_fastScanName() const
  236. +{
  237. + QFETCH(QByteArray, data);
  238. + QFETCH(QXmlStreamReader::Error, errorType);
  239. +
  240. + QXmlStreamReader reader(data);
  241. + QXmlStreamReader::TokenType tokenType;
  242. + while (!reader.atEnd())
  243. + tokenType = reader.readNext();
  244. +
  245. + QCOMPARE(tokenType, QXmlStreamReader::Invalid);
  246. + QCOMPARE(reader.error(), errorType);
  247. +}
  248. +
  249. void tst_QXmlStream::tokenErrorHandling_data() const
  250. {
  251. QTest::addColumn<QString>("fileName");
  252. --
  253. 2.46.0