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'

Roundup Issue Tracker: http://roundup-tracker.org/