From e576c5cee0240fbfa87566be906a51d9ac1613b2 Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Fri, 6 Dec 2024 12:21:01 -0800 Subject: [PATCH] CI: Configure bandit better Declare the tests to skip, rather than the tests to run. This ensures that we pick up new bandit checks automatically. I recently noticed a use of md5() without the usedforsecurity=False kwarg. Confused about why this wasn't caught in the gate, I eventually traced it back to B303 (which we explicitly enabled) being largely superseded by B324 (which did not exist when we wrote down the tests to enable). Flag a bunch of false-positives with "# nosec" comments, resolve two other errors, and skip some more-pervasive errors, to be resolved later. Change-Id: Ia054e4f7c9e5bf29064a66933e27830adbc107d3 --- bandit.yaml | 20 +++++++++++++++++++- swift/cli/account_audit.py | 4 ++-- swift/common/middleware/bulk.py | 8 ++++---- swift/common/middleware/xprofile.py | 2 +- swift/common/swob.py | 2 +- swift/common/utils/__init__.py | 2 +- swift/common/wsgi.py | 2 +- swift/obj/diskfile.py | 9 ++++----- 8 files changed, 33 insertions(+), 16 deletions(-) diff --git a/bandit.yaml b/bandit.yaml index a33fd24451..5da39f155d 100644 --- a/bandit.yaml +++ b/bandit.yaml @@ -79,10 +79,28 @@ # B703 : django_mark_safe # (optional) list included test IDs here, eg '[B101, B406]': -tests: [B102, B103, B302, B303, B304, B305, B306, B307, B308, B310, B401, B501, B502, B506, B601, B602, B609] +tests: # (optional) list skipped test IDs here, eg '[B101, B406]': skips: + # We default to binding to all interfaces + - B104 + # Yes, we sometimes catch just to quietly swallow an exception + - B110 + # We use insecure randomness all over the place, because + # it's exceedingly rare that we need secure randomness + - B311 + # We dynamically build SQL all over the place + - B608 + # We often use subprocesses, and require a lot of trust in our use of them + - B404 + - B603 + - B607 + # We parse xml + - B320 + - B405 + - B410 + - B603 ### (optional) plugin settings - some test plugins require configuration data ### that may be given here, per-plugin. All bandit test plugins have a built in diff --git a/swift/cli/account_audit.py b/swift/cli/account_audit.py index 09fd7490f4..b809f0c7e3 100755 --- a/swift/cli/account_audit.py +++ b/swift/cli/account_audit.py @@ -17,7 +17,6 @@ from __future__ import print_function import os import sys -from hashlib import md5 import getopt from itertools import chain @@ -28,6 +27,7 @@ from six.moves.urllib.parse import quote from swift.common.ring import Ring from swift.common.utils import split_path +from swift.common.utils.base import md5 from swift.common.bufferedhttp import http_connect @@ -91,7 +91,7 @@ class Auditor(object): conn = http_connect(node['ip'], node['port'], node['device'], part, 'GET', path, {}) resp = conn.getresponse() - calc_hash = md5() + calc_hash = md5(usedforsecurity=False) chunk = True while chunk: chunk = resp.read(8192) diff --git a/swift/common/middleware/bulk.py b/swift/common/middleware/bulk.py index df0c881a74..6484508057 100644 --- a/swift/common/middleware/bulk.py +++ b/swift/common/middleware/bulk.py @@ -197,7 +197,7 @@ payload sent to the proxy (the list of objects/containers to be deleted). import json import six import tarfile -from xml.sax import saxutils +from xml.sax.saxutils import escape # nosec B406 from time import time from eventlet import sleep import zlib @@ -247,14 +247,14 @@ def get_response_body(data_format, data_dict, error_list, root_tag): xml_key = key.replace(' ', '_').lower() output.extend([ '<', xml_key, '>', - saxutils.escape(str(data_dict[key])), + escape(str(data_dict[key])), '\n', ]) output.append('\n') for name, status in error_list: output.extend([ - '', saxutils.escape(name), '', - saxutils.escape(status), '\n', + '', escape(name), '', + escape(status), '\n', ]) output.extend(['\n\n']) if six.PY2: diff --git a/swift/common/middleware/xprofile.py b/swift/common/middleware/xprofile.py index 36425e6fa8..84d812acf6 100644 --- a/swift/common/middleware/xprofile.py +++ b/swift/common/middleware/xprofile.py @@ -97,7 +97,7 @@ from swift.common.middleware.x_profile.html_viewer import HTMLViewer from swift.common.middleware.x_profile.profile_model import ProfileLog -DEFAULT_PROFILE_PREFIX = '/tmp/log/swift/profile/default.profile' +DEFAULT_PROFILE_PREFIX = '/tmp/log/swift/profile/default.profile' # nosec B108 # unwind the iterator; it may call start_response, do lots of work, etc PROFILE_EXEC_EAGER = """ diff --git a/swift/common/swob.py b/swift/common/swob.py index 51841dbb52..60ccb332be 100644 --- a/swift/common/swob.py +++ b/swift/common/swob.py @@ -730,7 +730,7 @@ class Accept(object): """ # RFC 2616 section 2.2 - token = r'[^()<>@,;:\"/\[\]?={}\x00-\x20\x7f]+' + token = r'[^()<>@,;:\"/\[\]?={}\x00-\x20\x7f]+' # nosec B105 qdtext = r'[^"]' quoted_pair = r'(?:\\.)' quoted_string = r'"(?:' + qdtext + r'|' + quoted_pair + r')*"' diff --git a/swift/common/utils/__init__.py b/swift/common/utils/__init__.py index c6c3229244..054e4169ca 100644 --- a/swift/common/utils/__init__.py +++ b/swift/common/utils/__init__.py @@ -2854,7 +2854,7 @@ def friendly_close(resp): return drain_and_close(resp, read_limit=DEFAULT_DRAIN_LIMIT) -_rfc_token = r'[^()<>@,;:\"/\[\]?={}\x00-\x20\x7f]+' +_rfc_token = r'[^()<>@,;:\"/\[\]?={}\x00-\x20\x7f]+' # nosec B105 _rfc_extension_pattern = re.compile( r'(?:\s*;\s*(' + _rfc_token + r")\s*(?:=\s*(" + _rfc_token + r'|"(?:[^"\\]|\\.)*"))?)') diff --git a/swift/common/wsgi.py b/swift/common/wsgi.py index a9a22f2260..fa5367c291 100644 --- a/swift/common/wsgi.py +++ b/swift/common/wsgi.py @@ -1026,7 +1026,7 @@ def run_wsgi(conf_path, app_section, *args, **kwargs): if hasattr(os, 'set_inheritable'): # See https://www.python.org/dev/peps/pep-0446/ os.set_inheritable(write_fd, True) - os.execv(myself, sys.argv) + os.execv(myself, sys.argv) # nosec B606 logger.error('Somehow lived past os.execv()?!') exit('Somehow lived past os.execv()?!') elif child_pid == 0: diff --git a/swift/obj/diskfile.py b/swift/obj/diskfile.py index 616cda4d0f..d11e8673cc 100644 --- a/swift/obj/diskfile.py +++ b/swift/obj/diskfile.py @@ -1087,11 +1087,10 @@ class BaseDiskFileManager(object): key = info_key[:-5] + '_file' results[key] = join(datadir, info['filename']) if info else None - if verify: - assert self._verify_ondisk_files( - results, **kwargs), \ - "On-disk file search algorithm contract is broken: %s" \ - % str(results) + if verify and not self._verify_ondisk_files(results, **kwargs): + raise RuntimeError( + "On-disk file search algorithm contract is broken: %s" + % str(results)) return results