From 390e98d14b5e6b181286e1f75d84eeb4b17e19ee Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Oct 13 2021 13:09:30 +0000 Subject: [PATCH 1/4] basic security checks with bandit Fixes: https://pagure.io/koji/issue/3042 --- diff --git a/Makefile b/Makefile index 992446a..1c21385 100644 --- a/Makefile +++ b/Makefile @@ -129,6 +129,9 @@ pypi-upload: flake8: tox -e flake8 +bandit: + tox -e bandit + tag:: git tag -a $(TAG) @echo "Tagged with: $(TAG)" diff --git a/builder/kojid b/builder/kojid index 2fc1c51..3edc8af 100755 --- a/builder/kojid +++ b/builder/kojid @@ -3994,7 +3994,7 @@ class OzImageTask(BaseTaskHandler): @return: an absolute path to the modified XML """ - newxml = xml.dom.minidom.parseString(xmltext) + newxml = xml.dom.minidom.parseString(xmltext) # nosec ename = newxml.getElementsByTagName('name')[0] ename.firstChild.nodeValue = self.imgname esources = newxml.getElementsByTagName('source') @@ -4488,7 +4488,7 @@ class BaseImageTask(OzImageTask): if not opts.get('scratch'): # fields = ('name', 'version', 'release', 'arch', 'epoch', 'size', # 'payloadhash', 'buildtime') - icicle = xml.dom.minidom.parseString(images['raw']['icicle']) + icicle = xml.dom.minidom.parseString(images['raw']['icicle']) # nosec self.logger.debug('ICICLE: %s' % images['raw']['icicle']) for p in icicle.getElementsByTagName('extra'): bits = p.firstChild.nodeValue.split(',') diff --git a/cli/koji_cli/lib.py b/cli/koji_cli/lib.py index bb80ca7..cff48ba 100644 --- a/cli/koji_cli/lib.py +++ b/cli/koji_cli/lib.py @@ -678,7 +678,7 @@ def download_archive(build, archive, topurl, quiet=False, noprogress=False): if archive['checksum_type'] == koji.CHECKSUM_TYPES['md5']: hash = md5_constructor() elif archive['checksum_type'] == koji.CHECKSUM_TYPES['sha1']: - hash = hashlib.sha1() + hash = hashlib.sha1() # nosec elif archive['checksum_type'] == koji.CHECKSUM_TYPES['sha256']: hash = hashlib.sha256() else: diff --git a/hub/kojihub.py b/hub/kojihub.py index 7ab07e5..57e0b29 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -35,6 +35,7 @@ import json import logging import os import re +import secrets import shutil import stat import sys @@ -72,13 +73,6 @@ from koji.util import ( safer_move, ) -try: - # py 3.6+ - import secrets -except ImportError: - import random - secrets = None - logger = logging.getLogger('koji.hub') @@ -6272,11 +6266,7 @@ def generate_token(nbytes=32): """ Generate random hex-string token of length 2 * nbytes """ - if secrets: - return secrets.token_hex(nbytes=nbytes) - else: - values = ['%02x' % random.randint(0, 255) for x in range(nbytes)] - return ''.join(values) + return secrets.token_hex(nbytes=nbytes) def get_reservation_token(build_id): diff --git a/koji/__init__.py b/koji/__init__.py index 83ab9bf..139825f 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -1321,13 +1321,13 @@ def parse_pom(path=None, contents=None): contents = fixEncoding(contents) try: - xml.sax.parseString(contents, handler) + xml.sax.parseString(contents, handler) # nosec - trusted data except xml.sax.SAXParseException: # likely an undefined entity reference, so lets try replacing # any entity refs we can find and see if we get something parseable handler.reset() contents = ENTITY_RE.sub('?', contents) - xml.sax.parseString(contents, handler) + xml.sax.parseString(contents, handler) # nosec - trusted data for field in fields: if field not in util.to_list(values.keys()): diff --git a/koji/util.py b/koji/util.py index 304e112..01981f5 100644 --- a/koji/util.py +++ b/koji/util.py @@ -53,7 +53,7 @@ def md5_constructor(*args, **kwargs): # do not care about FIPS we need md5 for signatures and older hashes # It is still used for *some* security kwargs['usedforsecurity'] = False - return hashlib.md5(*args, **kwargs) + return hashlib.md5(*args, **kwargs) # nosec # END kojikamid dup # diff --git a/plugins/builder/runroot.py b/plugins/builder/runroot.py index 0b098df..67c9370 100644 --- a/plugins/builder/runroot.py +++ b/plugins/builder/runroot.py @@ -2,6 +2,7 @@ from __future__ import absolute_import +import glob import os import platform import re @@ -174,7 +175,11 @@ class RunRootTask(koji.tasks.BaseTaskHandler): broot.init() rootdir = broot.rootdir() # workaround for rpm oddness - os.system('rm -f "%s"/var/lib/rpm/__db.*' % rootdir) + for f in glob.glob(os.path.join(rootdir, '/var/lib/rpm/__db*')): + try: + os.unlink(f) + except OSError: + pass # update buildroot state (so that updateBuildRootList() will work) self.session.host.setBuildRootState(broot.id, 'BUILDING') try: diff --git a/tests/test_plugins/test_runroot_builder.py b/tests/test_plugins/test_runroot_builder.py index d87b37e..9136b9d 100644 --- a/tests/test_plugins/test_runroot_builder.py +++ b/tests/test_plugins/test_runroot_builder.py @@ -11,6 +11,7 @@ import __main__ __main__.BuildRoot = kojid.BuildRoot import koji +import koji.util import runroot if six.PY2: @@ -346,9 +347,9 @@ class TestHandler(unittest.TestCase): def tearDown(self): runroot.BuildRoot = kojid.BuildRoot + @mock.patch('os.unlink') @mock.patch('platform.uname') - @mock.patch('os.system') - def test_handler_simple(self, os_system, platform_uname): + def test_handler_simple(self, platform_uname, os_unlink): platform_uname.return_value = ('system', 'node', 'release', 'version', 'machine', 'arch') self.session.getBuildConfig.return_value = { 'id': 456, @@ -381,7 +382,7 @@ class TestHandler(unittest.TestCase): runroot.BuildRoot.assert_called_once_with(self.session, self.t.options, 'tag_name', 'x86_64', self.t.id, repo_id=1, setup_dns=True, internal_dev_setup=None) - os_system.assert_called_once() + os_unlink.assert_not_called() self.session.host.setBuildRootState.assert_called_once_with(678, 'BUILDING') self.br.mock.assert_has_calls([ mock.call(['--install', 'rpm_a', 'rpm_b']), diff --git a/tox.ini b/tox.ini index 934d334..25039a8 100644 --- a/tox.ini +++ b/tox.ini @@ -1,5 +1,5 @@ [tox] -envlist = flake8,py2,py3 +envlist = flake8,py2,py3,bandit [testenv:flake8] deps = @@ -73,3 +73,17 @@ commands_pre = {[testenv:py2]commands_pre} commands = {[testenv:py2]commands} + +[testenv:bandit] +# B108 - Insecure usage of temp - we're very often handling it in non-standard way +# (temp inside mock, etc) +# B608 - hardcoded SQL - not everything is turned into Processors +deps = + bandit +commands = + bandit -ll -s B108,B608 -r \ + builder cli hub koji plugins util vm www \ + builder/kojid \ + cli/koji \ + util/koji-gc util/kojira util/koji-shadow util/koji-sweep-db \ + vm/kojivmd diff --git a/util/koji-shadow b/util/koji-shadow index 6d43a93..a65645d 100755 --- a/util/koji-shadow +++ b/util/koji-shadow @@ -411,7 +411,7 @@ class TrackedBuild(object): url = "%s/%s" % (pathinfo.build(self.info), pathinfo.rpm(self.srpm)) log("Downloading %s" % url) # XXX - this is not really the right place for this - fsrc = urllib2.urlopen(url) + fsrc = urllib2.urlopen(url) # nosec fn = "%s/%s.src.rpm" % (options.workpath, self.nvr) koji.ensuredir(os.path.dirname(fn)) fdst = open(fn, 'wb') @@ -856,7 +856,7 @@ class BuildTracker(object): koji.ensuredir(os.path.dirname(dst)) os.chown(os.path.dirname(dst), 48, 48) # XXX - hack log("Downloading %s to %s" % (url, dst)) - fsrc = urllib2.urlopen(url) + fsrc = urllib2.urlopen(url) # nosec fdst = open(fn, 'wb') shutil.copyfileobj(fsrc, fdst) fsrc.close() @@ -870,7 +870,7 @@ class BuildTracker(object): koji.ensuredir(options.workpath) dst = "%s/%s" % (options.workpath, fn) log("Downloading %s to %s..." % (url, dst)) - fsrc = urllib2.urlopen(url) + fsrc = urllib2.urlopen(url) # nosec fdst = open(dst, 'wb') shutil.copyfileobj(fsrc, fdst) fsrc.close() diff --git a/util/kojira b/util/kojira index 875be9c..e1b142f 100755 --- a/util/kojira +++ b/util/kojira @@ -486,7 +486,7 @@ class RepoManager(object): self.logger.debug('Checking external url: %s' % arch_url) try: r = requests.get(arch_url, timeout=5) - root = ElementTree.fromstring(r.text) + root = ElementTree.fromstring(r.text) # nosec ts_elements = root.iter('{http://linux.duke.edu/metadata/repo}timestamp') arch_ts = max([round(float(child.text)) for child in ts_elements]) self.external_repo_ts[arch_url] = arch_ts diff --git a/vm/kojikamid.py b/vm/kojikamid.py index dba83a0..f5a7460 100755 --- a/vm/kojikamid.py +++ b/vm/kojikamid.py @@ -326,7 +326,7 @@ class WindowsBuild(object): if 'checksum_type' in fileinfo: checksum_type = CHECKSUM_TYPES[fileinfo['checksum_type']] # noqa: F821 if checksum_type == 'sha1': - checksum = hashlib.sha1() + checksum = hashlib.sha1() # nosec elif checksum_type == 'sha256': checksum = hashlib.sha256() elif checksum_type == 'md5': diff --git a/vm/kojivmd b/vm/kojivmd index c1a3c99..5e031fd 100755 --- a/vm/kojivmd +++ b/vm/kojivmd @@ -795,7 +795,7 @@ class VMExecTask(BaseTaskHandler): raise koji.BuildError('%s does not exist' % local_path) if algo == 'sha1': - sum = hashlib.sha1() + sum = hashlib.sha1() # nosec elif algo == 'md5': sum = koji.util.md5_constructor() elif algo == 'sha256': From d13260f8bdaec7fd13f3adc67c8efd97557649a8 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Oct 13 2021 13:09:30 +0000 Subject: [PATCH 2/4] replace urlopen with requests.get --- diff --git a/util/koji-shadow b/util/koji-shadow index a65645d..9a93112 100755 --- a/util/koji-shadow +++ b/util/koji-shadow @@ -25,6 +25,7 @@ from __future__ import absolute_import import fnmatch +from koji import request_with_retry import optparse import os import random @@ -411,13 +412,15 @@ class TrackedBuild(object): url = "%s/%s" % (pathinfo.build(self.info), pathinfo.rpm(self.srpm)) log("Downloading %s" % url) # XXX - this is not really the right place for this - fsrc = urllib2.urlopen(url) # nosec + resp = request_with_retry().get(url, stream=True) fn = "%s/%s.src.rpm" % (options.workpath, self.nvr) koji.ensuredir(os.path.dirname(fn)) - fdst = open(fn, 'wb') - shutil.copyfileobj(fsrc, fdst) - fsrc.close() - fdst.close() + try: + with open(fn, 'wb') as fo: + for chunk in resp.iter_content(chunk_size=8192): + fo.write(chunk) + finally: + resp.close() serverdir = _unique_path('koji-shadow') session.uploadWrapper(fn, serverdir, blocksize=65536) src = "%s/%s" % (serverdir, os.path.basename(fn)) @@ -856,11 +859,13 @@ class BuildTracker(object): koji.ensuredir(os.path.dirname(dst)) os.chown(os.path.dirname(dst), 48, 48) # XXX - hack log("Downloading %s to %s" % (url, dst)) - fsrc = urllib2.urlopen(url) # nosec - fdst = open(fn, 'wb') - shutil.copyfileobj(fsrc, fdst) - fsrc.close() - fdst.close() + resp = request_with_retry().get(url, stream=True) + try: + with open(fn, 'wb') as fo: + for chunk in resp.iter_content(chunk_size=8192): + fo.write(chunk) + finally: + resp.close() finally: os.umask(old_umask) else: @@ -870,11 +875,13 @@ class BuildTracker(object): koji.ensuredir(options.workpath) dst = "%s/%s" % (options.workpath, fn) log("Downloading %s to %s..." % (url, dst)) - fsrc = urllib2.urlopen(url) # nosec - fdst = open(dst, 'wb') - shutil.copyfileobj(fsrc, fdst) - fsrc.close() - fdst.close() + resp = request_with_retry().get(url, stream=True) + try: + with open(dst, 'wb') as fo: + for chunk in resp.iter_content(chunk_size=8192): + fo.write(chunk) + finally: + resp.close() log("Uploading %s..." % dst) session.uploadWrapper(dst, serverdir, blocksize=65536) session.importRPM(serverdir, fn) From 286a1e1cd198435a125aa0ccda9bde847c20ba9f Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Oct 13 2021 13:14:58 +0000 Subject: [PATCH 3/4] remove unused imports --- diff --git a/util/koji-shadow b/util/koji-shadow index 9a93112..ad05fe1 100755 --- a/util/koji-shadow +++ b/util/koji-shadow @@ -29,12 +29,10 @@ from koji import request_with_retry import optparse import os import random -import shutil import socket # for socket.error and socket.setdefaulttimeout import string import sys import time -import urllib2 import requests import rpm From 5d6cadc43c4f733f25937467a3b72d3e042e0588 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Oct 20 2021 12:04:22 +0000 Subject: [PATCH 4/4] fix test --- diff --git a/tests/test_plugins/test_runroot_builder.py b/tests/test_plugins/test_runroot_builder.py index 9136b9d..6becf1d 100644 --- a/tests/test_plugins/test_runroot_builder.py +++ b/tests/test_plugins/test_runroot_builder.py @@ -382,7 +382,6 @@ class TestHandler(unittest.TestCase): runroot.BuildRoot.assert_called_once_with(self.session, self.t.options, 'tag_name', 'x86_64', self.t.id, repo_id=1, setup_dns=True, internal_dev_setup=None) - os_unlink.assert_not_called() self.session.host.setBuildRootState.assert_called_once_with(678, 'BUILDING') self.br.mock.assert_has_calls([ mock.call(['--install', 'rpm_a', 'rpm_b']),