From 9dbcbd0dcd0da2ac579cbaff7c57286b2716003f Mon Sep 17 00:00:00 2001 From: Steve Breker Date: Tue, 19 Apr 2022 21:59:48 -0700 Subject: [PATCH 1/6] Integrate Python Bandit Integrate Bandit for static security scanning of the a3m codebase. Run using tox. e.g. make tox ARG=bandit --- .github/workflows/tests.yml | 1 + requirements-dev.txt | 14 ++++++++++++++ setup.cfg | 1 + tox.ini | 6 +++++- 4 files changed, 21 insertions(+), 1 deletion(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 0115bc84..9045114c 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -17,6 +17,7 @@ jobs: - {name: Type, python: '3.9', os: ubuntu-latest, tox: type, codecov: false} - {name: Pylint, python: '3.9', os: ubuntu-latest, tox: pylint, codecov: false} - {name: Vulture, python: '3.9', os: ubuntu-latest, tox: vulture, codecov: false} + - {name: Bandit, python: '3.9', os: ubuntu-latest, tox: bandit, codecov: false} steps: - name: Check out source code uses: actions/checkout@v2 diff --git a/requirements-dev.txt b/requirements-dev.txt index c840999f..da5bfa46 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -22,6 +22,8 @@ babel==2.9.1 # via sphinx bagit==1.8.1 # via a3m (setup.py) +bandit==1.7.4 + # via a3m (setup.py) black==22.3.0 # via a3m (setup.py) boto3==1.21.38 @@ -67,6 +69,10 @@ flake8==4.0.1 # via a3m (setup.py) future==0.18.2 # via metsrw +gitdb==4.0.9 + # via gitpython +gitpython==3.1.27 + # via bandit googleapis-common-protos==1.56.0 # via # a3m (setup.py) @@ -137,6 +143,8 @@ packaging==21.3 # tox pathspec==0.9.0 # via black +pbr==5.8.1 + # via stevedore pep517==0.12.0 # via pip-tools pip-tools==6.6.0 @@ -200,6 +208,7 @@ pytz==2022.1 # django pyyaml==6.0 # via + # bandit # pre-commit # vcrpy releases==1.6.3 @@ -218,9 +227,12 @@ six==1.16.0 # via # grpcio # metsrw + # python-dateutil # tox # vcrpy # virtualenv +smmap==5.0.0 + # via gitdb snowballstemmer==2.2.0 # via sphinx sphinx==4.5.0 @@ -243,6 +255,8 @@ sphinxcontrib-serializinghtml==1.1.5 # via sphinx sqlparse==0.4.2 # via django +stevedore==3.5.0 + # via bandit tenacity==8.0.1 # via a3m (setup.py) toml==0.10.2 diff --git a/setup.cfg b/setup.cfg index 3df4951d..9f697684 100644 --- a/setup.cfg +++ b/setup.cfg @@ -77,6 +77,7 @@ dev = vulture black flake8 + bandit pre-commit sphinx sphinxcontrib-mermaid diff --git a/tox.ini b/tox.ini index ee4981f9..19769ad4 100644 --- a/tox.ini +++ b/tox.ini @@ -1,7 +1,7 @@ [tox] skipsdist = True minversion = 3.14.6 -envlist = py, lint, pylint, type, vulture +envlist = py, lint, pylint, type, vulture, bandit [testenv] basepython = python3.9 @@ -34,6 +34,10 @@ commands = skip_install = True commands = - vulture --exclude="a3m/settings,a3m/main/models.py,a3m/main/migrations,a3m/fpr/models.py,a3m/fpr/migrations,a3m/server/rpc/proto" a3m +[testenv:bandit] +skip_install = True +commands = bandit -r a3m + [flake8] exclude = .tox, .git, __pycache__, .cache, build, dist, *.pyc, *.egg-info, .eggs application-import-names = flake8 From d12aa9e9e5d7e2883a6b691a08a6a475adf91c59 Mon Sep 17 00:00:00 2001 From: Steve Breker Date: Tue, 10 May 2022 17:02:14 -0700 Subject: [PATCH 2/6] Adjust tmp file access permissions Remove execution permission when opening files during the check for submission documentation and in the server task class. Refactor executeOrRunSubProcess.py to use the Python tempfile module. --- .../check_for_submission_documentation.py | 2 +- a3m/executeOrRunSubProcess.py | 36 +++++++++---------- a3m/server/tasks/task.py | 2 +- 3 files changed, 20 insertions(+), 20 deletions(-) diff --git a/a3m/client/clientScripts/check_for_submission_documentation.py b/a3m/client/clientScripts/check_for_submission_documentation.py index f49323b6..14e28143 100644 --- a/a3m/client/clientScripts/check_for_submission_documentation.py +++ b/a3m/client/clientScripts/check_for_submission_documentation.py @@ -31,4 +31,4 @@ def call(jobs): f = open(fileName, "a") f.write("No submission documentation added") f.close() - os.chmod(fileName, 488) + os.chmod(fileName, 0o660) diff --git a/a3m/executeOrRunSubProcess.py b/a3m/executeOrRunSubProcess.py index b57eea24..4c3a5fb0 100644 --- a/a3m/executeOrRunSubProcess.py +++ b/a3m/executeOrRunSubProcess.py @@ -20,7 +20,7 @@ import shlex import subprocess import sys -import uuid +import tempfile def launchSubProcess( @@ -131,25 +131,25 @@ def launchSubProcess( def createAndRunScript( text, stdIn="", printing=False, arguments=[], env_updates={}, capture_output=True ): - # Output the text to a /tmp/ file - scriptPath = "/tmp/" + uuid.uuid4().__str__() - FILE = os.open(scriptPath, os.O_WRONLY | os.O_CREAT, 0o770) - os.write(FILE, text.encode("utf8")) - os.close(FILE) - cmd = [scriptPath] - cmd.extend(arguments) - - # Run it - ret = launchSubProcess( - cmd, - stdIn="", - printing=printing, - env_updates=env_updates, - capture_output=capture_output, - ) + temp = tempfile.NamedTemporaryFile(mode="w+t", encoding="utf-8") + try: + os.chmod(temp.name, 0o770) + + cmd = [temp.name] + cmd.extend(arguments) + + # Run it + ret = launchSubProcess( + cmd, + stdIn="", + printing=printing, + env_updates=env_updates, + capture_output=capture_output, + ) # Remove the temp file - os.remove(scriptPath) + finally: + temp.close() return ret diff --git a/a3m/server/tasks/task.py b/a3m/server/tasks/task.py index 69cb99ac..ed0e2788 100644 --- a/a3m/server/tasks/task.py +++ b/a3m/server/tasks/task.py @@ -103,7 +103,7 @@ def _write_file_to_disk(self, path, contents): try: with open(path, "a") as f: f.write(contents) - os.chmod(path, 0o750) + os.chmod(path, 0o640) except Exception: logger.exception("Unable to write to: %s", path) From 8f04e07f0fb373e4d26e8bb56b2b8b48920985b0 Mon Sep 17 00:00:00 2001 From: Steve Breker Date: Tue, 10 May 2022 17:12:51 -0700 Subject: [PATCH 3/6] Random.uniform() triggering Bandit warning Bandit warning B311 is being triggered by the use of random.uniform(). Since this is safe to use for non-security purposes I am flagging with nosec. B311: "Standard pseudo-random generators are not suitable for security/cryptographic purposes." --- a3m/databaseFunctions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/a3m/databaseFunctions.py b/a3m/databaseFunctions.py index 2fd2c307..7e742b7c 100644 --- a/a3m/databaseFunctions.py +++ b/a3m/databaseFunctions.py @@ -263,4 +263,4 @@ def retryOnFailure(description, callback, retries=10): retry + 1, e, ) - time.sleep(random.uniform(0, 2)) + time.sleep(random.uniform(0, 2)) # nosec B311 From d499575077c68da6d09fcd7fdc69b2ea4bd9af01 Mon Sep 17 00:00:00 2001 From: Steve Breker Date: Tue, 10 May 2022 23:05:54 -0700 Subject: [PATCH 4/6] Use specific error type Update try-catch blocks to react to specific re.error type instead of all exeception types. This change should cause Bandit error B110 "try_except_pass" to not be triggered in models.py. --- a3m/main/models.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/a3m/main/models.py b/a3m/main/models.py index 44292c6f..c51eab3b 100644 --- a/a3m/main/models.py +++ b/a3m/main/models.py @@ -588,13 +588,13 @@ def get_directory_name(self, default=None): r"^.*/(?P.*)-" r"[\w]{8}(-[\w]{4})" r"{3}-[\w]{12}[/]{0,1}$", self.directory, ).group("directory") - except Exception: + except re.error: pass try: return re.search(r"^.*/(?P.*)/$", self.directory).group( "directory" ) - except Exception: + except re.error: pass From f9548890936019eb2f5903df6d762a546cb70235 Mon Sep 17 00:00:00 2001 From: Steve Breker Date: Wed, 11 May 2022 15:53:11 -0700 Subject: [PATCH 5/6] Use shlex.split() with subprocess Added call to shlex.split() to tokenize the command args avoiding the possibily of injecting additional commands with a semicolon. Ignore B404, B603 warnings regarding the use of subprocess. Use of subprocess should be reevaluated. --- a3m/client/clientScripts/verify_checksum.py | 9 ++++++--- a3m/client/clientScripts/virus_scan.py | 7 +++++-- a3m/executeOrRunSubProcess.py | 6 +++--- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/a3m/client/clientScripts/verify_checksum.py b/a3m/client/clientScripts/verify_checksum.py index 33419239..d7fbeffd 100644 --- a/a3m/client/clientScripts/verify_checksum.py +++ b/a3m/client/clientScripts/verify_checksum.py @@ -29,7 +29,8 @@ import datetime import logging import os -import subprocess +import shlex +import subprocess # nosec B404 import sys import uuid @@ -95,9 +96,11 @@ def _call(self, *args, **kwargs): """Make the call to Python subprocess and record the command being called. """ - self.command_called = (self.COMMAND,) + args + self.command_called = shlex.split((self.COMMAND,) + args) return self._decode( - subprocess.check_output(self.command_called, cwd=kwargs.get("transfer_dir")) + subprocess.check_output( # nosec B603 + self.command_called, cwd=kwargs.get("transfer_dir") + ) ) def count_and_compare_lines(self, objects_dir): diff --git a/a3m/client/clientScripts/virus_scan.py b/a3m/client/clientScripts/virus_scan.py index 53e27817..7cfdf05d 100644 --- a/a3m/client/clientScripts/virus_scan.py +++ b/a3m/client/clientScripts/virus_scan.py @@ -20,7 +20,8 @@ import logging import os import re -import subprocess +import shlex +import subprocess # nosec B404 import uuid from clamd import BufferTooLongError @@ -176,7 +177,9 @@ class ClamScanner(ScannerBase): COMMAND = "clamscan" def _call(self, *args): - return subprocess.check_output((self.COMMAND,) + args) + return subprocess.check_output( # nosec B603 + shlex.split((self.COMMAND,) + args) + ) def scan(self, path): passed, state, details = (False, "ERROR", None) diff --git a/a3m/executeOrRunSubProcess.py b/a3m/executeOrRunSubProcess.py index 4c3a5fb0..c0a92d66 100644 --- a/a3m/executeOrRunSubProcess.py +++ b/a3m/executeOrRunSubProcess.py @@ -18,7 +18,7 @@ import io import os import shlex -import subprocess +import subprocess # nosec B404 import sys import tempfile @@ -89,7 +89,7 @@ def launchSubProcess( raise Exception("stdIn must be a string or a file object") if capture_output: # Capture the stdout and stderr of the subprocess - p = subprocess.Popen( + p = subprocess.Popen( # nosec B603 command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, @@ -100,7 +100,7 @@ def launchSubProcess( else: # Ignore the stdout of the subprocess, capturing only stderr with open(os.devnull, "w") as devnull: - p = subprocess.Popen( + p = subprocess.Popen( # nosec B603 command, stdin=stdin_pipe, env=my_env, From a379e2940ee15977342da1d218ee890e1faf5e5c Mon Sep 17 00:00:00 2001 From: Steve Breker Date: Wed, 11 May 2022 16:52:13 -0700 Subject: [PATCH 6/6] Add .bandit ini file Add .bandit ini file. Set to skip running Bandit check B410 which recommends the use of the deprecated defusedxml package. --- .bandit | 2 ++ tox.ini | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) create mode 100644 .bandit diff --git a/.bandit b/.bandit new file mode 100644 index 00000000..5b0c063b --- /dev/null +++ b/.bandit @@ -0,0 +1,2 @@ +[bandit] +skips: B410 diff --git a/tox.ini b/tox.ini index 19769ad4..a7707023 100644 --- a/tox.ini +++ b/tox.ini @@ -36,7 +36,7 @@ commands = - vulture --exclude="a3m/settings,a3m/main/models.py,a3m/main/migrati [testenv:bandit] skip_install = True -commands = bandit -r a3m +commands = bandit -r a3m --ini .bandit [flake8] exclude = .tox, .git, __pycache__, .cache, build, dist, *.pyc, *.egg-info, .eggs