From f432c08882ffebe5074ea28de871559a98a4d094 Mon Sep 17 00:00:00 2001 From: Lars Knoll Date: Wed, 26 Feb 2020 10:42:10 +0100 Subject: Add an expansion limit for entities Recursively defined entities can easily exhaust all available memory. Limit entity expansion to a default of 4096 characters to avoid DoS attacks when a user loads untrusted content. [ChangeLog][QtCore][QXmlStream] QXmlStreamReader does now limit the expansion of entities to 4096 characters. Documents where a single entity expands to more characters than the limit are not considered well formed. The limit is there to avoid DoS attacks through recursively expanding entities when loading untrusted content. Qt 5.15 will add methods that allow changing that limit. Fixes: QTBUG-47417 Change-Id: I94387815d74fcf34783e136387ee57fac5ded0c9 Reviewed-by: Oswald Buddenhagen Reviewed-by: Volker Hilsheimer (cherry picked from commit fd4be84d23a0db4186cb42e736a9de3af722c7f7) Reviewed-by: Eirik Aavitsland --- src/corelib/serialization/qxmlstream.g | 14 ++++++++++++- src/corelib/serialization/qxmlstream_p.h | 14 ++++++++++++- .../serialization/qxmlstream/tst_qxmlstream.cpp | 23 ++++++++++++++++++++-- 3 files changed, 47 insertions(+), 4 deletions(-) diff --git a/src/corelib/serialization/qxmlstream.g b/src/corelib/serialization/qxmlstream.g index 10bfcd491c..5726bafb26 100644 --- a/src/corelib/serialization/qxmlstream.g +++ b/src/corelib/serialization/qxmlstream.g @@ -277,9 +277,19 @@ public: QHash entityHash; QHash parameterEntityHash; QXmlStreamSimpleStackentityReferenceStack; + int entityExpansionLimit = 4096; + int entityLength = 0; inline bool referenceEntity(Entity &entity) { if (entity.isCurrentlyReferenced) { - raiseWellFormedError(QXmlStream::tr("Recursive entity detected.")); + raiseWellFormedError(QXmlStream::tr("Self-referencing entity detected.")); + return false; + } + // entityLength represents the amount of additional characters the + // entity expands into (can be negative for e.g. &). It's used to + // avoid DoS attacks through recursive entity expansions + entityLength += entity.value.size() - entity.name.size() - 2; + if (entityLength > entityExpansionLimit) { + raiseWellFormedError(QXmlStream::tr("Entity expands to more characters than the entity expansion limit.")); return false; } entity.isCurrentlyReferenced = true; @@ -830,6 +840,8 @@ entity_done ::= ENTITY_DONE; /. case $rule_number: entityReferenceStack.pop()->isCurrentlyReferenced = false; + if (entityReferenceStack.isEmpty()) + entityLength = 0; clearSym(); break; ./ diff --git a/src/corelib/serialization/qxmlstream_p.h b/src/corelib/serialization/qxmlstream_p.h index 61f501f81b..31053f8e0b 100644 --- a/src/corelib/serialization/qxmlstream_p.h +++ b/src/corelib/serialization/qxmlstream_p.h @@ -774,9 +774,19 @@ public: QHash entityHash; QHash parameterEntityHash; QXmlStreamSimpleStackentityReferenceStack; + int entityExpansionLimit = 4096; + int entityLength = 0; inline bool referenceEntity(Entity &entity) { if (entity.isCurrentlyReferenced) { - raiseWellFormedError(QXmlStream::tr("Recursive entity detected.")); + raiseWellFormedError(QXmlStream::tr("Self-referencing entity detected.")); + return false; + } + // entityLength represents the amount of additional characters the + // entity expands into (can be negative for e.g. &). It's used to + // avoid DoS attacks through recursive entity expansions + entityLength += entity.value.size() - entity.name.size() - 2; + if (entityLength > entityExpansionLimit) { + raiseWellFormedError(QXmlStream::tr("Entity expands to more characters than the entity expansion limit.")); return false; } entity.isCurrentlyReferenced = true; @@ -1308,6 +1318,8 @@ bool QXmlStreamReaderPrivate::parse() case 10: entityReferenceStack.pop()->isCurrentlyReferenced = false; + if (entityReferenceStack.isEmpty()) + entityLength = 0; clearSym(); break; diff --git a/tests/auto/corelib/serialization/qxmlstream/tst_qxmlstream.cpp b/tests/auto/corelib/serialization/qxmlstream/tst_qxmlstream.cpp index 8fdf91b090..1f9a0d575d 100644 --- a/tests/auto/corelib/serialization/qxmlstream/tst_qxmlstream.cpp +++ b/tests/auto/corelib/serialization/qxmlstream/tst_qxmlstream.cpp @@ -393,8 +393,6 @@ public: return true; } - QXmlStreamReader reader(&inputFile); - /* See testcases.dtd which reads: 'Nonvalidating parsers * must also accept "invalid" testcases, but validating ones must reject them.' */ if(type == QLatin1String("invalid") || type == QLatin1String("valid")) @@ -580,6 +578,8 @@ private slots: void roundTrip() const; void roundTrip_data() const; + void entityExpansionLimit() const; + private: static QByteArray readFile(const QString &filename); @@ -1756,6 +1756,25 @@ void tst_QXmlStream::roundTrip_data() const "\n"; } +void tst_QXmlStream::entityExpansionLimit() const +{ + QString xml = QStringLiteral("" + "" + "" + "" + "" + "]>" + "&d;&d;&d;"); + { + QXmlStreamReader reader(xml); + do { + reader.readNext(); + } while (!reader.atEnd()); + QCOMPARE(reader.error(), QXmlStreamReader::NotWellFormedError); + } +} + void tst_QXmlStream::roundTrip() const { QFETCH(QString, in); -- cgit v0.2.1