From db13a82e625e02232daed6ccfd29ed452e1fa1a3 Mon Sep 17 00:00:00 2001 From: Lee Thomason Date: Sat, 28 Jul 2018 14:56:20 -0700 Subject: [PATCH] fix huge number of declaration security issue --- resources/xmltest-5662204197076992.xml | 1 + tinyxml2.cpp | 28 +++++++++++++++++--------- xmltest.cpp | 12 +++++++++++ 3 files changed, 32 insertions(+), 9 deletions(-) create mode 100644 resources/xmltest-5662204197076992.xml diff --git a/resources/xmltest-5662204197076992.xml b/resources/xmltest-5662204197076992.xml new file mode 100644 index 0000000..71325d3 --- /dev/null +++ b/resources/xmltest-5662204197076992.xml @@ -0,0 +1 @@ + \ No newline at end of file diff --git a/tinyxml2.cpp b/tinyxml2.cpp index de3d2fb..fd27f78 100755 --- a/tinyxml2.cpp +++ b/tinyxml2.cpp @@ -1032,15 +1032,25 @@ char* XMLNode::ParseDeep( char* p, StrPair* parentEndTag, int* curLineNumPtr ) XMLDeclaration* decl = node->ToDeclaration(); if ( decl ) { // Declarations are only allowed at document level - bool wellLocated = ( ToDocument() != 0 ); - if ( wellLocated ) { - // Multiple declarations are allowed but all declarations - // must occur before anything else - for ( const XMLNode* existingNode = _document->FirstChild(); existingNode; existingNode = existingNode->NextSibling() ) { - if ( !existingNode->ToDeclaration() ) { - wellLocated = false; - break; - } + // + // Multiple declarations are allowed but all declarations + // must occur before anything else. + // + // Optimized due to a security test case. If the first node is + // a declaration, and the last node is a declaration, then only + // declarations have so far been addded. + bool wellLocated = false; + + if (ToDocument()) { + if (FirstChild()) { + wellLocated = + FirstChild() && + FirstChild()->ToDeclaration() && + LastChild() && + LastChild()->ToDeclaration(); + } + else { + wellLocated = true; } } if ( !wellLocated ) { diff --git a/xmltest.cpp b/xmltest.cpp index a7dbc25..ac0d85c 100644 --- a/xmltest.cpp +++ b/xmltest.cpp @@ -2050,6 +2050,18 @@ int main( int argc, const char ** argv ) XMLTest("Stack overflow prevented.", XML_ELEMENT_DEPTH_EXCEEDED, doc.ErrorID()); } } + { + const char* TESTS[] = { + "./resources/xmltest-5662204197076992.xml", // Security-level performance issue. + 0 + }; + for (int i = 0; TESTS[i]; ++i) { + XMLDocument doc; + doc.LoadFile(TESTS[i]); + // Need only not crash / lock up. + XMLTest("Fuzz attack prevented.", true, true); + } + } { // Crashing reported via email. const char* xml =