From ba2e1e520b32ee3dc66abe56d3cd0fdcbb0c8380 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Wed, 22 Sep 2021 11:46:45 +0200 Subject: [PATCH] basic security checks with bandit Fixes: https://pagure.io/koji/issue/3042 --- Makefile | 3 +++ builder/kojid | 4 ++-- cli/koji_cli/lib.py | 2 +- hub/kojihub.py | 14 ++------------ koji/__init__.py | 4 ++-- koji/util.py | 2 +- plugins/builder/runroot.py | 7 ++++++- tests/test_plugins/test_runroot_builder.py | 7 ++++--- tox.ini | 16 +++++++++++++++- util/koji-shadow | 6 +++--- util/kojira | 2 +- vm/kojikamid.py | 2 +- vm/kojivmd | 2 +- 13 files changed, 42 insertions(+), 29 deletions(-) diff --git a/Makefile b/Makefile index 992446aa..1c213857 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 3ef503f9..b222b1a5 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 4ba7dd6c..df802044 100644 --- a/cli/koji_cli/lib.py +++ b/cli/koji_cli/lib.py @@ -680,7 +680,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 12a6295c..dd8dbe2f 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 83ab9bfd..139825f2 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 304e1125..01981f53 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 0b098df2..67c9370d 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 d87b37e2..9136b9da 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 934d3342..25039a89 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 6d43a93b..a65645d5 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 875be9cf..e1b142f5 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 dba83a00..f5a7460a 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 c1a3c991..5e031fdd 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':