Bandit [B411]: use defusedxml to prevent remote XML attacks

- putting xmlrpc stuff into koji.xmlrpcplus
- adding koji.xmlrpcplus.xmlrpc_server to refer
- replacing refs of original xmlrpc.client.dumps to enhanced
  koji.xmlrpcplus.dumps

fixes: #3964

cli/koji_cli/lib.py

@@ -23,7 +23,6 @@ import koji
 from koji import parse_arches
 from koji import _  # noqa: F401
 from koji.util import md5_constructor, to_list
-from koji.xmlrpcplus import xmlrpc_client
 
 
 # for compatibility with plugins based on older version of lib
@@ -208,7 +207,7 @@ class TaskWatcher(object):
         error = None
         try:
             self.session.getTaskResult(self.id)
-        except (six.moves.xmlrpc_client.Fault, koji.GenericError) as e:
+        except (koji.xmlrpcplus.Fault, koji.GenericError) as e:
             error = e
         if error is None:
             # print("%s: complete" % self.str())
@@ -922,6 +921,6 @@ def truncate_string(s, length=47):
 
 class DatetimeJSONEncoder(json.JSONEncoder):
     def default(self, o):
-        if isinstance(o, xmlrpc_client.DateTime):
+        if isinstance(o, koji.xmlrpcplus.DateTime):
             return str(o)
         return json.JSONEncoder.default(self, o)

koji/__init__.py

@@ -69,7 +69,7 @@ from requests.packages.urllib3.exceptions import MaxRetryError, HostChangedError
 from six.moves import range, zip
 
 from koji.tasks import parse_task_params
-from koji.xmlrpcplus import Fault, dumps, getparser, loads, xmlrpc_client
+from koji.xmlrpcplus import DateTime, Fault, dumps, getparser, loads
 from koji.util import deprecated
 from . import util
 from . import _version
@@ -3661,7 +3661,7 @@ def formatTime(value):
     """Format a timestamp so it looks nicer"""
     if not value and not isinstance(value, (int, float)):
         return ''
-    if isinstance(value, xmlrpc_client.DateTime):
+    if isinstance(value, DateTime):
         value = datetime.datetime.strptime(value.value, "%Y%m%dT%H:%M:%S")
     elif isinstance(value, (int, float)):
         value = datetime.datetime.fromtimestamp(value)
@@ -3684,7 +3684,7 @@ def formatTimeLong(value):
         return ''
     if isinstance(value, six.string_types):
         t = dateutil.parser.parse(value)
-    elif isinstance(value, xmlrpc_client.DateTime):
+    elif isinstance(value, DateTime):
         t = dateutil.parser.parse(value.value)
     elif isinstance(value, (int, float)):
         t = datetime.datetime.fromtimestamp(value)

koji/xmlrpcplus.py

@@ -6,9 +6,16 @@ from __future__ import absolute_import
 
 import types
 
+import defusedxml.xmlrpc as defusedxmlrpc
 import re
 import six
+# importing here for references in koji by defused.xmlrpc.monkey_patch() below
 import six.moves.xmlrpc_client as xmlrpc_client
+import six.moves.xmlrpc_server as xmlrpc_server  # noqa: F401
+
+
+# patching xmlrpc to protect against XML related attacks
+defusedxmlrpc.monkey_patch()
 
 # duplicate a few values that we need
 getparser = xmlrpc_client.getparser

kojihub/kojihub.py

@@ -47,7 +47,6 @@ import time
 import types
 import traceback
 from urllib.parse import parse_qs
-import xmlrpc.client
 import zipfile
 from collections import defaultdict, OrderedDict
 
@@ -513,7 +512,7 @@ class Task(object):
             # handle older base64 encoded data
             xml_request = base64.b64decode(xml_request)
         # note: loads accepts either bytes or string
-        params, method = xmlrpc.client.loads(xml_request)
+        params, method = koji.xmlrpcplus.loads(xml_request)
         return params
 
     def getResult(self, raise_fault=True):
@@ -534,8 +533,8 @@ class Task(object):
         try:
             # If the result is a Fault, then loads will raise it
             # This is normally what we want to happen
-            result, method = xmlrpc.client.loads(xml_result)
+            result, method = koji.xmlrpcplus.loads(xml_result)
-        except xmlrpc.client.Fault as fault:
+        except koji.xmlrpcplus.Fault as fault:
             if raise_fault:
                 raise
             # Note that you can't really return a fault over xmlrpc, except by
@@ -573,7 +572,7 @@ class Task(object):
                     # handle older base64 encoded data
                     task['request'] = base64.b64decode(task['request'])
                 # note: loads accepts either bytes or string
-                task['request'] = xmlrpc.client.loads(task['request'])[0]
+                task['request'] = koji.xmlrpcplus.loads(task['request'])[0]
         return results
 
     def runCallbacks(self, cbtype, old_info, attr, new_val):
