Mercurial > p > roundup > code
changeset 8237:57325fea9982
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.
| author | John Rouillard <rouilj@ieee.org> |
|---|---|
| date | Sun, 29 Dec 2024 19:11:01 -0500 |
| parents | 2d0bd038fc5e |
| children | 05405220dc38 |
| files | CHANGES.txt doc/installation.txt doc/upgrading.txt doc/xmlrpc.txt roundup/anypy/xmlrpc_.py roundup/cgi/client.py test/test_xmlrpc.py |
| diffstat | 7 files changed, 121 insertions(+), 2 deletions(-) [+] |
line wrap: on
line diff
--- a/CHANGES.txt Mon Dec 23 21:10:54 2024 -0500 +++ b/CHANGES.txt Sun Dec 29 19:11:01 2024 -0500 @@ -81,6 +81,9 @@ 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
--- a/doc/installation.txt Mon Dec 23 21:10:54 2024 -0500 +++ b/doc/installation.txt Sun Dec 29 19:11:01 2024 -0500 @@ -255,6 +255,11 @@ 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 @@ .. _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
--- a/doc/upgrading.txt Mon Dec 23 21:10:54 2024 -0500 +++ b/doc/upgrading.txt Sun Dec 29 19:11:01 2024 -0500 @@ -159,6 +159,36 @@ 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) ------------------------------------------
--- a/doc/xmlrpc.txt Mon Dec 23 21:10:54 2024 -0500 +++ b/doc/xmlrpc.txt Sun Dec 29 19:11:01 2024 -0500 @@ -79,7 +79,8 @@ 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::
--- a/roundup/anypy/xmlrpc_.py Mon Dec 23 21:10:54 2024 -0500 +++ b/roundup/anypy/xmlrpc_.py Sun Dec 29 19:11:01 2024 -0500 @@ -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.
--- a/roundup/cgi/client.py Mon Dec 23 21:10:54 2024 -0500 +++ b/roundup/cgi/client.py Sun Dec 29 19:11:01 2024 -0500 @@ -101,6 +101,9 @@ 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 @@ 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,
--- a/test/test_xmlrpc.py Mon Dec 23 21:10:54 2024 -0500 +++ b/test/test_xmlrpc.py Sun Dec 29 19:11:01 2024 -0500 @@ -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 @@ 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'
