basic security checks with bandit

Fixes: https://pagure.io/koji/issue/3042

Makefile

@@ -129,6 +129,9 @@ pypi-upload:
 flake8:
 	tox -e flake8
 
+bandit:
+	tox -e bandit
+
 tag::
 	git tag -a $(TAG)
 	@echo "Tagged with: $(TAG)"

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(',')

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:

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)
-        return secrets.token_hex(nbytes=nbytes)
-    else:
-        values = ['%02x' % random.randint(0, 255) for x in range(nbytes)]
-        return ''.join(values)
 
 
 def get_reservation_token(build_id):

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()):

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 #
 

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:

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, platform_uname, os_unlink):
-    def test_handler_simple(self, os_system, platform_uname):
         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']),

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

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()

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

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':

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':