#3043 basic security checks with bandit
Merged 2 years ago by tkopecek. Opened 2 years ago by tkopecek.
tkopecek/koji issue3042  into  master

file modified
+3
@@ -129,6 +129,9 @@ 

  flake8:

  	tox -e flake8

  

+ bandit:

+ 	tox -e bandit

+ 

  tag::

  	git tag -a $(TAG)

  	@echo "Tagged with: $(TAG)"

file modified
+2 -2
@@ -3994,7 +3994,7 @@ 

          @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 @@ 

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

file modified
+1 -1
@@ -678,7 +678,7 @@ 

      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:

file modified
+2 -12
@@ -35,6 +35,7 @@ 

  import logging

  import os

  import re

+ import secrets

  import shutil

  import stat

  import sys
@@ -72,13 +73,6 @@ 

      safer_move,

  )

  

- try:

-     # py 3.6+

-     import secrets

- except ImportError:

-     import random

-     secrets = None

- 

  

  logger = logging.getLogger('koji.hub')

  
@@ -6272,11 +6266,7 @@ 

      """

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

file modified
+2 -2
@@ -1321,13 +1321,13 @@ 

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

file modified
+1 -1
@@ -53,7 +53,7 @@ 

          # 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 #

  

file modified
+6 -1
@@ -2,6 +2,7 @@ 

  

  from __future__ import absolute_import

  

+ import glob

  import os

  import platform

  import re
@@ -174,7 +175,11 @@ 

          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:

@@ -11,6 +11,7 @@ 

  __main__.BuildRoot = kojid.BuildRoot

  

  import koji

+ import koji.util

  import runroot

  

  if six.PY2:
@@ -346,9 +347,9 @@ 

      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,6 @@ 

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

          self.session.host.setBuildRootState.assert_called_once_with(678, 'BUILDING')

          self.br.mock.assert_has_calls([

              mock.call(['--install', 'rpm_a', 'rpm_b']),

file modified
+15 -1
@@ -1,5 +1,5 @@ 

  [tox]

- envlist = flake8,py2,py3

+ envlist = flake8,py2,py3,bandit

  

  [testenv:flake8]

  deps =
@@ -73,3 +73,17 @@ 

      {[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

file modified
+22 -17
@@ -25,15 +25,14 @@ 

  from __future__ import absolute_import

  

  import fnmatch

+ 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
@@ -411,13 +410,15 @@ 

              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)

+             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 +857,13 @@ 

                  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)

-                 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 +873,13 @@ 

              koji.ensuredir(options.workpath)

              dst = "%s/%s" % (options.workpath, fn)

              log("Downloading %s to %s..." % (url, dst))

-             fsrc = urllib2.urlopen(url)

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

file modified
+1 -1
@@ -486,7 +486,7 @@ 

              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

file modified
+1 -1
@@ -326,7 +326,7 @@ 

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

file modified
+1 -1
@@ -795,7 +795,7 @@ 

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

rebased onto bf6b4ce618b2bea0708668f1b57a3ea37976da5d

2 years ago

koji.util.rmtree doesn't support GLOB path

1 new commit added

  • fix unlinking
2 years ago

It might be helpful to add banditinto envlist of tox.ini and Makefile.
Otherwise :thumbsup:

rebased onto 8fdd52ca727038df79b3478da9e0b29a0ca82644

2 years ago

pretty please pagure-ci rebuild

2 years ago

This is different.rm -rf will not raise an error. os.unlink() will raise if the file is already gone (eg. race conditions). I recommend you wrap the unlink() call in try/catch FileNotFoundError.

How about a comment here explaining why we're skipping all of these?

Why are we silencing the warnings here?

Removing code! :rocket:

Actually catching and ignoring parent OSError would be better than FileNotFoundError. The original os.system('rm -rf ...') code would not raise in a variety of cases.

rebased onto 9c505b0072cf613468d8742c5993f3f70d8f8a10

2 years ago

Updated, I've removed most of the skipped tests and replaced them with # nosec.

rebased onto 390e98d

2 years ago

I've replaced urlopen with requests. Anyway, it depends if we don't want to drop koji-shadow completely (#2873)

Metadata Update from @tkopecek:
- Pull-request tagged with: no_qe

2 years ago

1 new commit added

  • remove unused imports
2 years ago

pretty please pagure-ci rebuild

2 years ago

pretty please pagure-ci rebuild

2 years ago

unittest failed now, as it watches os.path.join(rootdir, '/var/lib/rpm/__db*')

1 new commit added

  • fix test
2 years ago

Commit a90552a fixes this pull-request

Pull-Request has been merged by tkopecek

2 years ago