From f41b8c70a7a56c69f8ebd9d20febc7756beebdfd Mon Sep 17 00:00:00 2001 From: Yu Ming Zhu Date: Mon, 15 Jan 2024 13:38:41 +0000 Subject: [PATCH] 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 | 5 ++--- koji/__init__.py | 6 +++--- koji/xmlrpcplus.py | 7 +++++++ kojihub/kojihub.py | 15 +++++++-------- requirements.txt | 1 + util/koji-gc | 5 ++--- util/koji-sidetag-cleanup | 3 +-- util/koji-sweep-db | 4 ++-- vm/kojikamid.py | 9 +++++++-- vm/kojivmd | 25 ++++++++++++------------- www/lib/kojiweb/util.py | 4 ++-- 11 files changed, 46 insertions(+), 38 deletions(-) diff --git a/cli/koji_cli/lib.py b/cli/koji_cli/lib.py index c605940f..a844d21e 100644 --- a/cli/koji_cli/lib.py +++ b/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) diff --git a/koji/__init__.py b/koji/__init__.py index 334b4034..a143a4c5 100644 --- a/koji/__init__.py +++ b/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) diff --git a/koji/xmlrpcplus.py b/koji/xmlrpcplus.py index 7da15935..0999e38c 100644 --- a/koji/xmlrpcplus.py +++ b/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 diff --git a/kojihub/kojihub.py b/kojihub/kojihub.py index 801fdfa6..eebb4e9c 100644 --- a/kojihub/kojihub.py +++ b/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) - except xmlrpc.client.Fault as fault: + result, method = koji.xmlrpcplus.loads(xml_result) + 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) - except xmlrpc.client.Fault as fault: + data, method = koji.xmlrpcplus.loads(val) + 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: diff --git a/requirements.txt b/requirements.txt index 3d8b263c..89175618 100644 --- a/requirements.txt +++ b/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 diff --git a/util/koji-gc b/util/koji-gc index 071203a6..134fede1 100755 --- a/util/koji-gc +++ b/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 diff --git a/util/koji-sidetag-cleanup b/util/koji-sidetag-cleanup index d2e13c4f..03509790 100755 --- a/util/koji-sidetag-cleanup +++ b/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)) diff --git a/util/koji-sweep-db b/util/koji-sweep-db index 69da15a0..5f858429 100755 --- a/util/koji-sweep-db +++ b/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) diff --git a/vm/kojikamid.py b/vm/kojikamid.py index fe271e22..70a929f6 100755 --- a/vm/kojikamid.py +++ b/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) diff --git a/vm/kojivmd b/vm/kojivmd index d6c0157e..7ddc8b24 100755 --- a/vm/kojivmd +++ b/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) - except xmlrpc.client.Fault as fault: - response = xmlrpc.client.dumps(fault) + response = dumps(response, methodresponse=1, allow_none=True) + except Fault as fault: + response = dumps(fault) except Exception: # report exception back to server - response = xmlrpc.client.dumps( - xmlrpc.client.Fault(1, "%s:%s" % (sys.exc_info()[0], sys.exc_info()[1])) + response = dumps( + 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 diff --git a/www/lib/kojiweb/util.py b/www/lib/kojiweb/util.py index 014add44..301951b4 100644 --- a/www/lib/kojiweb/util.py +++ b/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."""