Swiftdiff options
| author | Edwin Mons <edwin.mons@isode.com> | 2019-10-30 00:09:29 (GMT) |
|---|---|---|
| committer | Edwin Mons <edwin.mons@isode.com> | 2019-10-30 00:17:42 (GMT) |
| commit | a616265f7a5a48c5769262027795f99df91a6ae8 (patch) | |
| tree | ed7ebe35692083b599e58500a0067f7f14b6809e | |
| parent | 943f4cd11f35573e1af91be578cd058fac34b670 (diff) | |
| download | swift-a616265f7a5a48c5769262027795f99df91a6ae8.zip swift-a616265f7a5a48c5769262027795f99df91a6ae8.tar.bz2 | |
Fix libxml2 crash on certain invalid input
When the libxml2 parser is fed data with an odd combination of invalid
input (triggering the parser to assume 2 or 4 byte encodings were in
play), I/O errors might occur. In that case, the parser context will
not have its internal error set, but the call to xmlParseChunk will
return the right error. The parse() method now uses the output of
xmlParseChunk directly instead of trying to obtain the error from the
parser context afterwards.
Encoding errors during parsing were emitted to stderr because the
default error handlers were still in place. These have been replaced
with custom handlers that suppress the output.
Test-Information:
Unit tests pass on Debian 9
Change-Id: Ie01db4be467e5197203c9d07d3356f5d44d8b9b4
| -rw-r--r-- | Swiften/Parser/LibXMLParser.cpp | 14 | ||||
| -rw-r--r-- | Swiften/Parser/UnitTest/XMLParserTest.cpp | 15 |
2 files changed, 26 insertions, 3 deletions
diff --git a/Swiften/Parser/LibXMLParser.cpp b/Swiften/Parser/LibXMLParser.cpp index b8d941c..158958b 100644 --- a/Swiften/Parser/LibXMLParser.cpp +++ b/Swiften/Parser/LibXMLParser.cpp | |||
| @@ -100,12 +100,20 @@ static void handleError(void*, const char* /*m*/, ... ) { | |||
| 100 | static void handleWarning(void*, const char*, ... ) { | 100 | static void handleWarning(void*, const char*, ... ) { |
| 101 | } | 101 | } |
| 102 | 102 | ||
| 103 | static void handleGenericError(void*, const char*, ... ) { | ||
| 104 | } | ||
| 105 | |||
| 106 | static void handleStructuredError(void*, xmlErrorPtr) { | ||
| 107 | } | ||
| 108 | |||
| 103 | bool LibXMLParser::initialized = false; | 109 | bool LibXMLParser::initialized = false; |
| 104 | 110 | ||
| 105 | LibXMLParser::LibXMLParser(XMLParserClient* client, bool allowComments) : XMLParser(client, allowComments), p(new Private()) { | 111 | LibXMLParser::LibXMLParser(XMLParserClient* client, bool allowComments) : XMLParser(client, allowComments), p(new Private()) { |
| 106 | // Initialize libXML for multithreaded applications | 112 | // Initialize libXML for multithreaded applications |
| 107 | if (!initialized) { | 113 | if (!initialized) { |
| 108 | xmlInitParser(); | 114 | xmlInitParser(); |
| 115 | xmlSetGenericErrorFunc(nullptr, handleGenericError); | ||
| 116 | xmlSetStructuredErrorFunc(nullptr, handleStructuredError); | ||
| 109 | initialized = true; | 117 | initialized = true; |
| 110 | } | 118 | } |
| 111 | 119 | ||
| @@ -136,12 +144,12 @@ bool LibXMLParser::parse(const std::string& data, bool finalData) { | |||
| 136 | if (data.size() > std::numeric_limits<int>::max()) { | 144 | if (data.size() > std::numeric_limits<int>::max()) { |
| 137 | return false; | 145 | return false; |
| 138 | } | 146 | } |
| 139 | if (xmlParseChunk(p->context_, data.c_str(), static_cast<int>(data.size()), finalData) == XML_ERR_OK) { | 147 | auto error = xmlParseChunk(p->context_, data.c_str(), static_cast<int>(data.size()), finalData); |
| 148 | if (error == XML_ERR_OK) { | ||
| 140 | return true; | 149 | return true; |
| 141 | } | 150 | } |
| 142 | if (stopped_) return false; | 151 | if (stopped_) return false; |
| 143 | xmlError* error = xmlCtxtGetLastError(p->context_); | 152 | if (error == XML_WAR_NS_URI || error == XML_WAR_NS_URI_RELATIVE) { |
| 144 | if (error->code == XML_WAR_NS_URI || error->code == XML_WAR_NS_URI_RELATIVE) { | ||
| 145 | xmlCtxtResetLastError(p->context_); | 153 | xmlCtxtResetLastError(p->context_); |
| 146 | p->context_->errNo = XML_ERR_OK; | 154 | p->context_->errNo = XML_ERR_OK; |
| 147 | return true; | 155 | return true; |
diff --git a/Swiften/Parser/UnitTest/XMLParserTest.cpp b/Swiften/Parser/UnitTest/XMLParserTest.cpp index d38c1cc..89229c9 100644 --- a/Swiften/Parser/UnitTest/XMLParserTest.cpp +++ b/Swiften/Parser/UnitTest/XMLParserTest.cpp | |||
| @@ -45,6 +45,7 @@ class XMLParserTest : public CppUnit::TestFixture { | |||
| 45 | CPPUNIT_TEST(testParse_Doctype); | 45 | CPPUNIT_TEST(testParse_Doctype); |
| 46 | CPPUNIT_TEST(testParse_ProcessingInstructions); | 46 | CPPUNIT_TEST(testParse_ProcessingInstructions); |
| 47 | CPPUNIT_TEST(testParse_ProcessingPrefixedElement); | 47 | CPPUNIT_TEST(testParse_ProcessingPrefixedElement); |
| 48 | CPPUNIT_TEST(testParse_InvalidlyEncodedInput); | ||
| 48 | CPPUNIT_TEST_SUITE_END(); | 49 | CPPUNIT_TEST_SUITE_END(); |
| 49 | 50 | ||
| 50 | public: | 51 | public: |
| @@ -410,6 +411,20 @@ class XMLParserTest : public CppUnit::TestFixture { | |||
| 410 | CPPUNIT_ASSERT_EQUAL(std::string("uriPrefix"), client_.events[1].ns); | 411 | CPPUNIT_ASSERT_EQUAL(std::string("uriPrefix"), client_.events[1].ns); |
| 411 | } | 412 | } |
| 412 | 413 | ||
| 414 | void testParse_InvalidlyEncodedInput() { | ||
| 415 | ParserType testling(&client_); | ||
| 416 | |||
| 417 | // The following input was generated by a fuzzer, and triggered a crash in the LibXML2 parser because | ||
| 418 | // some types of error (buffer I/O errors, for instance) will not update the error in the parser context, | ||
| 419 | // and the code used to rely on that error always being set if parsing failed. | ||
| 420 | // This particular input will trick the parser into believing the encoding is UTF-16LE, which eventually will lead | ||
| 421 | // to two invalid encodings, followed by an I/O error. The latter will end parsing without updating the | ||
| 422 | // error in the parsing context, which used to trigger a crash. | ||
| 423 | testling.parse(std::string("<\0?\0\x80q type='get' id='aab9a'<<query xmlns='jabber:iq:roster'/>\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9b\x9br:i><quq:private'><storage xml s='s'\x00\x10</query></iq>", 271)); | ||
| 424 | testling.parse("<iq type='get'\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e\x9e to='ad5d8d2b25' ext='ca cs min@wonderland.t' id='aabda'><vCard xmlnr='vcard-temp'/>O/iq>"); | ||
| 425 | testling.parse("<\xff\xff\xff\x7fype:'get' to='won\x84" "erland.lit' id='aabea'><tuery xmlns='\xd8Vtp://jabber.org/p\x88ot\x8b" "col/disco#info'/>abber.org/protocol/disco#Nnfo'/></iq>"); | ||
| 426 | } | ||
| 427 | |||
| 413 | private: | 428 | private: |
| 414 | class Client : public XMLParserClient { | 429 | class Client : public XMLParserClient { |
| 415 | public: | 430 | public: |