@@ -13467,8 +13466,8 @@ class RootExports(object):
                             # handle older base64 encoded data
                             val = base64.b64decode(val)
                         # note: loads accepts either bytes or string
-                        data, method = xmlrpc.client.loads(val)
+                        data, method = koji.xmlrpcplus.loads(val)
-                    except xmlrpc.client.Fault as fault:
+                    except koji.xmlrpcplus.Fault as fault:
                         data = fault
                     task[f] = data
             yield task
@@ -13789,7 +13788,7 @@ class RootExports(object):
             xmlrpc DateTime value"""
         context.session.assertPerm('admin')
         buildinfo = get_build(build, strict=True)
-        if isinstance(ts, xmlrpc.client.DateTime):
+        if isinstance(ts, koji.xmlrpcplus.DateTime):
             # not recommended
             # the xmlrpclib.DateTime class is almost useless
             try:

requirements.txt

@@ -5,3 +5,4 @@ psycopg2-binary;python_version >= '3.0'
 python-multilib
 python-qpid-proton<0.38.0;python_version < '3.0'
 python-qpid-proton;python_version >= '3.0'
+defusedxml

util/koji-gc

@@ -15,7 +15,6 @@ import pprint
 import smtplib
 import sys
 import time
-import xmlrpc.client  # for Fault
 from email.mime import text as MIMEText
 from string import Template
 
@@ -1006,7 +1005,7 @@ def handle_prune():
                             session.untagBuildBypass(taginfo['id'], entry['build_id'],
                                                      force=bypass or is_admin)
                             untagged.setdefault(nvr, {})[tagname] = 1
-                        except (xmlrpc.client.Fault, koji.GenericError) as e:
+                        except (koji.xmlrpcplus.Fault, koji.GenericError) as e:
                             print("Warning: untag operation failed: %s" % e)
                             pass
                 # if action == 'keep' do nothing
@@ -1040,7 +1039,7 @@ def handle_prune():
                 print("Deleting untagged build: %s" % nvr)
                 try:
                     session.deleteBuild(build_id, strict=True)
-                except (xmlrpc.client.Fault, koji.GenericError) as e:
+                except (koji.xmlrpcplus.Fault, koji.GenericError) as e:
                     print("Warning: deletion failed: %s" % e)
                     # server issue
                     pass

util/koji-sidetag-cleanup

@@ -5,7 +5,6 @@ import configparser
 import datetime
 import optparse
 import os
-import xmlrpc
 
 import koji
 
@@ -124,7 +123,7 @@ def get_options():
 def ensure_connection(session):
     try:
         ret = session.getAPIVersion()
-    except xmlrpc.client.ProtocolError:
+    except koji.xmlrpcplus.xmlrpc_client.ProtocolError:
         error("Unable to connect to server")
     if ret != koji.API_VERSION:
         warn("The server is at API version %d and the client is at %d" % (ret, koji.API_VERSION))

util/koji-sweep-db

@@ -1,12 +1,12 @@
 #!/usr/bin/python3
 
 import os
-import xmlrpc.client
 from optparse import OptionParser
 from koji.context import context
 
 import koji
 import kojihub.db
+import koji.xmlrpcplus
 from kojihub.db import DeleteProcessor, QueryProcessor, BulkInsertProcessor
 
 
@@ -79,7 +79,7 @@ def clean_scratch_tasks(cursor, vacuum, test, age):
         task_id = row['id']
         request = row['request']
         try:
-            params, _ = xmlrpc.client.loads(request)
+            params, _ = koji.xmlrpcplus.xmlrpc_client.loads(request)
             opts = params[2]
             if opts['scratch']:
                 ids.append(task_id)

vm/kojikamid.py

@@ -38,7 +38,6 @@ import tempfile
 import threading
 import time
 import traceback
-import xmlrpc.client
 # urllib is required by the SCM class which is substituted into this file
 # do not remove the import below
 import urllib  # noqa: F401
@@ -46,6 +45,7 @@ import zipfile
 from configparser import RawConfigParser
 from optparse import OptionParser
 
+from defusedxml import xmlrpc
 import six    # noqa: F401, needed for imported code
 
 
@@ -53,6 +53,9 @@ MANAGER_PORT = 7000
 
 KOJIKAMID = True
 
+# patching xmlrpc to protect against XML related attacks
+xmlrpc.monkey_patch()
+
 # INSERT kojikamid dup #
 
 
@@ -635,7 +638,9 @@ def get_mgmt_server():
         macaddr, gateway = find_net_info()
     logger.debug('found MAC address %s, connecting to %s:%s',
                  macaddr, gateway, MANAGER_PORT)
-    server = xmlrpc.client.ServerProxy('http://%s:%s/' % (gateway, MANAGER_PORT), allow_none=True)
+    server = xmlrpc.xmlrpc_client.ServerProxy(
+        'http://%s:%s/' % (gateway, MANAGER_PORT), allow_none=True
+    )
     # we would set a timeout on the socket here, but that is apparently not
     # supported by python/cygwin/Windows
     task_port = server.getPort(macaddr)

vm/kojivmd

@@ -35,8 +35,6 @@ import subprocess
 import sys
 import threading
 import time
-import xmlrpc
-import xmlrpc.server
 from contextlib import closing
 from optparse import OptionParser
 
@@ -45,6 +43,7 @@ import libxml2
 import requests
 
 import koji
+from koji.xmlrpcplus import dumps, Fault, loads, xmlrpc_client, xmlrpc_server
 import koji.util
 from koji.daemon import SCM, TaskManager
 # TaskHandlers are required to be imported, do not remove them
@@ -295,15 +294,15 @@ def main(options, session):
 # Tasks for handling VM lifecycle
 ####################
 
-class DaemonXMLRPCServer(xmlrpc.server.SimpleXMLRPCServer):
+class DaemonXMLRPCServer(xmlrpc_server.SimpleXMLRPCServer):
     allow_reuse_address = True
 
     def __init__(self, addr, port):
         if sys.version_info[:2] <= (2, 4):
-            xmlrpc.server.SimpleXMLRPCServer.__init__(self, (addr, port),
+            xmlrpc_server.SimpleXMLRPCServer.__init__(self, (addr, port),
                                                       logRequests=False)
         else:
-            xmlrpc.server.SimpleXMLRPCServer.__init__(self, (addr, port),
+            xmlrpc_server.SimpleXMLRPCServer.__init__(self, (addr, port),
                                                       logRequests=False,
                                                       allow_none=True)
         self.logger = logging.getLogger('koji.vm.DaemonXMLRPCServer')
@@ -312,7 +311,7 @@ class DaemonXMLRPCServer(xmlrpc.server.SimpleXMLRPCServer):
 
     def server_close(self):
         self.active = False
-        xmlrpc.server.SimpleXMLRPCServer.server_close(self)
+        xmlrpc_server.SimpleXMLRPCServer.server_close(self)
 
     def handle_while_active(self):
         while self.active:
@@ -333,20 +332,20 @@ class DaemonXMLRPCServer(xmlrpc.server.SimpleXMLRPCServer):
         # Copy and paste from SimpleXMLRPCServer, with the addition of passing
         # allow_none=True to xmlrpclib.dumps()
         def _marshaled_dispatch(self, data, dispatch_method=None):
-            params, method = xmlrpc.client.loads(data)
+            params, method = loads(data)
             try:
                 if dispatch_method is not None:
                     response = dispatch_method(method, params)
                 else:
                     response = self._dispatch(method, params)
                 response = (response,)
-                response = xmlrpc.client.dumps(response, methodresponse=1, allow_none=True)
+                response = dumps(response, methodresponse=1, allow_none=True)
-            except xmlrpc.client.Fault as fault:
+            except Fault as fault:
-                response = xmlrpc.client.dumps(fault)
+                response = dumps(fault)
             except Exception:
                 # report exception back to server
-                response = xmlrpc.client.dumps(
+                response = dumps(
-                    xmlrpc.client.Fault(1, "%s:%s" % (sys.exc_info()[0], sys.exc_info()[1]))
+                    Fault(1, "%s:%s" % (sys.exc_info()[0], sys.exc_info()[1]))
                 )
             return response
 
@@ -480,7 +479,7 @@ class VMExecTask(BaseTaskHandler):
 
     def __init__(self, *args, **kw):
         super(VMExecTask, self).__init__(*args, **kw)
-        self.task_manager = xmlrpc.client.ServerProxy(
+        self.task_manager = xmlrpc_client.ServerProxy(
             'http://%s:%s/' % (self.options.privaddr, self.options.portbase), allow_none=True)
         self.port = None
         self.server = None

www/lib/kojiweb/util.py

@@ -26,7 +26,6 @@ import re
 import ssl
 import stat
 import urllib
-import xmlrpc.client
 # a bunch of exception classes that explainError needs
 from socket import error as socket_error
 from xml.parsers.expat import ExpatError
@@ -35,6 +34,7 @@ import Cheetah.Template
 
 import koji
 import koji.tasks
+from koji.xmlrpcplus import xmlrpc_client
 
 
 themeInfo = {}
@@ -726,7 +726,7 @@ a bug or a configuration issue."""
 The web interface is having difficulty communicating with the main \
 server. This most likely indicates a network issue."""
         level = 1
-    elif isinstance(error, (xmlrpc.client.ProtocolError, ExpatError)):
+    elif isinstance(error, (xmlrpc_client.ProtocolError, ExpatError)):
         str = """\
 The main server returned an invalid response. This could be caused by \
 a network issue or load issues on the server."""