issue2551116 - Replace xmlrpclib (xmlrpc.client) with defusedxml.

defusedxml will be used to moneypatch the problematic client and
server modules.

Test added using an xml bomb.

CHANGES.txt

@@ -81,6 +81,9 @@ Features:
   Rouillard) 
 - added fuzz testing for some code. Found issue2551382 and
   others. (John Rouillard)
+- issue2551116 - Replace xmlrpclib (xmlrpc.client) with defusedxml.
+  Added support for defusedxml to better secure the xmlrpc
+  endpoint. (John Rouillard)
 
 2024-07-13 2.4.0
 

doc/installation.txt

@@ -255,6 +255,11 @@ jinja2
   its TEMPLATE-INFO.txt file) you need
   to have the jinja2_ template engine installed.
 
+defusedxml
+  If you are going to enable and use the XMLRPC endpoint, you should
+  install the defusedxml_ module. It will still work with the default
+  xmlrpc standard library, but it will log a warning when used.
+
 .. _install/docutils:
 
 docutils
@@ -2371,6 +2376,7 @@ the test.
 .. _apache: https://httpd.apache.org/
 .. _brotli: https://pypi.org/project/Brotli/
 .. _`developer's guide`: developers.html
+.. _defusedxml: https://pypi.org/project/defusedxml/
 .. _docutils: https://pypi.org/project/docutils/
 .. _flup: https://pypi.org/project/flup/
 .. _gpg: https://www.gnupg.org/software/gpgme/index.html

doc/upgrading.txt

@@ -159,6 +159,36 @@ following diff::
 add the lines marked with ``+`` in the file in the location after
 check_main is assigned.
 
+Defusedxml support improves XMLRPC security (optional)
+------------------------------------------------------
+
+This release adds support for the defusedxml_ module. If it is
+installed it will be automatically used. The default xmlrpc module in
+the standard library has known issues when parsing crafted XML. It can
+take a lot of CPU time and consume large amounts of memory with small
+payloads.
+
+When the XMLRPC endpoint is used without defusedxml, it will log a
+warning to the log file. The log entry can be disabled by adding::
+
+
+    from roundup.cgi import client
+    client.WARN_FOR_MISSING_DEFUSEDXML = False
+
+to the ``interfaces.py`` file in the tracker home. (Create the file if
+it is missing.)
+
+XMLRPC access is enabled by default in the classic and other trackers.
+Upgrading to defusedxml is considered optional because the XMLRPC
+endpoint can be disabled in the tracker's ``config.ini``. Also
+``Xmlrpc Access`` can be removed from the ``Users`` role by commenting
+out a line in ``schema.py``.
+
+If you have enabled the xmlrpc endpoint, you should install
+defusedxml.
+
+.. _defusedxml: https://pypi.org/project/defusedxml/
+
 More secure session cookie handling (info)
 ------------------------------------------
 

doc/xmlrpc.txt

@@ -79,7 +79,8 @@ Both the standalone and embedded roundup XML endpoints used the
 default python XML parser. This parser is know to have security
 issues. For details see: https://pypi.org/project/defusedxml/.
 You may wish to use the rest interface which doesn't have the same
-issues. Patches with tests to roundup to use defusedxml are welcome.
+issues. If you install defusedxml, it will be automatically used to add
+some additional protection.
 
 .. caution::
 

roundup/anypy/xmlrpc_.py

@@ -1,6 +1,18 @@
 try:
     # Python 3+.
     from xmlrpc import client, server
+    # If client.defusedxml == False, client.py will warn that
+    # xmlrpc is insecure and defusedxml should be installed.
+    client.defusedxml=False
+    try:
+        from defusedxml import xmlrpc
+        xmlrpc.monkey_patch()
+        # figure out how to allow user to set xmlrpc.MAX_DATA = bytes
+        client.defusedxml=True
+    except ImportError:
+        # use regular xmlrpc with warnings
+        pass
+
     server.SimpleXMLRPCDispatcher
 except (ImportError, AttributeError):
     # Python 2.

roundup/cgi/client.py

