From 181ac4a83ba4a82be683fb0a6f08393d3c91320c Mon Sep 17 00:00:00 2001
From: Joanna Hulboj <joanna.hulboj@isode.com>
Date: Thu, 12 Sep 2019 09:54:19 +0100
Subject: Close the stream for disallowed XML features

According to RFC 6120 if any disallowed XML feature is encountered,
we should close the stream with a <restricted-xml/>. The following
features of XML are prohibited in XMPP:
- processing instructions
- internal or external DTD subsets
- internal or external entity references
- comments

Test-information:
Unit tests pass on Windows 10 and Ubuntu 18.04.1 LTS

Change-Id: I475920c91b7f9da51ab37c106a4783a52f6e3cae

diff --git a/Sluift/ElementConvertors/DOMElementConvertor.cpp b/Sluift/ElementConvertors/DOMElementConvertor.cpp
index 56b45aa..5e72cc8 100644
--- a/Sluift/ElementConvertors/DOMElementConvertor.cpp
+++ b/Sluift/ElementConvertors/DOMElementConvertor.cpp
@@ -185,7 +185,7 @@ boost::optional<std::string> DOMElementConvertor::convertToLua(
 
     // Parse the payload again
     ParserClient parserClient(L);
-    std::shared_ptr<XMLParser> parser(parsers.createXMLParser(&parserClient));
+    std::shared_ptr<XMLParser> parser(parsers.createXMLParser(&parserClient, false));
     bool result = parser->parse(serializedPayload);
     assert(result);
 
diff --git a/Swift/Controllers/Settings/XMLSettingsProvider.cpp b/Swift/Controllers/Settings/XMLSettingsProvider.cpp
index 2573af0..8415209 100644
--- a/Swift/Controllers/Settings/XMLSettingsProvider.cpp
+++ b/Swift/Controllers/Settings/XMLSettingsProvider.cpp
@@ -18,7 +18,7 @@ namespace Swift {
 XMLSettingsProvider::XMLSettingsProvider(const std::string& xmlConfig) : level_(0) {
     if (!xmlConfig.empty()) {
         PlatformXMLParserFactory factory;
-        auto parser = factory.createXMLParser(this);
+        auto parser = factory.createXMLParser(this, true);
         if (parser->parse(xmlConfig)) {
             SWIFT_LOG(debug) << "Found and parsed system config" << std::endl;
         }
diff --git a/Swiften/Parser/ExpatParser.cpp b/Swiften/Parser/ExpatParser.cpp
index a50949b..640d561 100644
--- a/Swiften/Parser/ExpatParser.cpp
+++ b/Swiften/Parser/ExpatParser.cpp
@@ -72,8 +72,21 @@ static void handleEntityDeclaration(void* parser, const XML_Char*, int, const XM
     static_cast<ExpatParser*>(parser)->stopParser();
 }
 
+static void handleComment(void* parser, const XML_Char* /*data*/) {
+    if (!static_cast<ExpatParser*>(parser)->allowsComments()) {
+        static_cast<ExpatParser*>(parser)->stopParser();
+    }
+}
+
+static void handleProcessingInstruction(void* parser, const XML_Char* /*target*/, const XML_Char* /*data*/) {
+    static_cast<ExpatParser*>(parser)->stopParser();
+}
+
+static void handleDoctypeDeclaration(void* parser, const XML_Char* /*doctypeName*/, const XML_Char* /*sysid*/, const XML_Char* /*pubid*/, int /*has_internal_subset*/) {
+    static_cast<ExpatParser*>(parser)->stopParser();
+}
 
-ExpatParser::ExpatParser(XMLParserClient* client) : XMLParser(client), p(new Private()) {
+ExpatParser::ExpatParser(XMLParserClient* client, bool allowComments) : XMLParser(client, allowComments), p(new Private()) {
     p->parser_ = XML_ParserCreateNS("UTF-8", NAMESPACE_SEPARATOR);
     XML_SetUserData(p->parser_, this);
     XML_SetElementHandler(p->parser_, handleStartElement, handleEndElement);
@@ -81,6 +94,9 @@ ExpatParser::ExpatParser(XMLParserClient* client) : XMLParser(client), p(new Pri
     XML_SetXmlDeclHandler(p->parser_, handleXMLDeclaration);
     XML_SetEntityDeclHandler(p->parser_, handleEntityDeclaration);
     XML_SetNamespaceDeclHandler(p->parser_, handleNamespaceDeclaration, nullptr);
+    XML_SetCommentHandler(p->parser_, handleComment);
+    XML_SetProcessingInstructionHandler(p->parser_, handleProcessingInstruction);
+    XML_SetDoctypeDeclHandler(p->parser_, handleDoctypeDeclaration, nullptr);
 }
 
 ExpatParser::~ExpatParser() {
diff --git a/Swiften/Parser/ExpatParser.h b/Swiften/Parser/ExpatParser.h
index 7583339..34d790d 100644
--- a/Swiften/Parser/ExpatParser.h
+++ b/Swiften/Parser/ExpatParser.h
@@ -16,7 +16,7 @@
 namespace Swift {
     class SWIFTEN_API ExpatParser : public XMLParser, public boost::noncopyable {
         public:
-            ExpatParser(XMLParserClient* client);
+            ExpatParser(XMLParserClient* client, bool allowComments = false);
             ~ExpatParser();
 
             bool parse(const std::string& data, bool finalData = false);
diff --git a/Swiften/Parser/LibXMLParser.cpp b/Swiften/Parser/LibXMLParser.cpp
index 192f44b..4e02059 100644
--- a/Swiften/Parser/LibXMLParser.cpp
+++ b/Swiften/Parser/LibXMLParser.cpp
@@ -59,6 +59,24 @@ static void handleCharacterData(void* parser, const xmlChar* data, int len) {
     static_cast<XMLParser*>(parser)->getClient()->handleCharacterData(std::string(reinterpret_cast<const char*>(data), static_cast<size_t>(len)));
 }
 
+static void handleComment(void* parser, const xmlChar* /*data*/) {
+    if (!static_cast<LibXMLParser*>(parser)->allowsComments()) {
+        static_cast<LibXMLParser*>(parser)->stopParser();
+    }
+}
+
+static void handleEntityDeclaration(void * parser, const xmlChar* /*name*/, int /*type*/, const xmlChar* /*publicId*/, const xmlChar* /*systemId*/, xmlChar* /*content*/) {
+    static_cast<LibXMLParser*>(parser)->stopParser();
+}
+
+static void handleProcessingInstruction(void* parser, const xmlChar* /*target*/, const xmlChar* /*data*/) {
+    static_cast<LibXMLParser*>(parser)->stopParser();
+}
+
+static void handleExternalSubset(void* parser, const xmlChar * /*name*/, const xmlChar * /*ExternalID*/, const xmlChar * /*SystemID*/) {
+    static_cast<LibXMLParser*>(parser)->stopParser();
+}
+
 static void handleError(void*, const char* /*m*/, ... ) {
     /*
     va_list args;
@@ -73,7 +91,7 @@ static void handleWarning(void*, const char*, ... ) {
 
 bool LibXMLParser::initialized = false;
 
-LibXMLParser::LibXMLParser(XMLParserClient* client) : XMLParser(client), p(new Private()) {
+LibXMLParser::LibXMLParser(XMLParserClient* client, bool allowComments) : XMLParser(client, allowComments), p(new Private()) {
     // Initialize libXML for multithreaded applications
     if (!initialized) {
         xmlInitParser();
@@ -87,6 +105,10 @@ LibXMLParser::LibXMLParser(XMLParserClient* client) : XMLParser(client), p(new P
     p->handler_.characters = &handleCharacterData;
     p->handler_.warning = &handleWarning;
     p->handler_.error = &handleError;
+    p->handler_.comment = &handleComment;
+    p->handler_.entityDecl = &handleEntityDeclaration;
+    p->handler_.processingInstruction = &handleProcessingInstruction;
+    p->handler_.externalSubset = &handleExternalSubset;
 
     p->context_ = xmlCreatePushParserCtxt(&p->handler_, this, nullptr, 0, nullptr);
     xmlCtxtUseOptions(p->context_, XML_PARSE_NOENT);
@@ -106,6 +128,7 @@ bool LibXMLParser::parse(const std::string& data, bool finalData) {
     if (xmlParseChunk(p->context_, data.c_str(), static_cast<int>(data.size()), finalData) == XML_ERR_OK) {
         return true;
     }
+    if (stopped_) return false;
     xmlError* error = xmlCtxtGetLastError(p->context_);
     if (error->code == XML_WAR_NS_URI || error->code == XML_WAR_NS_URI_RELATIVE) {
         xmlCtxtResetLastError(p->context_);
@@ -115,4 +138,9 @@ bool LibXMLParser::parse(const std::string& data, bool finalData) {
     return false;
 }
 
+void LibXMLParser::stopParser() {
+    stopped_ = true;
+    xmlStopParser(p->context_);
+}
+
 }
diff --git a/Swiften/Parser/LibXMLParser.h b/Swiften/Parser/LibXMLParser.h
index a863867..e21770d 100644
--- a/Swiften/Parser/LibXMLParser.h
+++ b/Swiften/Parser/LibXMLParser.h
@@ -19,13 +19,16 @@ namespace Swift {
      */
     class LibXMLParser : public XMLParser, public boost::noncopyable {
         public:
-            LibXMLParser(XMLParserClient* client);
+            LibXMLParser(XMLParserClient* client, bool allowComments = false);
             virtual ~LibXMLParser();
 
             bool parse(const std::string& data, bool finalData = false);
 
+            void stopParser();
+
         private:
             static bool initialized;
+            bool stopped_ = false;
 
             struct Private;
             const std::unique_ptr<Private> p;
diff --git a/Swiften/Parser/PayloadParsers/UnitTest/PayloadsParserTester.h b/Swiften/Parser/PayloadParsers/UnitTest/PayloadsParserTester.h
index dcdbffa..8f9e0e1 100644
--- a/Swiften/Parser/PayloadParsers/UnitTest/PayloadsParserTester.h
+++ b/Swiften/Parser/PayloadParsers/UnitTest/PayloadsParserTester.h
@@ -19,7 +19,7 @@ namespace Swift {
     class PayloadsParserTester : public XMLParserClient {
         public:
             PayloadsParserTester() : level(0) {
-                xmlParser = PlatformXMLParserFactory().createXMLParser(this);
+                xmlParser = PlatformXMLParserFactory().createXMLParser(this, false);
             }
 
             bool parse(const std::string& data) {
diff --git a/Swiften/Parser/PlatformXMLParserFactory.cpp b/Swiften/Parser/PlatformXMLParserFactory.cpp
index bf66734..a424aca 100644
--- a/Swiften/Parser/PlatformXMLParserFactory.cpp
+++ b/Swiften/Parser/PlatformXMLParserFactory.cpp
@@ -20,11 +20,11 @@ namespace Swift {
 PlatformXMLParserFactory::PlatformXMLParserFactory() {
 }
 
-std::unique_ptr<XMLParser> PlatformXMLParserFactory::createXMLParser(XMLParserClient* client) {
+std::unique_ptr<XMLParser> PlatformXMLParserFactory::createXMLParser(XMLParserClient* client, bool allowComments) {
 #ifdef HAVE_LIBXML
-    return std::make_unique<LibXMLParser>(client);
+    return std::make_unique<LibXMLParser>(client, allowComments);
 #else
-    return std::make_unique<ExpatParser>(client);
+    return std::make_unique<ExpatParser>(client, allowComments);
 #endif
 }
 
diff --git a/Swiften/Parser/PlatformXMLParserFactory.h b/Swiften/Parser/PlatformXMLParserFactory.h
index fa3ca19..d72a513 100644
--- a/Swiften/Parser/PlatformXMLParserFactory.h
+++ b/Swiften/Parser/PlatformXMLParserFactory.h
@@ -14,6 +14,6 @@ namespace Swift {
         public:
             PlatformXMLParserFactory();
 
-            virtual std::unique_ptr<XMLParser> createXMLParser(XMLParserClient*);
+            virtual std::unique_ptr<XMLParser> createXMLParser(XMLParserClient*, bool allowComments = false);
     };
 }
diff --git a/Swiften/Parser/UnitTest/XMLParserTest.cpp b/Swiften/Parser/UnitTest/XMLParserTest.cpp
index c026b4b..63d30ea 100644
--- a/Swiften/Parser/UnitTest/XMLParserTest.cpp
+++ b/Swiften/Parser/UnitTest/XMLParserTest.cpp
@@ -39,6 +39,10 @@ class XMLParserTest : public CppUnit::TestFixture {
         CPPUNIT_TEST(testParse_InternalEntity);
         //CPPUNIT_TEST(testParse_UndefinedPrefix);
         //CPPUNIT_TEST(testParse_UndefinedAttributePrefix);
+        CPPUNIT_TEST(testParse_AllowCommentsInXML);
+        CPPUNIT_TEST(testParse_DisallowCommentsInXML);
+        CPPUNIT_TEST(testParse_Doctype);
+        CPPUNIT_TEST(testParse_ProcessingInstructions);
         CPPUNIT_TEST_SUITE_END();
 
     public:
@@ -185,7 +189,7 @@ class XMLParserTest : public CppUnit::TestFixture {
         }
 
         void testParse_UnhandledXML() {
-            ParserType testling(&client_);
+            ParserType testling(&client_, true);
 
             CPPUNIT_ASSERT(testling.parse("<iq><!-- Testing --></iq>"));
 
@@ -331,13 +335,44 @@ class XMLParserTest : public CppUnit::TestFixture {
         void testParse_UndefinedAttributePrefix() {
             ParserType testling(&client_);
 
-            CPPUNIT_ASSERT(testling.parse(
-                "<foo bar:baz='bla'/>"));
+            CPPUNIT_ASSERT(testling.parse("<foo bar:baz='bla'/>"));
 
             CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(1), client_.events[0].attributes.getEntries().size());
             CPPUNIT_ASSERT_EQUAL(std::string("bar:baz"), client_.events[0].attributes.getEntries()[0].getAttribute().getName());
         }
 
+        void testParse_AllowCommentsInXML() {
+            ParserType testling(&client_, true);
+
+            CPPUNIT_ASSERT(testling.parse("<message><!-- Some More Comments Testing --></message>"));
+
+            CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(2), client_.events.size());
+
+            CPPUNIT_ASSERT_EQUAL(Client::StartElement, client_.events[0].type);
+            CPPUNIT_ASSERT_EQUAL(std::string("message"), client_.events[0].data);
+
+            CPPUNIT_ASSERT_EQUAL(Client::EndElement, client_.events[1].type);
+            CPPUNIT_ASSERT_EQUAL(std::string("message"), client_.events[1].data);
+        }
+
+        void testParse_DisallowCommentsInXML() {
+            ParserType testling(&client_);
+
+            CPPUNIT_ASSERT(!testling.parse("<message><!-- Some More Comments Testing --></message>"));
+        }
+
+        void testParse_Doctype() {
+            ParserType testling(&client_);
+
+            CPPUNIT_ASSERT(!testling.parse("<!DOCTYPE greeting SYSTEM \"hello.dtd\">"));
+        }
+
+        void testParse_ProcessingInstructions() {
+            ParserType testling(&client_);
+
+            CPPUNIT_ASSERT(!testling.parse("<?xml-stylesheet type=\"text/xsl\" href=\"Sample.xsl\"?>"));
+        }
+
     private:
         class Client : public XMLParserClient {
             public:
@@ -378,6 +413,7 @@ class XMLParserTest : public CppUnit::TestFixture {
                 void handleNamespaceDeclaration(const std::string& prefix, const std::string& uri) override {
                     namespaces_[prefix] = uri;
                 }
+
                 std::vector<Event> events;
             private:
                 NamespaceMap namespaces_;
diff --git a/Swiften/Parser/XMLParser.cpp b/Swiften/Parser/XMLParser.cpp
index 8e92fe4..8a0799f 100644
--- a/Swiften/Parser/XMLParser.cpp
+++ b/Swiften/Parser/XMLParser.cpp
@@ -8,7 +8,7 @@
 
 namespace Swift {
 
-XMLParser::XMLParser(XMLParserClient* client) : client_(client) {
+XMLParser::XMLParser(XMLParserClient* client, bool allowComments) : client_(client), allowComments_(allowComments){
 }
 
 XMLParser::~XMLParser() {
diff --git a/Swiften/Parser/XMLParser.h b/Swiften/Parser/XMLParser.h
index ad79b2d..3b09d22 100644
--- a/Swiften/Parser/XMLParser.h
+++ b/Swiften/Parser/XMLParser.h
@@ -15,7 +15,7 @@ namespace Swift {
 
     class SWIFTEN_API XMLParser {
         public:
-            XMLParser(XMLParserClient* client);
+            XMLParser(XMLParserClient* client, bool allowComments = false);
             virtual ~XMLParser();
 
             virtual bool parse(const std::string& data, bool finalData = false) = 0;
@@ -24,7 +24,12 @@ namespace Swift {
                 return client_;
             }
 
+            bool allowsComments() const {
+                return allowComments_;
+            }
+
         private:
             XMLParserClient* client_;
+            const bool allowComments_ = false;
     };
 }
diff --git a/Swiften/Parser/XMLParserFactory.h b/Swiften/Parser/XMLParserFactory.h
index 595512b..ae3c90e 100644
--- a/Swiften/Parser/XMLParserFactory.h
+++ b/Swiften/Parser/XMLParserFactory.h
@@ -18,6 +18,6 @@ namespace Swift {
         public:
             virtual ~XMLParserFactory();
 
-            virtual std::unique_ptr<XMLParser> createXMLParser(XMLParserClient*) = 0;
+            virtual std::unique_ptr<XMLParser> createXMLParser(XMLParserClient*, bool allowComments = false) = 0;
     };
 }
-- 
cgit v0.10.2-6-g49f6