diff --git a/CHANGES.txt b/CHANGES.txt index 746ac31c..738efcb4 100644 --- a/CHANGES.txt +++ b/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 diff --git a/doc/installation.txt b/doc/installation.txt index bc6f23bf..7e05ec5e 100644 --- a/doc/installation.txt +++ b/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 diff --git a/doc/upgrading.txt b/doc/upgrading.txt index 063ce02c..e1d163d3 100644 --- a/doc/upgrading.txt +++ b/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) ------------------------------------------ diff --git a/doc/xmlrpc.txt b/doc/xmlrpc.txt index bf2d939b..fc25a983 100644 --- a/doc/xmlrpc.txt +++ b/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:: diff --git a/roundup/anypy/xmlrpc_.py b/roundup/anypy/xmlrpc_.py index 37ce7ea0..9d39e2e0 100644 --- a/roundup/anypy/xmlrpc_.py +++ b/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. diff --git a/roundup/cgi/client.py b/roundup/cgi/client.py index 234bb192..33679280 100644 --- a/roundup/cgi/client.py +++ b/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 = ''"""An error has occurred

An error has occurred

@@ -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, diff --git a/test/test_xmlrpc.py b/test/test_xmlrpc.py index a68c1870..ae134689 100644 --- a/test/test_xmlrpc.py +++ b/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 = """ + + + + + ]> + + filter + + + &d; + + + + 0 + 2 + 3 + + + + + + username + demo + + + + + + """ + 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'