@@ -101,6 +101,9 @@ def add_message(msg_list, msg, escape=True):
     msg_list.append(msg)
     return msg_list  # for unittests
 
+# if set to False via interfaces.py do not log a warning when
+# xmlrpc is used and defusedxml is not installed.
+WARN_FOR_MISSING_DEFUSEDXML = True
 
 default_err_msg = ''"""<html><head><title>An error has occurred</title></head>
 <body><h1>An error has occurred</h1>
@@ -656,6 +659,8 @@ def handle_xmlrpc(self):
             csrf_ok = False  # we had an error, failed check
 
         if csrf_ok is True:
+            if WARN_FOR_MISSING_DEFUSEDXML and (not xmlrpc_.client.defusedxml):
+                logger.warning(self._("XMLRPC endpoint is not using defusedxml. Improve security by installing defusedxml."))
             handler = xmlrpc.RoundupDispatcher(self.db,
                                                self.instance.actions,
                                                self.translator,

test/test_xmlrpc.py

@@ -5,7 +5,7 @@
 #
 
 from __future__ import print_function
-import unittest, os, shutil, errno, sys, difflib, re
+import unittest, os, shutil, errno, pytest, sys, difflib, re
 
 from roundup.anypy import xmlrpc_
 MultiCall = xmlrpc_.client.MultiCall
@@ -21,6 +21,19 @@
 from .test_mysql import skip_mysql
 from .test_postgresql import skip_postgresql
 
+from .pytest_patcher import mark_class
+from roundup.anypy.xmlrpc_ import client
+
+if client.defusedxml:
+    skip_defusedxml = lambda func, *args, **kwargs: func
+
+    skip_defusedxml_used = mark_class(pytest.mark.skip(
+        reason='Skipping non-defusedxml tests: defusedxml library in use'))
+else:
+    skip_defusedxml = mark_class(pytest.mark.skip(
+        reason='Skipping defusedxml tests: defusedxml library not available'))
+
+    skip_defusedxml_used = lambda func, *args, **kwargs: func
 
 class XmlrpcTest(object):
 
@@ -314,6 +327,55 @@ class S:
         for n, r in enumerate(result):
             self.assertEqual(r, results[n])
 
+    @skip_defusedxml
+    def testDefusedXmlBomb(self):
+        self.XmlBomb(expectIn=b"defusedxml.common.EntitiesForbidden")
+
+    @skip_defusedxml_used
+    def testNonDefusedXmlBomb(self):
+        self.XmlBomb(expectIn=b"1234567890"*511)
+
+    def XmlBomb(self, expectIn=None):
+
+        bombInput = """<?xml version='1.0'?>
+        <!DOCTYPE xmlbomb [
+        <!ENTITY a "1234567890" >
+        <!ENTITY b "&a;&a;&a;&a;&a;&a;&a;&a;">
+        <!ENTITY c "&b;&b;&b;&b;&b;&b;&b;&b;">
+        <!ENTITY d "&c;&c;&c;&c;&c;&c;&c;&c;">
+        ]>
+        <methodCall>
+        <methodName>filter</methodName>
+        <params>
+        <param>
+        <value><string>&d;</string></value>
+        </param>
+        <param>
+        <value><array><data>
+        <value><string>0</string></value>
+        <value><string>2</string></value>
+        <value><string>3</string></value>
+        </data></array></value>
+        </param>
+        <param>
+        <value><struct>
+        <member>
+        <name>username</name>
+        <value><string>demo</string></value>
+        </member>
+        </struct></value>
+        </param>
+        </params>
+        </methodCall>
+        """
+        translator = TranslationService.get_translation(
+            language=self.instance.config["TRACKER_LANGUAGE"],
+            tracker_home=self.instance.config["TRACKER_HOME"])
+        self.server = RoundupDispatcher(self.db, self.instance.actions,
+            translator, allow_none = True)
+        response = self.server.dispatch(bombInput)
+        print(response)
+        self.assertIn(expectIn, response)
 
 class anydbmXmlrpcTest(XmlrpcTest, unittest.TestCase):
     backend = 'anydbm'